netdev - Re: [PATCH v3 net] net: stmmac: dwmac4: extend timeout for VLAN Tag register busy bit check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6nikxeiovzdrlxq5alpvhvgpkeky2gkevk6mczivfxsar3hoof@n2gxyrrten6m>
Date: Tue, 24 Sep 2024 21:50:48 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Shenwei Wang <shenwei.wang@....com>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	"horms@...nel.org" <horms@...nel.org>, Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, Ong Boon Leong <boon.leong.ong@...el.com>, 
	Wong Vee Khee <vee.khee.wong@...el.com>, Chuah Kim Tatt <kim.tatt.chuah@...el.com>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linux-stm32@...md-mailman.stormreply.com" <linux-stm32@...md-mailman.stormreply.com>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>, 
	dl-linux-imx <linux-imx@....com>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v3 net] net: stmmac: dwmac4: extend timeout for VLAN Tag
 register busy bit check

On Tue, Sep 24, 2024 at 06:13:27PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Serge Semin <fancer.lancer@...il.com>
> > Sent: Tuesday, September 24, 2024 2:30 AM
> > To: Shenwei Wang <shenwei.wang@....com>
> > Cc: David S. Miller <davem@...emloft.net>; Eric Dumazet
> > <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo Abeni
> > <pabeni@...hat.com>; Maxime Coquelin <mcoquelin.stm32@...il.com>;
> > horms@...nel.org; Alexandre Torgue <alexandre.torgue@...s.st.com>; Jose
> > Abreu <joabreu@...opsys.com>; Ong Boon Leong <boon.leong.ong@...el.com>;
> > Wong Vee Khee <vee.khee.wong@...el.com>; Chuah Kim Tatt
> > <kim.tatt.chuah@...el.com>; netdev@...r.kernel.org; linux-stm32@...md-
> > mailman.stormreply.com; linux-arm-kernel@...ts.infradead.org;
> > imx@...ts.linux.dev; dl-linux-imx <linux-imx@....com>; Andrew Lunn
> > <andrew@...n.ch>
> > Subject: [EXT] Re: [PATCH v3 net] net: stmmac: dwmac4: extend timeout for
> > VLAN Tag register busy bit check
> > >
> > > Overnight testing revealed that when EEE is active, the busy bit can
> > > remain set for up to approximately 300ms. The new 500ms timeout
> > > provides a safety margin.
> > >
> > > Fixes: ed64639bc1e0 ("net: stmmac: Add support for VLAN Rx filtering")
> > > Signed-off-by: Shenwei Wang <shenwei.wang@....com>
> > > Reviewed-by: Andrew Lunn <andrew@...n.ch>
> > 
> > > Reviewed-by: Serge Semin <fancer.lancer@...il.com>
> > 
> > Please note, you can't add the R-b tag without explicitly getting one from the
> > reviewer/maintainer/etc. Please read the chapter "When to use Acked-by:, Cc:,
> > and Co-developed-by:" in Documentation/process/submitting-patches.rst
> > 
> 

> I apologize, Serge. 
> I made an error in how I utilized the r-b function here. My intention was to explicitly 
> include you in the next version of the patch.

No problem. Just remember you can't add the formal
Reviewed-by/Acked-by/etc tags to the patch until you _explicitly_ get
one from the reviewers. It means you must wait until the reviewers
send you an email message with the tag typed in the text. Thus you
must drop my tag from your v4 patch.

Here is an excerpt from the kernel doc regarding this:

"Be careful in the addition of tags to your patches, as only Cc: is appropriate
for addition without the explicit permission of the person named; using
Reported-by: is fine most of the time as well, but ask for permission if
the bug was reported in private."

(see Documentation/process/5.Posting.rst for details)

-Serge(y)

> 
> Thanks,
> Shenwei
> 
> > > ---
> > > Changes in V3:
> > >  - re-org the error-check flow per Serge's review.
> > >
> > > Changes in v2:
> > >  - replace the udelay with readl_poll_timeout per Simon's review.
> > >
> > > ---
> > >  .../net/ethernet/stmicro/stmmac/dwmac4_core.c  | 18
> > > +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > > b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > > index a1858f083eef..0d27dd71b43e 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/ethtool.h>
> > >  #include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > >  #include "stmmac.h"
> > >  #include "stmmac_pcs.h"
> > >  #include "dwmac4.h"
> > > @@ -471,7 +472,7 @@ static int dwmac4_write_vlan_filter(struct net_device
> > *dev,
> > >                                   u8 index, u32 data)  {
> > >       void __iomem *ioaddr = (void __iomem *)dev->base_addr;
> > > -     int i, timeout = 10;
> > > +     int ret;
> > >       u32 val;
> > >
> > >       if (index >= hw->num_vlan)
> > > @@ -487,16 +488,15 @@ static int dwmac4_write_vlan_filter(struct
> > > net_device *dev,
> > >
> > >       writel(val, ioaddr + GMAC_VLAN_TAG);
> > >
> > > -     for (i = 0; i < timeout; i++) {
> > > -             val = readl(ioaddr + GMAC_VLAN_TAG);
> > > -             if (!(val & GMAC_VLAN_TAG_CTRL_OB))
> > > -                     return 0;
> > > -             udelay(1);
> > 
> > > +     ret = readl_poll_timeout(ioaddr + GMAC_VLAN_TAG, val,
> > > +                              !(val & GMAC_VLAN_TAG_CTRL_OB),
> > > +                              1000, 500000); //Timeout 500ms
> > 
> > Please drop the comment at the end of the statement. First of all the
> > C++-style comments are discouraged to be used in the kernel code except
> > when in the block of the SPDX licence identifier, or when documenting structs in
> > headers. Secondly the tail-comments are discouraged either (see
> > Documentation/process/maintainer-tip.rst - yes, it's for tip-tree, but the rule see
> > informally applicable for the entire kernel). Thirdly the comment is pointless here
> > since the literal
> > 500000 means exactly that.
> > 
> > -Serge(y)
> > 
> > > +     if (ret) {
> > > +             netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");
> > > +             return -EBUSY;
> > >       }
> > >
> > > -     netdev_err(dev, "Timeout accessing MAC_VLAN_Tag_Filter\n");
> > > -
> > > -     return -EBUSY;
> > > +     return 0;
> > >  }
> > >
> > >  static int dwmac4_add_hw_vlan_rx_fltr(struct net_device *dev,
> > > --
> > > 2.34.1
> > >