| Message ID | E1tfeyR-003YGJ-Gb@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2a64c96356c87aa8af826605943e5524bf45e24d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] Revert "net: stmmac: Specify hardware capability value when FIFO size isn't specified" | expand |
On 2/5/25 1:57 PM, Russell King (Oracle) wrote: > This reverts commit 8865d22656b4, which caused breakage for platforms > which are not using xgmac2 or gmac4. Only these two cores have the > capability of providing the FIFO sizes from hardware capability fields > (which are provided in priv->dma_cap.[tr]x_fifo_size.) > > All other cores can not, which results in these two fields containing > zero. We also have platforms that do not provide a value in > priv->plat->[tr]x_fifo_size, resulting in these also being zero. > > This causes the new tests introduced by the reverted commit to fail, > and produce e.g.: > > stmmaceth f0804000.eth: Can't specify Rx FIFO size > > An example of such a platform which fails is QEMU's npcm750-evb. > This uses dwmac1000 which, as noted above, does not have the capability > to provide the FIFO sizes from hardware. > > Therefore, revert the commit to maintain compatibility with the way > the driver used to work. > > Reported-by: Guenter Roeck <linux@roeck-us.net> > Link: https://lore.kernel.org/r/4e98f967-f636-46fb-9eca-d383b9495b86@roeck-us.net > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Given the fallout caused by the blamed commit, the imminent net PR, and the substantial agreement about the patch already shared by many persons on the ML, unless someone raises very serious concerns very soon, I'm going to apply this patch (a little) earlier than the 24h grace period, to fit the mentioned PR. Cheers, Paolo
On 05/02/2025 12:57, Russell King (Oracle) wrote: > This reverts commit 8865d22656b4, which caused breakage for platforms > which are not using xgmac2 or gmac4. Only these two cores have the > capability of providing the FIFO sizes from hardware capability fields > (which are provided in priv->dma_cap.[tr]x_fifo_size.) > > All other cores can not, which results in these two fields containing > zero. We also have platforms that do not provide a value in > priv->plat->[tr]x_fifo_size, resulting in these also being zero. > > This causes the new tests introduced by the reverted commit to fail, > and produce e.g.: > > stmmaceth f0804000.eth: Can't specify Rx FIFO size > > An example of such a platform which fails is QEMU's npcm750-evb. > This uses dwmac1000 which, as noted above, does not have the capability > to provide the FIFO sizes from hardware. > > Therefore, revert the commit to maintain compatibility with the way > the driver used to work. > > Reported-by: Guenter Roeck <linux@roeck-us.net> > Link: https://lore.kernel.org/r/4e98f967-f636-46fb-9eca-d383b9495b86@roeck-us.net > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Tested on my Firefly RK3288 Tested-by: Steven Price <steven.price@arm.com> > --- > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 +++++++++---------- > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index d04543e5697b..b34ebb916b89 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -2424,6 +2424,11 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv) > u32 chan = 0; > u8 qmode = 0; > > + if (rxfifosz == 0) > + rxfifosz = priv->dma_cap.rx_fifo_size; > + if (txfifosz == 0) > + txfifosz = priv->dma_cap.tx_fifo_size; > + > /* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */ > if (priv->plat->has_gmac4 || priv->plat->has_xgmac) { > rxfifosz /= rx_channels_count; > @@ -2892,6 +2897,11 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode, > int rxfifosz = priv->plat->rx_fifo_size; > int txfifosz = priv->plat->tx_fifo_size; > > + if (rxfifosz == 0) > + rxfifosz = priv->dma_cap.rx_fifo_size; > + if (txfifosz == 0) > + txfifosz = priv->dma_cap.tx_fifo_size; > + > /* Adjust for real per queue fifo size */ > rxfifosz /= rx_channels_count; > txfifosz /= tx_channels_count; > @@ -5868,6 +5878,9 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu) > const int mtu = new_mtu; > int ret; > > + if (txfifosz == 0) > + txfifosz = priv->dma_cap.tx_fifo_size; > + > txfifosz /= priv->plat->tx_queues_to_use; > > if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) { > @@ -7219,29 +7232,15 @@ static int stmmac_hw_init(struct stmmac_priv *priv) > priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues; > } > > - if (!priv->plat->rx_fifo_size) { > - if (priv->dma_cap.rx_fifo_size) { > - priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; > - } else { > - dev_err(priv->device, "Can't specify Rx FIFO size\n"); > - return -ENODEV; > - } > - } else if (priv->dma_cap.rx_fifo_size && > - priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { > + if (priv->dma_cap.rx_fifo_size && > + priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { > dev_warn(priv->device, > "Rx FIFO size (%u) exceeds dma capability\n", > priv->plat->rx_fifo_size); > priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; > } > - if (!priv->plat->tx_fifo_size) { > - if (priv->dma_cap.tx_fifo_size) { > - priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size; > - } else { > - dev_err(priv->device, "Can't specify Tx FIFO size\n"); > - return -ENODEV; > - } > - } else if (priv->dma_cap.tx_fifo_size && > - priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { > + if (priv->dma_cap.tx_fifo_size && > + priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { > dev_warn(priv->device, > "Tx FIFO size (%u) exceeds dma capability\n", > priv->plat->tx_fifo_size);
On Thu, Feb 06, 2025 at 09:08:10AM +0100, Paolo Abeni wrote: > On 2/5/25 1:57 PM, Russell King (Oracle) wrote: > > This reverts commit 8865d22656b4, which caused breakage for platforms > > which are not using xgmac2 or gmac4. Only these two cores have the > > capability of providing the FIFO sizes from hardware capability fields > > (which are provided in priv->dma_cap.[tr]x_fifo_size.) > > > > All other cores can not, which results in these two fields containing > > zero. We also have platforms that do not provide a value in > > priv->plat->[tr]x_fifo_size, resulting in these also being zero. > > > > This causes the new tests introduced by the reverted commit to fail, > > and produce e.g.: > > > > stmmaceth f0804000.eth: Can't specify Rx FIFO size > > > > An example of such a platform which fails is QEMU's npcm750-evb. > > This uses dwmac1000 which, as noted above, does not have the capability > > to provide the FIFO sizes from hardware. > > > > Therefore, revert the commit to maintain compatibility with the way > > the driver used to work. > > > > Reported-by: Guenter Roeck <linux@roeck-us.net> > > Link: https://lore.kernel.org/r/4e98f967-f636-46fb-9eca-d383b9495b86@roeck-us.net > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Given the fallout caused by the blamed commit, the imminent net PR, and > the substantial agreement about the patch already shared by many persons > on the ML, unless someone raises very serious concerns very soon, I'm > going to apply this patch (a little) earlier than the 24h grace period, > to fit the mentioned PR. Thanks. Here's the missing Fixes tag that I missed: Fixes: 8865d22656b4 ("net: stmmac: Specify hardware capability value when FIFO size isn't specified") Not sure if patchwork will pick that up.
On 2/6/25 11:47 AM, Russell King (Oracle) wrote: > On Thu, Feb 06, 2025 at 09:08:10AM +0100, Paolo Abeni wrote: >> On 2/5/25 1:57 PM, Russell King (Oracle) wrote: >>> This reverts commit 8865d22656b4, which caused breakage for platforms >>> which are not using xgmac2 or gmac4. Only these two cores have the >>> capability of providing the FIFO sizes from hardware capability fields >>> (which are provided in priv->dma_cap.[tr]x_fifo_size.) >>> >>> All other cores can not, which results in these two fields containing >>> zero. We also have platforms that do not provide a value in >>> priv->plat->[tr]x_fifo_size, resulting in these also being zero. >>> >>> This causes the new tests introduced by the reverted commit to fail, >>> and produce e.g.: >>> >>> stmmaceth f0804000.eth: Can't specify Rx FIFO size >>> >>> An example of such a platform which fails is QEMU's npcm750-evb. >>> This uses dwmac1000 which, as noted above, does not have the capability >>> to provide the FIFO sizes from hardware. >>> >>> Therefore, revert the commit to maintain compatibility with the way >>> the driver used to work. >>> >>> Reported-by: Guenter Roeck <linux@roeck-us.net> >>> Link: https://lore.kernel.org/r/4e98f967-f636-46fb-9eca-d383b9495b86@roeck-us.net >>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> >> >> Given the fallout caused by the blamed commit, the imminent net PR, and >> the substantial agreement about the patch already shared by many persons >> on the ML, unless someone raises very serious concerns very soon, I'm >> going to apply this patch (a little) earlier than the 24h grace period, >> to fit the mentioned PR. > > Thanks. Here's the missing Fixes tag that I missed: > > Fixes: 8865d22656b4 ("net: stmmac: Specify hardware capability value when > FIFO size isn't specified") > > Not sure if patchwork will pick that up. Thank you indeed for the missing piece. I'll add that when applying the patch (~now). /P
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Wed, 05 Feb 2025 12:57:47 +0000 you wrote: > This reverts commit 8865d22656b4, which caused breakage for platforms > which are not using xgmac2 or gmac4. Only these two cores have the > capability of providing the FIFO sizes from hardware capability fields > (which are provided in priv->dma_cap.[tr]x_fifo_size.) > > All other cores can not, which results in these two fields containing > zero. We also have platforms that do not provide a value in > priv->plat->[tr]x_fifo_size, resulting in these also being zero. > > [...] Here is the summary with links: - [net] Revert "net: stmmac: Specify hardware capability value when FIFO size isn't specified" https://git.kernel.org/netdev/net/c/2a64c96356c8 You are awesome, thank you!
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index d04543e5697b..b34ebb916b89 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2424,6 +2424,11 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv) u32 chan = 0; u8 qmode = 0; + if (rxfifosz == 0) + rxfifosz = priv->dma_cap.rx_fifo_size; + if (txfifosz == 0) + txfifosz = priv->dma_cap.tx_fifo_size; + /* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */ if (priv->plat->has_gmac4 || priv->plat->has_xgmac) { rxfifosz /= rx_channels_count; @@ -2892,6 +2897,11 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode, int rxfifosz = priv->plat->rx_fifo_size; int txfifosz = priv->plat->tx_fifo_size; + if (rxfifosz == 0) + rxfifosz = priv->dma_cap.rx_fifo_size; + if (txfifosz == 0) + txfifosz = priv->dma_cap.tx_fifo_size; + /* Adjust for real per queue fifo size */ rxfifosz /= rx_channels_count; txfifosz /= tx_channels_count; @@ -5868,6 +5878,9 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu) const int mtu = new_mtu; int ret; + if (txfifosz == 0) + txfifosz = priv->dma_cap.tx_fifo_size; + txfifosz /= priv->plat->tx_queues_to_use; if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) { @@ -7219,29 +7232,15 @@ static int stmmac_hw_init(struct stmmac_priv *priv) priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues; } - if (!priv->plat->rx_fifo_size) { - if (priv->dma_cap.rx_fifo_size) { - priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; - } else { - dev_err(priv->device, "Can't specify Rx FIFO size\n"); - return -ENODEV; - } - } else if (priv->dma_cap.rx_fifo_size && - priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { + if (priv->dma_cap.rx_fifo_size && + priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { dev_warn(priv->device, "Rx FIFO size (%u) exceeds dma capability\n", priv->plat->rx_fifo_size); priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; } - if (!priv->plat->tx_fifo_size) { - if (priv->dma_cap.tx_fifo_size) { - priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size; - } else { - dev_err(priv->device, "Can't specify Tx FIFO size\n"); - return -ENODEV; - } - } else if (priv->dma_cap.tx_fifo_size && - priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { + if (priv->dma_cap.tx_fifo_size && + priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { dev_warn(priv->device, "Tx FIFO size (%u) exceeds dma capability\n", priv->plat->tx_fifo_size);
This reverts commit 8865d22656b4, which caused breakage for platforms which are not using xgmac2 or gmac4. Only these two cores have the capability of providing the FIFO sizes from hardware capability fields (which are provided in priv->dma_cap.[tr]x_fifo_size.) All other cores can not, which results in these two fields containing zero. We also have platforms that do not provide a value in priv->plat->[tr]x_fifo_size, resulting in these also being zero. This causes the new tests introduced by the reverted commit to fail, and produce e.g.: stmmaceth f0804000.eth: Can't specify Rx FIFO size An example of such a platform which fails is QEMU's npcm750-evb. This uses dwmac1000 which, as noted above, does not have the capability to provide the FIFO sizes from hardware. Therefore, revert the commit to maintain compatibility with the way the driver used to work. Reported-by: Guenter Roeck <linux@roeck-us.net> Link: https://lore.kernel.org/r/4e98f967-f636-46fb-9eca-d383b9495b86@roeck-us.net Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-)