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

[Serge Semin]

Hi Paolo

On Thu, Nov 30, 2023 at 10:55:34AM +0100, Paolo Abeni wrote:
> On Tue, 2023-11-28 at 05:56 +0000, Jianheng Zhang wrote:
> > The status bits of register MAC_FPE_CTRL_STS are clear on read. Using
> > 32-bit read for MAC_FPE_CTRL_STS in dwmac5_fpe_configure() and
> > dwmac5_fpe_send_mpacket() clear the status bits. Then the stmmac interrupt
> > handler missing FPE event status and leads to FPE handshaking failure and
> > retries.
> > To avoid clear status bits of MAC_FPE_CTRL_STS in dwmac5_fpe_configure()
> > and dwmac5_fpe_send_mpacket(), add fpe_csr to stmmac_fpe_cfg structure to
> > cache the control bits of MAC_FPE_CTRL_STS and to avoid reading
> > MAC_FPE_CTRL_STS in those methods.
> > 
> > Fixes: 5a5586112b92 ("net: stmmac: support FPE link partner hand-shaking procedure")
> > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > Signed-off-by: Jianheng Zhang <jianheng@xxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac5.c       | 45 +++++++++-------------
> >  drivers/net/ethernet/stmicro/stmmac/dwmac5.h       |  4 +-
> >  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    |  3 +-
> >  drivers/net/ethernet/stmicro/stmmac/hwif.h         |  4 +-
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  8 +++-
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c    |  1 +
> >  include/linux/stmmac.h                             |  1 +
> >  7 files changed, 36 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> > index e95d35f..8fd1675 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
> > @@ -710,28 +710,22 @@ void dwmac5_est_irq_status(void __iomem *ioaddr, struct net_device *dev,
> >  	}
> >  }
> >  
> > -void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,
> > +void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> > +			  u32 num_txq, u32 num_rxq,
> >  			  bool enable)
> >  {
> >  	u32 value;
> >  
> > -	if (!enable) {
> > -		value = readl(ioaddr + MAC_FPE_CTRL_STS);
> > -
> > -		value &= ~EFPE;
> > -
> > -		writel(value, ioaddr + MAC_FPE_CTRL_STS);
> > -		return;
> > +	if (enable) {
> > +		cfg->fpe_csr = EFPE;
> > +		value = readl(ioaddr + GMAC_RXQ_CTRL1);
> > +		value &= ~GMAC_RXQCTRL_FPRQ;
> > +		value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
> > +		writel(value, ioaddr + GMAC_RXQ_CTRL1);
> > +	} else {
> > +		cfg->fpe_csr = 0;
> >  	}
> > -
> > -	value = readl(ioaddr + GMAC_RXQ_CTRL1);
> > -	value &= ~GMAC_RXQCTRL_FPRQ;
> > -	value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
> > -	writel(value, ioaddr + GMAC_RXQ_CTRL1);
> > -
> > -	value = readl(ioaddr + MAC_FPE_CTRL_STS);
> > -	value |= EFPE;
> > -	writel(value, ioaddr + MAC_FPE_CTRL_STS);
> > +	writel(cfg->fpe_csr, ioaddr + MAC_FPE_CTRL_STS);
> >  }
> >  
> >  int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> > @@ -741,6 +735,9 @@ int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> >  
> >  	status = FPE_EVENT_UNKNOWN;
> >  
> > +	/* Reads from the MAC_FPE_CTRL_STS register should only be performed
> > +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> > +	 */
> >  	value = readl(ioaddr + MAC_FPE_CTRL_STS);
> >  
> >  	if (value & TRSP) {
> > @@ -766,19 +763,15 @@ int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> >  	return status;
> >  }
> >  
> > -void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, enum stmmac_mpacket_type type)
> > +void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> > +			     enum stmmac_mpacket_type type)
> >  {
> > -	u32 value;
> > +	u32 value = cfg->fpe_csr;
> >  
> > -	value = readl(ioaddr + MAC_FPE_CTRL_STS);
> > -
> > -	if (type == MPACKET_VERIFY) {
> > -		value &= ~SRSP;
> > +	if (type == MPACKET_VERIFY)
> >  		value |= SVER;
> > -	} else {
> > -		value &= ~SVER;
> > +	else if (type == MPACKET_RESPONSE)
> >  		value |= SRSP;
> > -	}
> >  
> >  	writel(value, ioaddr + MAC_FPE_CTRL_STS);
> >  }
> 

> It's unclear to me why it's not necessary to preserve the SVER/SRSP
> bits across MAC_FPE_CTRL_STS writes. I guess they are not part of the
> status bits? perhaps an explicit comment somewhere will help?

The SRSP and SVER are self-cleared flags with no effect on zero
writing. Their responsibility is to emit the Respond and Verify
mPackets respectively. As soon as the packets are sent, the flags will
be reset by hardware automatically. So no, they aren't a part of the
status bits.

Note since 'value' now isn't read from the MAC_FPE_CTRL_STS register,
there is no point in clearing up these flags in the local variable
because 'value' has now them cleared by default.

Not sure whether a comment about that is required, since the described
behavior is well documented in the Synopsys HW-manual.

-Serge(y)

> 
> Thanks
> 
> Paolo
> 
>