Re: [PATCH net v4 3/3] net: stmmac: Specify hardware capability value when FIFO size isn't specified

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Feb 03, 2025 at 11:45:05AM +0900, Kunihiko Hayashi wrote:
> Hi all,
> 
> On 2025/02/02 5:35, Russell King (Oracle) wrote:
> > On Sat, Feb 01, 2025 at 11:14:41AM -0800, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > On Mon, Jan 27, 2025 at 10:38:20AM +0900, Kunihiko Hayashi wrote:
> > > > When Tx/Rx FIFO size is not specified in advance, the driver checks if
> > > > the value is zero and sets the hardware capability value in functions
> > > > where that value is used.
> > > > 
> > > > Consolidate the check and settings into function stmmac_hw_init() and
> > > > remove redundant other statements.
> > > > 
> > > > If FIFO size is zero and the hardware capability also doesn't have
> > upper
> > > > limit values, return with an error message.
> > > > 
> > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > > 
> > > This patch breaks qemu's stmmac emulation, for example for
> > > npcm750-evb. The error message is:
> > > 	stmmaceth f0804000.eth: Can't specify Rx FIFO size
> 
> Sorry for inconvenience.
> 
> > Interesting. I looked at QEMU to see whether anything in the Debian
> > stable version of QEMU might possibly have STMMAC emulation, but
> > drew a blank... Even trying to find where in QEMU it emulates the
> > STMMAC. I do see that it does include this, so maybe I can use that
> > to test some of my stmmac changes. Thanks!
> > 
> > > The setup function called for the emulated hardware is
> > dwmac1000_setup().
> > > That function does not set the DMA rx or tx fifo size.
> > > 
> > > At the same time, the rx and tx fifo size is not provided in the
> > > devicetree file (nuvoton-npcm750.dtsi), so the failure is obvious.
> > > 
> > > I understand that the real hardware may be based on a more recent
> > > version of the DWMAC IP which provides the DMA tx/rx fifo size, but
> > > I do wonder: Are the benefits of this patch so substantial that it
> > > warrants breaking the qemu emulation of this network interface >
> > Please see my message sent a while back on an earlier revision of this
> > patch series. I reviewed the stmmac driver for the fifo sizes and
> > documented what I found.
> > 
> > https://lore.kernel.org/r/Z4_ZilVFKacuAUE8@shell.armlinux.org.uk
> > 
> > To save clicking on the link, I'll reproduce the relevant part below.
> > It appears that dwmac1000 has no way to specify the FIFO size, and
> > thus would have priv->dma_cap.rx_fifo_size and
> > priv->dma_cap.tx_fifo_size set to zero.
> > 
> > Given the responses, I'm now of the opinion that the patch series is
> > wrong, and probably should be reverted - I never really understood
> > the motivation why the series was necessary. It seemed to me to be a
> > "wouldn't it be nice if" series rather than something that is
> > functionally necessary.
> > 
> > 
> > Here's the extract from my previous email:
> > 
> > Now looking at the defintions:
> > 
> > drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_RXFIFOSIZE
> > GENMASK(4, 0)
> > drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define
> > XGMAC_HWFEAT_RXFIFOSIZE GENMASK(4, 0)
> > 
> > So there's a 5-bit bitfield that describes the receive FIFO size for
> > these two MACs. Then we have:
> > 
> > drivers/net/ethernet/stmicro/stmmac/common.h:#define
> > DMA_HW_FEAT_RXFIFOSIZE    0x00080000       /* Rx FIFO > 2048 Bytes */
> > 
> > which is used here:
> > 
> > drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c:
> > dma_cap->rxfifo_over_2048 = (hw_cap & DMA_HW_FEAT_RXFIFOSIZE) >> 19;
> > 
> > which is only used to print a Y/N value in a debugfs file, otherwise
> > having no bearing on driver behaviour.
> > 
> > So, I suspect MACs other than xgmac2 or dwmac4 do not have the ability
> > to describe the hardware FIFO sizes in hardware, thus why there's the
> > override and no checking of what the platform provided - and doing so
> > would break the driver. This is my interpretation from the code alone.
> > 
> 
> The {tx,rx}_queus_to_use are referenced in stmmac_ethtool.c, stmmac_tc.c,
> and stmmac_selftests.c as the number of queues, so I've thought that
> these variables should not be non-zero.

Huh? We're talking about {tx,rx}_fifo_size, not _queues_to_use.

> However, currently the variables are allowed to be zero, so I understand
> this patch 3/3 breaks on the chips that hasn't hardware capabilities.
> 
> In hwif.c, stmmac_hw[] defines four patterns of hardwares:
> 
> "dwmac100"  .gmac=false, .gmac4=false, .xgmac=false, .get_hw_feature = NULL
> "dwmac1000" .gmac=true,  .gmac4=false, .xgmac=false, .get_hw_feature = dwmac1000_get_hw_feature()
> "dwmac4"    .gmac=false, .gmac4=true,  .xgmac=false, .get_hw_feature = dwmac4_get_hw_feature()
> "dwxgmac2"  .gmac=false, .gmac4=false, .xgmac=true , .get_hw_feature = dwxgmac2_get_hw_feature()
> 
> As Russell said, the dwmac100 can't get the number of queues from the hardware
> capability. And some environments (at least QEMU device that Guenter said)
> seems the capability values are zero in spite of dwmac1000.

Huh? I mentioned dwmac1000, not dwmac100.

> Since I can't test all of the device patterns, so I appreciate checking each
> hardware and finding the issue.
> 
> The patch 3/3 includes some cleanup and code reduction, though, I think
> it would be better to revert it once.

I'm not sure you're discussing the same issue as the rest of us.
You seem to be talking about a different pair of structure members
(queues_to_use) whereas your patches and the problem at hand is with
the changes made to {tx,rx}_fifo_size.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!