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

[Serge Semin]

On Tue, Sep 24, 2024 at 06:13:27PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Serge Semin <fancer.lancer at gmail.com>
> > Sent: Tuesday, September 24, 2024 2:30 AM
> > To: Shenwei Wang <shenwei.wang at nxp.com>
> > Cc: David S. Miller <davem at davemloft.net>; Eric Dumazet
> > <edumazet at google.com>; Jakub Kicinski <kuba at kernel.org>; Paolo Abeni
> > <pabeni at redhat.com>; Maxime Coquelin <mcoquelin.stm32 at gmail.com>;
> > horms at kernel.org; Alexandre Torgue <alexandre.torgue at foss.st.com>; Jose
> > Abreu <joabreu at synopsys.com>; Ong Boon Leong <boon.leong.ong at intel.com>;
> > Wong Vee Khee <vee.khee.wong at intel.com>; Chuah Kim Tatt
> > <kim.tatt.chuah at intel.com>; netdev at vger.kernel.org; linux-stm32 at st-md-
> > mailman.stormreply.com; linux-arm-kernel at lists.infradead.org;
> > imx at lists.linux.dev; dl-linux-imx <linux-imx at nxp.com>; Andrew Lunn
> > <andrew at lunn.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 at nxp.com>
> > > Reviewed-by: Andrew Lunn <andrew at lunn.ch>
> > 
> > > Reviewed-by: Serge Semin <fancer.lancer at gmail.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
> > >