Kernel Newbies archive on lore.kernel.org
 help / color / Atom feed
From: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
To: "Bjørn Mork" <bjorn@mork.no>
Cc: kernelnewbies@kernelnewbies.org
Subject: Re: [PATCH] staging: rtl8192u: specify printk's KERN_<LEVEL> in ieee80211
Date: Wed, 15 Jan 2020 16:47:47 +1300
Message-ID: <20200115034747.GA673667@VinodS820.SoltiusNZ.lan> (raw)
In-Reply-To: <874kwzyby4.fsf@miraculix.mork.no>

On Mon, Jan 13, 2020 at 09:53:23AM +0100, Bjørn Mork wrote:
> 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().

Done that in newer version of the patch. I also removed references to 
netdevice->name as this is populated automatically when using netdev_*
(Correct me if I'm wrong pleae)

https://elixir.bootlin.com/linux/v5.4.11/source/net/core/dev.c#L9980
https://elixir.bootlin.com/linux/v5.4.11/source/include/linux/netdevice.h#L4682

> 
> 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.

Fascinating! I hand't come across this before. Thanks :)

> 
> 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.
 
What I did in this new patch was to receive an additional parameter on
the parse_subframe method so it could have access to a netdevice ref.

Do you agree with this approach or do you suggest me to address it 
differently?
> 
> 
> 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


Hi Greg, Hi Bjørn,

I appreciate the comments that both of you made regarding my original patch.

I implemented those suggestions, however, this time I replaced also the existing 
printk calls (with dbg lvl set) to keep it consistent.  

Please let me know if you think I should change anything else.


The original commit message follows

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.

Once they are fixed, checkpatch reports 'WARNING: Prefer [subsystem eg:
netdev]_dbg([subsystem]dev, ... then dev_dbg(dev, ... then
pr_debug(...  to printk(KERN_DEBUG ...'. Fix this by replacing
relevant printk_<level> statements with their netdev_<level>
equivalent.

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 .../staging/rtl8192u/ieee80211/ieee80211_rx.c | 62 +++++++++----------
 1 file changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
index 00fea127bdc3..e101f7b13c7e 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
@@ -232,8 +232,7 @@ ieee80211_rx_frame_mgmt(struct ieee80211_device *ieee, struct sk_buff *skb,

 	#ifdef NOT_YET
 	if (ieee->iw_mode == IW_MODE_MASTER) {
-		printk(KERN_DEBUG "%s: Master mode not yet supported.\n",
-		       ieee->dev->name);
+		netdev_dbg(ieee->dev, "Master mode not yet supported.\n");
 		return 0;
 /*
   hostap_update_sta_ps(ieee, (struct hostap_ieee80211_hdr_4addr *)
@@ -261,9 +260,9 @@ ieee80211_rx_frame_mgmt(struct ieee80211_device *ieee, struct sk_buff *skb,

 	    if (ieee->iw_mode == IW_MODE_MASTER) {
 		if (type != WLAN_FC_TYPE_MGMT && type != WLAN_FC_TYPE_CTRL) {
-			printk(KERN_DEBUG "%s: unknown management frame "
+			netdev_dbg(skb->dev, "unknown management frame "
 			       "(type=0x%02x, stype=0x%02x) dropped\n",
-			       skb->dev->name, type, stype);
+			       type, stype);
 			return -1;
 		}

@@ -271,8 +270,8 @@ ieee80211_rx_frame_mgmt(struct ieee80211_device *ieee, struct sk_buff *skb,
 		return 0;
 	}

-	printk(KERN_DEBUG "%s: hostap_rx_frame_mgmt: management frame "
-	       "received in non-Host AP mode\n", skb->dev->name);
+	netdev_dbg(skb->dev, "hostap_rx_frame_mgmt: management frame "
+	       "received in non-Host AP mode\n");
 	return -1;
 	#endif
 }
@@ -349,9 +348,9 @@ ieee80211_rx_frame_decrypt(struct ieee80211_device *ieee, struct sk_buff *skb,
 	if (ieee->tkip_countermeasures &&
 	    strcmp(crypt->ops->name, "TKIP") == 0) {
 		if (net_ratelimit()) {
-			printk(KERN_DEBUG "%s: TKIP countermeasures: dropped "
+			netdev_dbg(ieee->dev, "TKIP countermeasures: dropped "
 			       "received packet from %pM\n",
-			       ieee->dev->name, hdr->addr2);
+			       hdr->addr2);
 		}
 		return -1;
 	}
@@ -397,9 +396,9 @@ ieee80211_rx_frame_decrypt_msdu(struct ieee80211_device *ieee, struct sk_buff *s
 	res = crypt->ops->decrypt_msdu(skb, keyidx, hdrlen, crypt->priv);
 	atomic_dec(&crypt->refcnt);
 	if (res < 0) {
-		printk(KERN_DEBUG "%s: MSDU decryption/MIC verification failed"
+		netdev_dbg(ieee->dev, "MSDU decryption/MIC verification failed"
 		       " (SA=%pM keyidx=%d)\n",
-		       ieee->dev->name, hdr->addr2, keyidx);
+		       hdr->addr2, keyidx);
 		return -1;
 	}

@@ -749,7 +748,8 @@ static void RxReorderIndicatePacket(struct ieee80211_device *ieee,
 	kfree(prxbIndicateArray);
 }

-static u8 parse_subframe(struct sk_buff *skb,
+static u8 parse_subframe(struct ieee80211_device *ieee,
+			 struct sk_buff *skb,
 			 struct ieee80211_rx_stats *rx_stats,
 			 struct ieee80211_rxb *rxb, u8 *src, u8 *dst)
 {
@@ -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",\
-						__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);
+				netdev_dbg(ieee->dev, "A-MSDU parse error!! pRfd->nTotalSubframe : %d\n",
+					   rxb->nr_subframes);
+				netdev_dbg(ieee->dev, "A-MSDU parse error!! Subframe Length: %d\n", nSubframe_Length);
+				netdev_dbg(ieee->dev, "nRemain_Length is %d and nSubframe_Length is : %d\n", skb->len, nSubframe_Length);
+				netdev_dbg(ieee->dev, "The Packet SeqNum is %d\n", SeqNum);
 				return 0;
 			}

@@ -904,8 +904,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 	stats = &ieee->stats;

 	if (skb->len < 10) {
-		printk(KERN_INFO "%s: SKB length < 10\n",
-		       dev->name);
+		netdev_info(dev, "SKB length < 10\n");
 		goto rx_dropped;
 	}

@@ -919,7 +918,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,

 	if (HTCCheck(ieee, skb->data)) {
 		if (net_ratelimit())
-			printk("find HTCControl\n");
+			netdev_warn(dev, "find HTCControl\n");
 		hdrlen += 4;
 		rx_stats->bContainHTC = true;
 	}
@@ -1113,7 +1112,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,

 	if (ieee->host_decrypt && (fc & IEEE80211_FCTL_WEP) &&
 	    (keyidx = ieee80211_rx_frame_decrypt(ieee, skb, crypt)) < 0) {
-		printk("decrypt frame error\n");
+		netdev_dbg(ieee->dev, "decrypt frame error\n");
 		goto rx_dropped;
 	}

@@ -1141,9 +1140,8 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 			flen -= hdrlen;

 		if (frag_skb->tail + flen > frag_skb->end) {
-			printk(KERN_WARNING "%s: host decrypted and "
-			       "reassembled frame did not fit skb\n",
-			       dev->name);
+			netdev_warn(dev, "host decrypted and "
+			       "reassembled frame did not fit skb\n");
 			ieee80211_frag_cache_invalidate(ieee, hdr);
 			goto rx_dropped;
 		}
@@ -1178,7 +1176,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 	 * encrypted/authenticated */
 	if (ieee->host_decrypt && (fc & IEEE80211_FCTL_WEP) &&
 	    ieee80211_rx_frame_decrypt_msdu(ieee, skb, keyidx, crypt)) {
-		printk("==>decrypt msdu error\n");
+		netdev_dbg(ieee->dev, "==>decrypt msdu error\n");
 		goto rx_dropped;
 	}

@@ -1250,7 +1248,7 @@ int ieee80211_rx(struct ieee80211_device *ieee, struct sk_buff *skb,
 		goto rx_dropped;
 	/* to parse amsdu packets */
 	/* qos data packets & reserved bit is 1 */
-	if (parse_subframe(skb, rx_stats, rxb, src, dst) == 0) {
+	if (parse_subframe(ieee, skb, rx_stats, rxb, src, dst) == 0) {
 		/* only to free rxb, and not submit the packets to upper layer */
 		for (i = 0; i < rxb->nr_subframes; i++) {
 			dev_kfree_skb(rxb->subframes[i]);
@@ -1863,7 +1861,7 @@ int ieee80211_parse_info_param(struct ieee80211_device *ieee,
 				info_element->data[0] == 0x00 &&
 				info_element->data[1] == 0x13 &&
 				info_element->data[2] == 0x74)) {
-				printk("========>%s(): athros AP is exist\n", __func__);
+				netdev_dbg(ieee->dev, "========> athros AP is exist\n");
 				network->atheros_cap_exist = true;
 			} else
 				network->atheros_cap_exist = false;
@@ -1980,8 +1978,8 @@ int ieee80211_parse_info_param(struct ieee80211_device *ieee,
 			}
 			break;
 		case MFIE_TYPE_QOS_PARAMETER:
-			printk(KERN_ERR
-			       "QoS Error need to parse QOS_PARAMETER IE\n");
+			netdev_err(ieee->dev,
+				   "QoS Error need to parse QOS_PARAMETER IE\n");
 			break;

 		case MFIE_TYPE_COUNTRY:
@@ -2357,14 +2355,14 @@ static inline void ieee80211_process_probe_response(
 			if (IS_COUNTRY_IE_VALID(ieee)) {
 				// Case 1: Country code
 				if (!is_legal_channel(ieee, network->channel)) {
-					printk("GetScanInfo(): For Country code, filter probe response at channel(%d).\n", network->channel);
+					netdev_warn(ieee->dev, "GetScanInfo(): For Country code, filter probe response at channel(%d).\n", network->channel);
 					goto out;
 				}
 			} else {
 				// Case 2: No any country code.
 				// Filter over channel ch12~14
 				if (network->channel > 11) {
-					printk("GetScanInfo(): For Global Domain, filter probe response at channel(%d).\n", network->channel);
+					netdev_warn(ieee->dev, "GetScanInfo(): For Global Domain, filter probe response at channel(%d).\n", network->channel);
 					goto out;
 				}
 			}
@@ -2372,14 +2370,14 @@ static inline void ieee80211_process_probe_response(
 			if (IS_COUNTRY_IE_VALID(ieee)) {
 				// Case 1: Country code
 				if (!is_legal_channel(ieee, network->channel)) {
-					printk("GetScanInfo(): For Country code, filter beacon at channel(%d).\n", network->channel);
+					netdev_warn(ieee->dev, "GetScanInfo(): For Country code, filter beacon at channel(%d).\n", network->channel);
 					goto out;
 				}
 			} else {
 				// Case 2: No any country code.
 				// Filter over channel ch12~14
 				if (network->channel > 14) {
-					printk("GetScanInfo(): For Global Domain, filter beacon at channel(%d).\n", network->channel);
+					netdev_warn(ieee->dev, "GetScanInfo(): For Global Domain, filter beacon at channel(%d).\n", network->channel);
 					goto out;
 				}
 			}
--
2.24.1

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

  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
2020-01-15  3:47   ` Paulo Miguel Almeida [this message]
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=20200115034747.GA673667@VinodS820.SoltiusNZ.lan \
    --to=paulo.miguel.almeida.rodenas@gmail.com \
    --cc=bjorn@mork.no \
    --cc=kernelnewbies@kernelnewbies.org \
    /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