Re: [Outreachy kernel] [PATCH v2] staging: rtl8192u: Replace printk() with appropriate net_*macro_ratelimited()
On Tue, 6 Mar 2018, Arushi Singhal wrote: > > > On Mon, Mar 5, 2018 at 1:07 PM, Julia Lawall wrote: > > > On Mon, 5 Mar 2018, Arushi Singhal wrote: > > > Replace printk having a log level with the appropriate > > net_*macro_ratelimited. > > It's better to use actual device name as a prefix in error > messages. > > I asked several questions about the above two lines, which as > far as I can > see haven't been answered. Please excuse me if I missed the > answers, but I > would still like to know what they are. > > > Hi Julia > Sorry, I missed your above two question on my last patch and only saw the > suggestions :(. > > -Why did I choose this function? > > Here we are using printk() which is the raw way to print something and it is > always preferred to use dev_*macro() and netdev_*macro() because of the > benefits they provide like dynamic debugging interface. > > And as we are using network subsystem so, netdev_*macro() or > net_*ratelimited() can be used to standardized the output within the network > subsystem. > > > > Namely, why did you choose to change to rate limited output, and > what does > the "device name as a prefix" comment mean? > > > Yes your point is correct, as before function used is printk(), so changing > it to ratelimit is not a good idea. So here I should have preferred changing > it to netdev_*macro(). > > As every kernel subsystem has its own printk format like net_* for network s > ubsystem. And it is preferred to use them to standardize the format of the o > utput within the subsystem. Sure, but I don't think the ratelimited function does that. At least, it doesn't seem to take any arguments that will help it with that. julia > Best > Arushi > > julia > > > Indentation is also changed, to fix the checkpatch issue if > line is not > > exceding 80 characters. > > > > Signed-off-by: Arushi Singhal > > > --- > > changes in v2 > > *change the subject line, in v1 driver name was wrong. > > > > .../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 22 > +++--- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git > a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > > index e6648f7..200fe5f 100644 > > --- > a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > > +++ > b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > > @@ -73,7 +73,7 @@ static void *ieee80211_ccmp_init(int > key_idx) > > > > priv->tfm = (void *)crypto_alloc_cipher("aes", 0, > CRYPTO_ALG_ASYNC); > > if (IS_ERR(priv->tfm)) { > > - printk(KERN_DEBUG "ieee80211_crypt_ccmp: could > not allocate crypto API aes\n"); > > + net_dbg_ratelimited("ieee80211_crypt_ccmp: could > not allocate crypto API aes\n"); > > priv->tfm = NULL; > > goto fail; > > } > > @@ -276,22 +276,22 @@ static int ieee80211_ccmp_decrypt(struct > sk_buff *skb, int hdr_len, void *priv) > > keyidx = pos[3]; > > if (!(keyidx & (1 << 5))) { > > if (net_ratelimit()) { > > - printk(KERN_DEBUG "CCMP: received packet > without ExtIV flag from %pM\n", > > - hdr->addr2); > > + net_dbg_ratelimited("CCMP: received > packet without ExtIV flag from %pM\n", > > + hdr->addr2); > > } > > key->dot11RSNAStatsCCMPFormatErrors++; > > return -2; > > } > > keyidx >>= 6; > > if (key->key_idx != keyidx) { > > - printk(KERN_DEBUG "CCMP: RX tkey->key_idx=%d > frame keyidx=%d priv=%p\n", > > - key->key_idx, keyidx, priv); > > + net_dbg_ratelimited("CCMP: RX tkey->key_idx=%d > frame keyidx=%d priv=%p\n", > > + key->key_idx, keyidx, priv); > > return -6; > > } > > if (!key->key_set) { > > if (net_ratelimit()) { > > - printk(KERN_DEBUG "CCMP: received packet > from %pM with keyid=%d that does not have a configured key\n", > > - hdr->addr2, keyidx); > > + net_dbg_ratelimited("CCMP: received > packet from %pM with keyid=%d that does not have a configured > key\n", > > + hdr->addr2, keyidx); > > } > > return -3; > > } > > @@ -306,8
Re: [Outreachy kernel] [PATCH v2] staging: rtl8192u: Replace printk() with appropriate net_*macro_ratelimited()
On Mon, 5 Mar 2018, Arushi Singhal wrote: > Replace printk having a log level with the appropriate > net_*macro_ratelimited. > It's better to use actual device name as a prefix in error messages. I asked several questions about the above two lines, which as far as I can see haven't been answered. Please excuse me if I missed the answers, but I would still like to know what they are. Namely, why did you choose to change to rate limited output, and what does the "device name as a prefix" comment mean? julia > Indentation is also changed, to fix the checkpatch issue if line is not > exceding 80 characters. > > Signed-off-by: Arushi Singhal > --- > changes in v2 > *change the subject line, in v1 driver name was wrong. > > .../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 22 > +++--- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > index e6648f7..200fe5f 100644 > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_ccmp.c > @@ -73,7 +73,7 @@ static void *ieee80211_ccmp_init(int key_idx) > > priv->tfm = (void *)crypto_alloc_cipher("aes", 0, CRYPTO_ALG_ASYNC); > if (IS_ERR(priv->tfm)) { > - printk(KERN_DEBUG "ieee80211_crypt_ccmp: could not allocate > crypto API aes\n"); > + net_dbg_ratelimited("ieee80211_crypt_ccmp: could not allocate > crypto API aes\n"); > priv->tfm = NULL; > goto fail; > } > @@ -276,22 +276,22 @@ static int ieee80211_ccmp_decrypt(struct sk_buff *skb, > int hdr_len, void *priv) > keyidx = pos[3]; > if (!(keyidx & (1 << 5))) { > if (net_ratelimit()) { > - printk(KERN_DEBUG "CCMP: received packet without ExtIV > flag from %pM\n", > - hdr->addr2); > + net_dbg_ratelimited("CCMP: received packet without > ExtIV flag from %pM\n", > + hdr->addr2); > } > key->dot11RSNAStatsCCMPFormatErrors++; > return -2; > } > keyidx >>= 6; > if (key->key_idx != keyidx) { > - printk(KERN_DEBUG "CCMP: RX tkey->key_idx=%d frame keyidx=%d > priv=%p\n", > - key->key_idx, keyidx, priv); > + net_dbg_ratelimited("CCMP: RX tkey->key_idx=%d frame keyidx=%d > priv=%p\n", > + key->key_idx, keyidx, priv); > return -6; > } > if (!key->key_set) { > if (net_ratelimit()) { > - printk(KERN_DEBUG "CCMP: received packet from %pM with > keyid=%d that does not have a configured key\n", > - hdr->addr2, keyidx); > + net_dbg_ratelimited("CCMP: received packet from %pM > with keyid=%d that does not have a configured key\n", > + hdr->addr2, keyidx); > } > return -3; > } > @@ -306,8 +306,8 @@ static int ieee80211_ccmp_decrypt(struct sk_buff *skb, > int hdr_len, void *priv) > > if (memcmp(pn, key->rx_pn, CCMP_PN_LEN) <= 0) { > if (net_ratelimit()) { > - printk(KERN_DEBUG "CCMP: replay detected: STA=%pM > previous PN %pm received PN %pm\n", > -hdr->addr2, key->rx_pn, pn); > + net_dbg_ratelimited("CCMP: replay detected: STA=%pM > previous PN %pm received PN %pm\n", > + hdr->addr2, key->rx_pn, pn); > } > key->dot11RSNAStatsCCMPReplays++; > return -4; > @@ -341,8 +341,8 @@ static int ieee80211_ccmp_decrypt(struct sk_buff *skb, > int hdr_len, void *priv) > > if (memcmp(mic, a, CCMP_MIC_LEN) != 0) { > if (net_ratelimit()) { > - printk(KERN_DEBUG "CCMP: decrypt failed: > STA=%pM\n", > - hdr->addr2); > + net_dbg_ratelimited("CCMP: decrypt failed: > STA=%pM\n", > + hdr->addr2); > } > key->dot11RSNAStatsCCMPDecryptErrors++; > return -5; > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20180305073256.GA4347%40seema-Inspiron-15-3567. > For more options, visit https://groups.google.com/d/optout. >