Re: [PATCH] staging: rtl8192u: specify printk's KERN_<LEVEL> in ieee80211

["Bjørn Mork" <bjorn@mork.no>]

Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> writes:

> Checkpatch reports 'WARNING: printk() should include KERN_<LEVEL>
> facility level'. Fix this by specifying a relevant KERN_<LEVEL> value
> for each line in which it was missing.


Although this is true, there are also additional best practice rules wrt
use of printk in drivers and debug level printks in particular.
Checkpatch does not tell you everything, unfortunately ;-)

You should always use dev_dbg() or netif_dbg() or similar macros instead
of printk in drivers.  This way debug messages can be compiled away when
not needed, or even dynamically enabled/disabled on kernels built with
dynamic debugging.  You should also drop stuff like __func__ since that
can be enabled dynamically as necessary with dev_dbg().

Take a look at
https://www.kernel.org/doc/html/v5.4/admin-guide/dynamic-debug-howto.html
and play it if you haven't already.  This an extremely useful feature.

See some of the drivers in drivers/net/wireless for examples of how to
use dev_dbg().


> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
>  .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 24 +++++++++----------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> index 00fea127bdc3..f38986d74005 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> @@ -810,11 +810,11 @@ static u8 parse_subframe(struct sk_buff *skb,
>  			nSubframe_Length = (nSubframe_Length >> 8) + (nSubframe_Length << 8);
>  
>  			if (skb->len < (ETHERNET_HEADER_SIZE + nSubframe_Length)) {
> -				printk("%s: A-MSDU parse error!! pRfd->nTotalSubframe : %d\n",\
> +				printk(KERN_DEBUG "%s: A-MSDU parse error!! pRfd->nTotalSubframe : %d\n",
>  						__func__, rxb->nr_subframes);
> -				printk("%s: A-MSDU parse error!! Subframe Length: %d\n", __func__, nSubframe_Length);
> -				printk("nRemain_Length is %d and nSubframe_Length is : %d\n", skb->len, nSubframe_Length);
> -				printk("The Packet SeqNum is %d\n", SeqNum);
> +				printk(KERN_DEBUG "%s: A-MSDU parse error!! Subframe Length: %d\n", __func__, nSubframe_Length);
> +				printk(KERN_DEBUG "nRemain_Length is %d and nSubframe_Length is : %d\n", skb->len, nSubframe_Length);
> +				printk(KERN_DEBUG "The Packet SeqNum is %d\n", SeqNum);
>  				return 0;
>  			}
>  

I see that this function, and many others in this driver, does not
access any device specific data.  So you'll probably have to do
something about that.  A bit more work required here than just setting
the printk level.


Hmm... I was going to suggest that you looked at the driver's TODO file
just to make sure that this work isn't futile e.g because it conflicts
with planned/suggested driver restructuring.  But I see that the TODO
file is missing.  Weird.  I thought it was required for all staging
drivers.  Learning something new every day...



Bjørn

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies