| Message ID | 20221115235519.679115-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: renesas: rswitch: Fix MAC address info | expand |
On 16 Nov 08:55, Yoshihiro Shimoda wrote: >Smatch detected the following warning. > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > '%pM' cannot be followed by 'n' > >The 'n' should be '\n'. > >Reported-by: Dan Carpenter <error27@gmail.com> >Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> >Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") I would drop the Fixes tag, this shoiuldn't go to net and -stable. and please tag either [PATCH net] or [PATCH net-next], this one should be net-next. Thanks, you can add: Reviewed-by: Saeed Mahameed <saeed@kernel.org>
On Wed, Nov 16, 2022 at 03:14:53PM -0800, Saeed Mahameed wrote: > On 16 Nov 08:55, Yoshihiro Shimoda wrote: > > Smatch detected the following warning. > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > '%pM' cannot be followed by 'n' > > > > The 'n' should be '\n'. > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > I would drop the Fixes tag, this shoiuldn't go to net and -stable. and > please tag either [PATCH net] or [PATCH net-next], this one should be > net-next. > > Thanks, > you can add: > Reviewed-by: Saeed Mahameed <saeed@kernel.org> You can have a Fixes: tag on a patch sent to net-next no problem. No need to drop it. But in the future, using git-format-patch --subject-prefix="PATCH net" (or net-next) would be indeed advisable.
On Wed, Nov 16, 2022 at 03:14:53PM -0800, Saeed Mahameed wrote: > On 16 Nov 08:55, Yoshihiro Shimoda wrote: > > Smatch detected the following warning. > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > '%pM' cannot be followed by 'n' > > > > The 'n' should be '\n'. > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > I would drop the Fixes tag, this shoiuldn't go to net and -stable. Some maintainers would want a Fixes tag for this and some wouldn't... But either way Fixes doesn't really have to do with -stable. In fact sometimes the Fixes tag let's you automatically filter out fixes to code which is newer than the -stable tree so it can mean the opposite of -stable. regards, dan carpenter
On Thu, Nov 17, 2022 at 06:58:19AM +0300, Dan Carpenter wrote: > On Wed, Nov 16, 2022 at 03:14:53PM -0800, Saeed Mahameed wrote: > > On 16 Nov 08:55, Yoshihiro Shimoda wrote: > > > Smatch detected the following warning. > > > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > > '%pM' cannot be followed by 'n' > > > > > > The 'n' should be '\n'. > > > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > > > I would drop the Fixes tag, this shoiuldn't go to net and -stable. > > Some maintainers would want a Fixes tag for this and some wouldn't... Immediately after I sent this email a different maintainer asked me to add a Fixes tag to a patch removing an unnecessary NULL check to silence checker warning about inconsistent NULL checking. Generally I would have put a Fixes tag here because the typo gets to the users but I wouldn't put a Fixes tag for typos in the comments. regards, dan carpenter
On Wed, Nov 16, 2022 at 08:55:19AM +0900, Yoshihiro Shimoda wrote: > Smatch detected the following warning. > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > '%pM' cannot be followed by 'n' > > The 'n' should be '\n'. > > Reported-by: Dan Carpenter <error27@gmail.com> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/net/ethernet/renesas/rswitch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > index f3d27aef1286..51ce5c26631b 100644 > --- a/drivers/net/ethernet/renesas/rswitch.c > +++ b/drivers/net/ethernet/renesas/rswitch.c > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > } > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", You can safely drop '\n' from here. It is not needed while printing one line. Thanks > priv->rdev[i]->ndev->dev_addr); > > return 0; > -- > 2.25.1 >
Hi Vladimir, > From: Vladimir Oltean, Sent: Thursday, November 17, 2022 8:36 AM > > On Wed, Nov 16, 2022 at 03:14:53PM -0800, Saeed Mahameed wrote: > > On 16 Nov 08:55, Yoshihiro Shimoda wrote: > > > Smatch detected the following warning. > > > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > > '%pM' cannot be followed by 'n' > > > > > > The 'n' should be '\n'. > > > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > > > I would drop the Fixes tag, this shoiuldn't go to net and -stable. and > > please tag either [PATCH net] or [PATCH net-next], this one should be > > net-next. > > > > Thanks, > > you can add: > > Reviewed-by: Saeed Mahameed <saeed@kernel.org> > > You can have a Fixes: tag on a patch sent to net-next no problem. > No need to drop it. I got it. > But in the future, using git-format-patch > --subject-prefix="PATCH net" (or net-next) would be indeed advisable. I'll send v2 patch by using --subject-prefix="PATCH v2 net-next". Best regards, Yoshihiro Shimoda
Hi Leon, > From: Leon Romanovsky, Sent: Thursday, November 17, 2022 3:09 PM > > On Wed, Nov 16, 2022 at 08:55:19AM +0900, Yoshihiro Shimoda wrote: > > Smatch detected the following warning. > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > '%pM' cannot be followed by 'n' > > > > The 'n' should be '\n'. > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/net/ethernet/renesas/rswitch.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > index f3d27aef1286..51ce5c26631b 100644 > > --- a/drivers/net/ethernet/renesas/rswitch.c > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > > } > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", > > You can safely drop '\n' from here. It is not needed while printing one > line. Oh, I didn't know that. I'll remove '\n' from here on v2 patch. Best regards, Yoshihiro Shimoda > Thanks > > > priv->rdev[i]->ndev->dev_addr); > > > > return 0; > > -- > > 2.25.1 > >
Hi Shimoda-san, On Thu, Nov 17, 2022 at 9:58 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Leon Romanovsky, Sent: Thursday, November 17, 2022 3:09 PM > > On Wed, Nov 16, 2022 at 08:55:19AM +0900, Yoshihiro Shimoda wrote: > > > Smatch detected the following warning. > > > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > > '%pM' cannot be followed by 'n' > > > > > > The 'n' should be '\n'. > > > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- > > > drivers/net/ethernet/renesas/rswitch.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > > index f3d27aef1286..51ce5c26631b 100644 > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > > > } > > > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > > > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", > > > > You can safely drop '\n' from here. It is not needed while printing one > > line. > > Oh, I didn't know that. I'll remove '\n' from here on v2 patch. Please don't remove it. The convention is to have the newlines. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Nov 17, 2022 at 09:59:55AM +0100, Geert Uytterhoeven wrote: > Hi Shimoda-san, > > On Thu, Nov 17, 2022 at 9:58 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Leon Romanovsky, Sent: Thursday, November 17, 2022 3:09 PM > > > On Wed, Nov 16, 2022 at 08:55:19AM +0900, Yoshihiro Shimoda wrote: > > > > Smatch detected the following warning. > > > > > > > > drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: > > > > '%pM' cannot be followed by 'n' > > > > > > > > The 'n' should be '\n'. > > > > > > > > Reported-by: Dan Carpenter <error27@gmail.com> > > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > --- > > > > drivers/net/ethernet/renesas/rswitch.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > > > index f3d27aef1286..51ce5c26631b 100644 > > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > > > > } > > > > > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > > > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > > > > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", > > > > > > You can safely drop '\n' from here. It is not needed while printing one > > > line. > > > > Oh, I didn't know that. I'll remove '\n' from here on v2 patch. > > Please don't remove it. The convention is to have the newlines. Can you please explain why? Thanks > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Leon, On Thu, Nov 17, 2022 at 10:14 AM Leon Romanovsky <leon@kernel.org> wrote: > On Thu, Nov 17, 2022 at 09:59:55AM +0100, Geert Uytterhoeven wrote: > > On Thu, Nov 17, 2022 at 9:58 AM Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > From: Leon Romanovsky, Sent: Thursday, November 17, 2022 3:09 PM > > > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > > > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > > > > > } > > > > > > > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > > > > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > > > > > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", > > > > > > > > You can safely drop '\n' from here. It is not needed while printing one > > > > line. > > > > > > Oh, I didn't know that. I'll remove '\n' from here on v2 patch. > > > > Please don't remove it. The convention is to have the newlines. > > Can you please explain why? I'm quite sure this was discussed in the context of commits 5fd29d6ccbc98884 ("printk: clean up handling of log-levels and newlines") and 4bcc595ccd80decb ("printk: reinstate KERN_CONT for printing continuation lines"), but I couldn't find a pointer to an official statement. I did find[1], which states: The printk subsystem will, for every printk, check if the last printk has a newline termination and if it doesn't and the current printk does not start with KERN_CONT will insert a newline. The negative to this approach is the last printk, if it does not have a newline, is buffered and not emitted until another printk occurs. There is also the (now small) possibility that multiple concurrent kernel threads or processes could interleave printks without a terminating newline and a different process could emit a printk that starts with KERN_CONT and the emitted message could be garbled. [1] https://lore.kernel.org/all/b867ee8a02043ec6b18c9330bfe3a091d66c816c.camel@perches.com Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Nov 17, 2022 at 10:38:16AM +0100, Geert Uytterhoeven wrote: > Hi Leon, > > On Thu, Nov 17, 2022 at 10:14 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Nov 17, 2022 at 09:59:55AM +0100, Geert Uytterhoeven wrote: > > > On Thu, Nov 17, 2022 at 9:58 AM Yoshihiro Shimoda > > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > > From: Leon Romanovsky, Sent: Thursday, November 17, 2022 3:09 PM > > > > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > > > > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > > > > > > } > > > > > > > > > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > > > > > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > > > > > > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", > > > > > > > > > > You can safely drop '\n' from here. It is not needed while printing one > > > > > line. > > > > > > > > Oh, I didn't know that. I'll remove '\n' from here on v2 patch. > > > > > > Please don't remove it. The convention is to have the newlines. > > > > Can you please explain why? > > I'm quite sure this was discussed in the context of commits > 5fd29d6ccbc98884 ("printk: clean up handling of log-levels and > newlines") and 4bcc595ccd80decb ("printk: reinstate KERN_CONT for > printing continuation lines"), but I couldn't find a pointer to an > official statement. Not a printk expert, but in first commit, Linus removed need of "\n" together with KERN_CONT, and in second commit he returned KERN_CONT, but didn't return need of "\n". > > I did find[1], which states: > > The printk subsystem will, for every printk, check > if the last printk has a newline termination and if > it doesn't and the current printk does not start with > KERN_CONT will insert a newline. > > The negative to this approach is the last printk, > if it does not have a newline, is buffered and not > emitted until another printk occurs. I have no idea if it is continue to be true in 2022. > > There is also the (now small) possibility that > multiple concurrent kernel threads or processes > could interleave printks without a terminating > newline and a different process could emit a > printk that starts with KERN_CONT and the emitted > message could be garbled. > > [1] https://lore.kernel.org/all/b867ee8a02043ec6b18c9330bfe3a091d66c816c.camel@perches.com > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Thursday, November 17, 2022 6:38 PM > > Hi Leon, > > On Thu, Nov 17, 2022 at 10:14 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Nov 17, 2022 at 09:59:55AM +0100, Geert Uytterhoeven wrote: > > > On Thu, Nov 17, 2022 at 9:58 AM Yoshihiro Shimoda > > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > > From: Leon Romanovsky, Sent: Thursday, November 17, 2022 3:09 PM > > > > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > > > > @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) > > > > > > } > > > > > > > > > > > > for (i = 0; i < RSWITCH_NUM_PORTS; i++) > > > > > > - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", > > > > > > + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", > > > > > > > > > > You can safely drop '\n' from here. It is not needed while printing one > > > > > line. > > > > > > > > Oh, I didn't know that. I'll remove '\n' from here on v2 patch. > > > > > > Please don't remove it. The convention is to have the newlines. > > > > Can you please explain why? > > I'm quite sure this was discussed in the context of commits > 5fd29d6ccbc98884 ("printk: clean up handling of log-levels and > newlines") and 4bcc595ccd80decb ("printk: reinstate KERN_CONT for > printing continuation lines"), but I couldn't find a pointer to an > official statement. > > I did find[1], which states: > > The printk subsystem will, for every printk, check > if the last printk has a newline termination and if > it doesn't and the current printk does not start with > KERN_CONT will insert a newline. > > The negative to this approach is the last printk, > if it does not have a newline, is buffered and not > emitted until another printk occurs. Thank you for your reply and sharing information. I'll keep '\n' on v2 patch. Best regards, Yoshihiro Shimoda > There is also the (now small) possibility that > multiple concurrent kernel threads or processes > could interleave printks without a terminating > newline and a different process could emit a > printk that starts with KERN_CONT and the emitted > message could be garbled. > > [1] > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c index f3d27aef1286..51ce5c26631b 100644 --- a/drivers/net/ethernet/renesas/rswitch.c +++ b/drivers/net/ethernet/renesas/rswitch.c @@ -1714,7 +1714,7 @@ static int rswitch_init(struct rswitch_private *priv) } for (i = 0; i < RSWITCH_NUM_PORTS; i++) - netdev_info(priv->rdev[i]->ndev, "MAC address %pMn", + netdev_info(priv->rdev[i]->ndev, "MAC address %pM\n", priv->rdev[i]->ndev->dev_addr); return 0;
Smatch detected the following warning. drivers/net/ethernet/renesas/rswitch.c:1717 rswitch_init() warn: '%pM' cannot be followed by 'n' The 'n' should be '\n'. Reported-by: Dan Carpenter <error27@gmail.com> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/rswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)