[v2] staging: wlan-ng: remove commented debug printk messages
Commit Message
printk messages are added for program flow tracing and are left
commented. These commented log messages should be removed as they
are no more useful for program execution.
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Changes in v2:
1. Resending as v2 since I incorrectly send multiple emails for the patch
earlier. Feedback from gregkh@linuxfoundation.org
drivers/staging/wlan-ng/p80211netdev.c | 22 ----------------------
1 file changed, 22 deletions(-)
--
2.30.2
Comments
On Sun, Oct 23, 2022 at 08:35:47PM +0700, Bagas Sanjaya wrote:
> On Sat, Oct 22, 2022 at 01:03:42AM +0530, Deepak R Varma wrote:
> > diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> > index e04fc666d218..6bef419e8ad0 100644
> > --- a/drivers/staging/wlan-ng/p80211netdev.c
> > +++ b/drivers/staging/wlan-ng/p80211netdev.c
> > @@ -881,55 +881,42 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> > wlandev->rx.mgmt++;
> > switch (fstype) {
> > case WLAN_FSTYPE_ASSOCREQ:
> > - /* printk("assocreq"); */
> > wlandev->rx.assocreq++;
> > break;
> > wlandev->rx.ctl_unknown++;
> > break;
> > }
> > - /* printk("\n"); */
> > drop = 2;
> > break;
> >
> > @@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> > wlandev->rx.cfack_cfpoll++;
> > break;
> > default:
> > - /* printk("unknown"); */
> > wlandev->rx.data_unknown++;
> > break;
> > }
>
> Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
Hi Sanjaya,
Sure they can, but I think they are very basic tracing message and do not appear
to carry much of information useful the event of debugging. Do you have a
suggestion on what additional information may be added to make them more useful?
If you still think we should have then in the CONFIG_DEBUG_KERNEL guard, let me
know and I will attempt to improve these.
Thank you,
./drv
>
> --
> An old man doll... just what I always wanted! - Clara
On Sat, Oct 22, 2022 at 01:03:42AM +0530, Deepak R Varma wrote:
> diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> index e04fc666d218..6bef419e8ad0 100644
> --- a/drivers/staging/wlan-ng/p80211netdev.c
> +++ b/drivers/staging/wlan-ng/p80211netdev.c
> @@ -881,55 +881,42 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> wlandev->rx.mgmt++;
> switch (fstype) {
> case WLAN_FSTYPE_ASSOCREQ:
> - /* printk("assocreq"); */
> wlandev->rx.assocreq++;
> break;
> case WLAN_FSTYPE_ASSOCRESP:
> - /* printk("assocresp"); */
> wlandev->rx.assocresp++;
> break;
> case WLAN_FSTYPE_REASSOCREQ:
> - /* printk("reassocreq"); */
> wlandev->rx.reassocreq++;
> break;
> case WLAN_FSTYPE_REASSOCRESP:
> - /* printk("reassocresp"); */
> wlandev->rx.reassocresp++;
> break;
> case WLAN_FSTYPE_PROBEREQ:
> - /* printk("probereq"); */
> wlandev->rx.probereq++;
> break;
> case WLAN_FSTYPE_PROBERESP:
> - /* printk("proberesp"); */
> wlandev->rx.proberesp++;
> break;
> case WLAN_FSTYPE_BEACON:
> - /* printk("beacon"); */
> wlandev->rx.beacon++;
> break;
> case WLAN_FSTYPE_ATIM:
> - /* printk("atim"); */
> wlandev->rx.atim++;
> break;
> case WLAN_FSTYPE_DISASSOC:
> - /* printk("disassoc"); */
> wlandev->rx.disassoc++;
> break;
> case WLAN_FSTYPE_AUTHEN:
> - /* printk("authen"); */
> wlandev->rx.authen++;
> break;
> case WLAN_FSTYPE_DEAUTHEN:
> - /* printk("deauthen"); */
> wlandev->rx.deauthen++;
> break;
> default:
> - /* printk("unknown"); */
> wlandev->rx.mgmt_unknown++;
> break;
> }
> - /* printk("\n"); */
> drop = 2;
> break;
>
> @@ -943,35 +930,27 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> wlandev->rx.ctl++;
> switch (fstype) {
> case WLAN_FSTYPE_PSPOLL:
> - /* printk("pspoll"); */
> wlandev->rx.pspoll++;
> break;
> case WLAN_FSTYPE_RTS:
> - /* printk("rts"); */
> wlandev->rx.rts++;
> break;
> case WLAN_FSTYPE_CTS:
> - /* printk("cts"); */
> wlandev->rx.cts++;
> break;
> case WLAN_FSTYPE_ACK:
> - /* printk("ack"); */
> wlandev->rx.ack++;
> break;
> case WLAN_FSTYPE_CFEND:
> - /* printk("cfend"); */
> wlandev->rx.cfend++;
> break;
> case WLAN_FSTYPE_CFENDCFACK:
> - /* printk("cfendcfack"); */
> wlandev->rx.cfendcfack++;
> break;
> default:
> - /* printk("unknown"); */
> wlandev->rx.ctl_unknown++;
> break;
> }
> - /* printk("\n"); */
> drop = 2;
> break;
>
> @@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> wlandev->rx.cfack_cfpoll++;
> break;
> default:
> - /* printk("unknown"); */
> wlandev->rx.data_unknown++;
> break;
> }
Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
On Sun, Oct 23, 2022 at 08:35:47PM +0700, Bagas Sanjaya wrote:
> On Sat, Oct 22, 2022 at 01:03:42AM +0530, Deepak R Varma wrote:
> > diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
> > index e04fc666d218..6bef419e8ad0 100644
> > --- a/drivers/staging/wlan-ng/p80211netdev.c
> > +++ b/drivers/staging/wlan-ng/p80211netdev.c
> > @@ -881,55 +881,42 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> > wlandev->rx.mgmt++;
> > switch (fstype) {
> > case WLAN_FSTYPE_ASSOCREQ:
> > - /* printk("assocreq"); */
> > wlandev->rx.assocreq++;
> > break;
> > case WLAN_FSTYPE_ASSOCRESP:
> > - /* printk("assocresp"); */
> > wlandev->rx.assocresp++;
> > break;
> > case WLAN_FSTYPE_REASSOCREQ:
> > - /* printk("reassocreq"); */
> > wlandev->rx.reassocreq++;
> > break;
> > case WLAN_FSTYPE_REASSOCRESP:
> > - /* printk("reassocresp"); */
> > wlandev->rx.reassocresp++;
> > break;
> > case WLAN_FSTYPE_PROBEREQ:
> > - /* printk("probereq"); */
> > wlandev->rx.probereq++;
> > break;
> > case WLAN_FSTYPE_PROBERESP:
> > - /* printk("proberesp"); */
> > wlandev->rx.proberesp++;
> > break;
> > case WLAN_FSTYPE_BEACON:
> > - /* printk("beacon"); */
> > wlandev->rx.beacon++;
> > break;
> > case WLAN_FSTYPE_ATIM:
> > - /* printk("atim"); */
> > wlandev->rx.atim++;
> > break;
> > case WLAN_FSTYPE_DISASSOC:
> > - /* printk("disassoc"); */
> > wlandev->rx.disassoc++;
> > break;
> > case WLAN_FSTYPE_AUTHEN:
> > - /* printk("authen"); */
> > wlandev->rx.authen++;
> > break;
> > case WLAN_FSTYPE_DEAUTHEN:
> > - /* printk("deauthen"); */
> > wlandev->rx.deauthen++;
> > break;
> > default:
> > - /* printk("unknown"); */
> > wlandev->rx.mgmt_unknown++;
> > break;
> > }
> > - /* printk("\n"); */
> > drop = 2;
> > break;
> >
> > @@ -943,35 +930,27 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> > wlandev->rx.ctl++;
> > switch (fstype) {
> > case WLAN_FSTYPE_PSPOLL:
> > - /* printk("pspoll"); */
> > wlandev->rx.pspoll++;
> > break;
> > case WLAN_FSTYPE_RTS:
> > - /* printk("rts"); */
> > wlandev->rx.rts++;
> > break;
> > case WLAN_FSTYPE_CTS:
> > - /* printk("cts"); */
> > wlandev->rx.cts++;
> > break;
> > case WLAN_FSTYPE_ACK:
> > - /* printk("ack"); */
> > wlandev->rx.ack++;
> > break;
> > case WLAN_FSTYPE_CFEND:
> > - /* printk("cfend"); */
> > wlandev->rx.cfend++;
> > break;
> > case WLAN_FSTYPE_CFENDCFACK:
> > - /* printk("cfendcfack"); */
> > wlandev->rx.cfendcfack++;
> > break;
> > default:
> > - /* printk("unknown"); */
> > wlandev->rx.ctl_unknown++;
> > break;
> > }
> > - /* printk("\n"); */
> > drop = 2;
> > break;
> >
> > @@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> > wlandev->rx.cfack_cfpoll++;
> > break;
> > default:
> > - /* printk("unknown"); */
> > wlandev->rx.data_unknown++;
> > break;
> > }
>
> Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
No, they should just be removed as was done here.
thanks,
greg k-h
On 10/23/22 21:13, Greg KH wrote:
>>> @@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
>>> wlandev->rx.cfack_cfpoll++;
>>> break;
>>> default:
>>> - /* printk("unknown"); */
>>> wlandev->rx.data_unknown++;
>>> break;
>>> }
>>
>> Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
>
> No, they should just be removed as was done here.
>
What if in case of debugging without these printks?
On 10/22/22 05:40, Deepak R Varma wrote:
> On Sun, Oct 23, 2022 at 08:35:47PM +0700, Bagas Sanjaya wrote:
>> On Sat, Oct 22, 2022 at 01:03:42AM +0530, Deepak R Varma wrote:
>>> diff --git a/drivers/staging/wlan-ng/p80211netdev.c b/drivers/staging/wlan-ng/p80211netdev.c
>>> index e04fc666d218..6bef419e8ad0 100644
>>> --- a/drivers/staging/wlan-ng/p80211netdev.c
>>> +++ b/drivers/staging/wlan-ng/p80211netdev.c
>>> @@ -881,55 +881,42 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
>>> wlandev->rx.mgmt++;
>>> switch (fstype) {
>>> case WLAN_FSTYPE_ASSOCREQ:
>>> - /* printk("assocreq"); */
>>> wlandev->rx.assocreq++;
>>> break;
>>> wlandev->rx.ctl_unknown++;
>>> break;
>>> }
>>> - /* printk("\n"); */
>>> drop = 2;
>>> break;
>>>
>>> @@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
>>> wlandev->rx.cfack_cfpoll++;
>>> break;
>>> default:
>>> - /* printk("unknown"); */
>>> wlandev->rx.data_unknown++;
>>> break;
>>> }
>>
>> Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
>
> Hi Sanjaya,
> Sure they can, but I think they are very basic tracing message and do not appear
> to carry much of information useful the event of debugging. Do you have a
> suggestion on what additional information may be added to make them more useful?
>
> If you still think we should have then in the CONFIG_DEBUG_KERNEL guard, let me
> know and I will attempt to improve these.
>
> Thank you,
> ./drv
>
Greg said we should just deleting these printks [1].
[1]: https://lore.kernel.org/lkml/Y1VL%2FwITM64U6qLi@kroah.com/
Thanks anyway.
On Mon, Oct 24, 2022 at 10:08:00AM +0700, Bagas Sanjaya wrote:
> On 10/23/22 21:13, Greg KH wrote:
> >>> @@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
> >>> wlandev->rx.cfack_cfpoll++;
> >>> break;
> >>> default:
> >>> - /* printk("unknown"); */
> >>> wlandev->rx.data_unknown++;
> >>> break;
> >>> }
> >>
> >> Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
> >
> > No, they should just be removed as was done here.
> >
>
> What if in case of debugging without these printks?
I can not parse this line, sorry.
On 10/24/22 11:08, Greg KH wrote:
>>>> Shouldn't these printks be guarded under CONFIG_DEBUG_KERNEL instead?
>>>
>>> No, they should just be removed as was done here.
>>>
>>
>> What if in case of debugging without these printks?
>
> I can not parse this line, sorry.
OK.
Ah, I mean I have to see Documentation/dev-tools/kgdb.rst and
Documentation/dev-tools/gdb-kernel-debugging.rst.
@@ -881,55 +881,42 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
wlandev->rx.mgmt++;
switch (fstype) {
case WLAN_FSTYPE_ASSOCREQ:
- /* printk("assocreq"); */
wlandev->rx.assocreq++;
break;
case WLAN_FSTYPE_ASSOCRESP:
- /* printk("assocresp"); */
wlandev->rx.assocresp++;
break;
case WLAN_FSTYPE_REASSOCREQ:
- /* printk("reassocreq"); */
wlandev->rx.reassocreq++;
break;
case WLAN_FSTYPE_REASSOCRESP:
- /* printk("reassocresp"); */
wlandev->rx.reassocresp++;
break;
case WLAN_FSTYPE_PROBEREQ:
- /* printk("probereq"); */
wlandev->rx.probereq++;
break;
case WLAN_FSTYPE_PROBERESP:
- /* printk("proberesp"); */
wlandev->rx.proberesp++;
break;
case WLAN_FSTYPE_BEACON:
- /* printk("beacon"); */
wlandev->rx.beacon++;
break;
case WLAN_FSTYPE_ATIM:
- /* printk("atim"); */
wlandev->rx.atim++;
break;
case WLAN_FSTYPE_DISASSOC:
- /* printk("disassoc"); */
wlandev->rx.disassoc++;
break;
case WLAN_FSTYPE_AUTHEN:
- /* printk("authen"); */
wlandev->rx.authen++;
break;
case WLAN_FSTYPE_DEAUTHEN:
- /* printk("deauthen"); */
wlandev->rx.deauthen++;
break;
default:
- /* printk("unknown"); */
wlandev->rx.mgmt_unknown++;
break;
}
- /* printk("\n"); */
drop = 2;
break;
@@ -943,35 +930,27 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
wlandev->rx.ctl++;
switch (fstype) {
case WLAN_FSTYPE_PSPOLL:
- /* printk("pspoll"); */
wlandev->rx.pspoll++;
break;
case WLAN_FSTYPE_RTS:
- /* printk("rts"); */
wlandev->rx.rts++;
break;
case WLAN_FSTYPE_CTS:
- /* printk("cts"); */
wlandev->rx.cts++;
break;
case WLAN_FSTYPE_ACK:
- /* printk("ack"); */
wlandev->rx.ack++;
break;
case WLAN_FSTYPE_CFEND:
- /* printk("cfend"); */
wlandev->rx.cfend++;
break;
case WLAN_FSTYPE_CFENDCFACK:
- /* printk("cfendcfack"); */
wlandev->rx.cfendcfack++;
break;
default:
- /* printk("unknown"); */
wlandev->rx.ctl_unknown++;
break;
}
- /* printk("\n"); */
drop = 2;
break;
@@ -1007,7 +986,6 @@ static int p80211_rx_typedrop(struct wlandevice *wlandev, u16 fc)
wlandev->rx.cfack_cfpoll++;
break;
default:
- /* printk("unknown"); */
wlandev->rx.data_unknown++;
break;
}