Re: [PATCH] net: stmmac: fix FPE events losing

[Serge Semin]

On Wed, Nov 15, 2023 at 02:25:37PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 14, 2023 at 02:59:57PM +0300, Serge Semin wrote:
> > On Tue, Nov 14, 2023 at 11:07:34AM +0000, Jianheng Zhang wrote:
> > > The 32-bit access of register MAC_FPE_CTRL_STS may clear the FPE status
> > > bits unexpectedly. Use 8-bit access for MAC_FPE_CTRL_STS control bits to
> > > avoid unexpected access of MAC_FPE_CTRL_STS status bits that can reduce
> > > the FPE handshake retries.
> > > 
> > > The bit[19:17] of register MAC_FPE_CTRL_STS are status register bits.
> > > Those bits are clear on read (or write of 1 when RCWE bit in
> > > MAC_CSR_SW_Ctrl register is set). Using 32-bit access for
> > > MAC_FPE_CTRL_STS control bits makes side effects that clear the status
> > > bits. Then the stmmac interrupt handler missing FPE event status and
> > > leads to FPE handshake failure and retries.
> > > 
> > > The bit[7:0] of register MAC_FPE_CTRL_STS are control bits or reserved
> > > that have no access side effects, so can use 8-bit access for
> > > MAC_FPE_CTRL_STS control bits.
> > > 
> > > Fixes: 5a5586112b92 ("net: stmmac: support FPE link partner hand-shaking procedure")
> > > Signed-off-by: jianheng <jianheng@xxxxxxxxxxxx>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwmac5.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> > > index e95d35f..7333995 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> > > @@ -716,11 +716,11 @@ void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,
> > >  	u32 value;
> > >  
> > >  	if (!enable) {
> > 
> > > -		value = readl(ioaddr + MAC_FPE_CTRL_STS);
> > > +		value = readb(ioaddr + MAC_FPE_CTRL_STS);
> > 
> > Note this may break the platforms which don't support non-32 MMIOs for
> > some devices. None of the currently supported glue-drivers explicitly
> > state they have such peculiarity, but at the same time the STMMAC-core
> > driver at the present state uses the dword IO ops only. For instance
> > the PCIe subsystem has the special accessors for such cases:
> > pci_generic_config_read32()
> > pci_generic_config_write32()
> > which at the very least are utilized on the Tegra and Loongson
> > platforms to access the host CSR spaces. These platforms are also
> > equipped with the DW MACs. The problem might be irrelevant for all the
> > currently supported DW MAC controllers implementations though, but
> > still it worth to draw an attention to the problem possibility and in
> > order to prevent it before ahead it would be better to just avoid
> > using the byte-/word- IOs if it's possible.
> 

> Yes, this exists for configuration accesses, and is damn annoying
> because the read-modify-write of these can end up clearing PCI
> status register bits that are W1C.
> 

Right, but at least on Tegra these accessors are utilized to reach the
Root Port config space only, which is basically the normal memory
mapped CSRs. No PCIe bus transfers is performed for it, just normal
AHB/AXI/etc bus IOs passed directly to the DW PCIe CDM (Configuration
Dependent Module, that is generic PCIe config-space and
Synopsys-specific CSRs).

> I've never heard of that problem with MMIO though.

I was working with such platform and it was indeed tricky to have linux
working there well for all such peculiar SoC devices. Some drivers had the
"reg-io-width"/"reg-shift" DT-properties supported, which helped very
much in that matter since it tweaked the respective drivers to using
the allowed on my platform instructions only.

-Serge(y)

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