Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Thu, 30 Nov 2023 16:09:37 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Paolo Abeni <pabeni@redhat.com>
Cc:     Jianheng Zhang <Jianheng.Zhang@synopsys.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <Jose.Abreu@synopsys.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Simon Horman <horms@kernel.org>,
        Andrew Halaney <ahalaney@redhat.com>,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
        Shenwei Wang <shenwei.wang@nxp.com>,
        Johannes Zink <j.zink@pengutronix.de>,
        "Russell King (Oracle" <rmk+kernel@armlinux.org.uk>,
        Jochen Henneberg <jh@henneberg-systemdesign.com>,
        Voon Weifeng <weifeng.voon@intel.com>,
        Mohammad Athari Bin Ismail <mohammad.athari.ismail@intel.com>,
        Ong Boon Leong <boon.leong.ong@intel.com>,
        Tan Tee Min <tee.min.tan@intel.com>,
        "open list:STMMAC ETHERNET DRIVER" <netdev@vger.kernel.org>,
        "moderated list:ARM/STM32 ARCHITECTURE" 
        <linux-stm32@st-md-mailman.stormreply.com>,
        "moderated list:ARM/STM32 ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        open list <linux-kernel@vger.kernel.org>,
        James Li <James.Li1@synopsys.com>,
        Martin McKenny <Martin.McKenny@synopsys.com>
Subject: Re: [PATCH v3] net: stmmac: fix FPE events losing
Message-ID: <5djt72m664jtskz4i7vu63cqpb67o4qeu2roqb6322slsypwos@vmf4n2emdazd>
References: <CY5PR12MB6372BF02C49FC9E628D0EC02BFBCA@CY5PR12MB6372.namprd12.prod.outlook.com>
 <1716792a3881338b1a416b1f4dd85a9437746ec2.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1716792a3881338b1a416b1f4dd85a9437746ec2.camel@redhat.com>
Precedence: bulk

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@gmail.com>
> > Signed-off-by: Jianheng Zhang <jianheng@synopsys.com>
> > ---
> >  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
> 
>