| Message ID | 20240923202602.506066-1-shenwei.wang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,net] net: stmmac: dwmac4: extend timeout for VLAN Tag register busy bit check | expand |
Hi Shenwei On Mon, Sep 23, 2024 at 03:26:02PM -0500, Shenwei Wang wrote: > Increase the timeout for checking the busy bit of the VLAN Tag register > from 10µs to 500ms. This change is necessary to accommodate scenarios > where Energy Efficient Ethernet (EEE) is enabled. > > 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@nxp.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Serge Semin <fancer.lancer@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 > --- > 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 >
On Mon, Sep 23, 2024 at 03:26:02PM -0500, Shenwei Wang wrote: > Increase the timeout for checking the busy bit of the VLAN Tag register > from 10µs to 500ms. This change is necessary to accommodate scenarios > where Energy Efficient Ethernet (EEE) is enabled. > > 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@nxp.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> Since you are respinning Your Signed-off-by: should come last. Andrew
> -----Original Message----- > From: Serge Semin <fancer.lancer@gmail.com> > Sent: Tuesday, September 24, 2024 2:30 AM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; > horms@kernel.org; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose > Abreu <joabreu@synopsys.com>; Ong Boon Leong <boon.leong.ong@intel.com>; > Wong Vee Khee <vee.khee.wong@intel.com>; Chuah Kim Tatt > <kim.tatt.chuah@intel.com>; netdev@vger.kernel.org; linux-stm32@st-md- > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; > imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>; Andrew Lunn > <andrew@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@nxp.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > Reviewed-by: Serge Semin <fancer.lancer@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. 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 > >
On Tue, Sep 24, 2024 at 06:13:27PM +0000, Shenwei Wang wrote: > > > > -----Original Message----- > > From: Serge Semin <fancer.lancer@gmail.com> > > Sent: Tuesday, September 24, 2024 2:30 AM > > To: Shenwei Wang <shenwei.wang@nxp.com> > > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet > > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > > <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>; > > horms@kernel.org; Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose > > Abreu <joabreu@synopsys.com>; Ong Boon Leong <boon.leong.ong@intel.com>; > > Wong Vee Khee <vee.khee.wong@intel.com>; Chuah Kim Tatt > > <kim.tatt.chuah@intel.com>; netdev@vger.kernel.org; linux-stm32@st-md- > > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org; > > imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>; Andrew Lunn > > <andrew@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@nxp.com> > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > > Reviewed-by: Serge Semin <fancer.lancer@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 > > >
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 + 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,
Increase the timeout for checking the busy bit of the VLAN Tag register from 10µs to 500ms. This change is necessary to accommodate scenarios where Energy Efficient Ethernet (EEE) is enabled. 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@nxp.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Reviewed-by: Serge Semin <fancer.lancer@gmail.com> --- 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(-) -- 2.34.1