Kernel Newbies archive on lore.kernel.org
 help / color / Atom feed
From: "Bjørn Mork" <bjorn@mork.no>
To: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc: kernelnewbies@kernelnewbies.org
Subject: Re: [PATCH] staging: rtl8192u: specify printk's KERN_<LEVEL> in ieee80211
Date: Mon, 13 Jan 2020 09:53:23 +0100
Message-ID: <874kwzyby4.fsf@miraculix.mork.no> (raw)
In-Reply-To: <20200113050247.GA318164@localhost.localdomain> (Paulo Miguel Almeida's message of "Mon, 13 Jan 2020 18:02:47 +1300")

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

  parent reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-13  5:02 Paulo Miguel Almeida
2020-01-13  8:29 ` Greg KH
2020-01-13  8:53 ` Bjørn Mork [this message]
2020-01-15  3:47   ` Paulo Miguel Almeida
2020-01-15  8:17     ` Greg KH

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=874kwzyby4.fsf@miraculix.mork.no \
    --to=bjorn@mork.no \
    --cc=kernelnewbies@kernelnewbies.org \
    --cc=paulo.miguel.almeida.rodenas@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel Newbies archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernelnewbies/0 kernelnewbies/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernelnewbies kernelnewbies/ https://lore.kernel.org/kernelnewbies \
		kernelnewbies@kernelnewbies.org
	public-inbox-index kernelnewbies

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernelnewbies.kernelnewbies


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git