[v1] net: stmmac: fix FPE events losing

[PATCH] net: stmmac: fix FPE events losing

Posted by Jianheng Zhang 2 years, 1 month ago

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@synopsys.com>
---
 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);
 
 		value &= ~EFPE;
 
-		writel(value, ioaddr + MAC_FPE_CTRL_STS);
+		writeb(value, ioaddr + MAC_FPE_CTRL_STS);
 		return;
 	}
 
@@ -729,9 +729,9 @@ void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,
 	value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
 	writel(value, ioaddr + GMAC_RXQ_CTRL1);
 
-	value = readl(ioaddr + MAC_FPE_CTRL_STS);
+	value = readb(ioaddr + MAC_FPE_CTRL_STS);
 	value |= EFPE;
-	writel(value, ioaddr + MAC_FPE_CTRL_STS);
+	writeb(value, ioaddr + MAC_FPE_CTRL_STS);
 }
 
 int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
@@ -770,7 +770,7 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, enum stmmac_mpacket_type type
 {
 	u32 value;
 
-	value = readl(ioaddr + MAC_FPE_CTRL_STS);
+	value = readb(ioaddr + MAC_FPE_CTRL_STS);
 
 	if (type == MPACKET_VERIFY) {
 		value &= ~SRSP;
@@ -780,5 +780,5 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, enum stmmac_mpacket_type type
 		value |= SRSP;
 	}
 
-	writel(value, ioaddr + MAC_FPE_CTRL_STS);
+	writeb(value, ioaddr + MAC_FPE_CTRL_STS);
 }
-- 
1.8.3.1

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

Posted by Andrew Lunn 2 years, 1 month ago

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.

Is it possible to call the core of stmmac_fpe_irq_status() to extract
the information from these bits and then call
stmmac_fpe_event_status()?

Alternatively, can you actually set RCWE in MAC_CSR_SW_Ctrl and add a
mask to dwmac5_fpe_configure() etc so they don't write 1 to these
bits? That seems safer than assuming 8 bit reads work.

      Andrew

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

Posted by Serge Semin 2 years, 1 month ago

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@synopsys.com>
> ---
>  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.

-Serge(y)

>  
>  		value &= ~EFPE;
>  
> -		writel(value, ioaddr + MAC_FPE_CTRL_STS);
> +		writeb(value, ioaddr + MAC_FPE_CTRL_STS);
>  		return;
>  	}
>  
> @@ -729,9 +729,9 @@ void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,
>  	value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
>  	writel(value, ioaddr + GMAC_RXQ_CTRL1);
>  
> -	value = readl(ioaddr + MAC_FPE_CTRL_STS);
> +	value = readb(ioaddr + MAC_FPE_CTRL_STS);
>  	value |= EFPE;
> -	writel(value, ioaddr + MAC_FPE_CTRL_STS);
> +	writeb(value, ioaddr + MAC_FPE_CTRL_STS);
>  }
>  
>  int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> @@ -770,7 +770,7 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, enum stmmac_mpacket_type type
>  {
>  	u32 value;
>  
> -	value = readl(ioaddr + MAC_FPE_CTRL_STS);
> +	value = readb(ioaddr + MAC_FPE_CTRL_STS);
>  
>  	if (type == MPACKET_VERIFY) {
>  		value &= ~SRSP;
> @@ -780,5 +780,5 @@ void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, enum stmmac_mpacket_type type
>  		value |= SRSP;
>  	}
>  
> -	writel(value, ioaddr + MAC_FPE_CTRL_STS);
> +	writeb(value, ioaddr + MAC_FPE_CTRL_STS);
>  }
> -- 
> 1.8.3.1
> 
>