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

From: Jianheng Zhang
Date: Tue Nov 21 2023 - 04:32:22 EST

Next message: Zhu Jun: "[PATCH v1] selftests/media_tests: fix a resource leak"
Previous message: Justin Lai: "RE: [PATCH net-next v11 09/13] net:ethernet:realtek:rtase: Implement pci_driver suspend and resume function"
Next in thread: Serge Semin: "Re: [PATCH] net: stmmac: fix FPE events losing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> From: Jianheng Zhang
> Sent: Thursday, November 16, 2023 12:01 AM
> To: Russell King <linux@xxxxxxxxxxxxxxx>; Serge Semin <fancer.lancer@xxxxxxxxx>
> Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>; Jose Abreu <joabreu@xxxxxxxxxxxx>; David S.
> Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski
> <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Maxime Coquelin
> <mcoquelin.stm32@xxxxxxxxx>; Tan Tee Min <tee.min.tan@xxxxxxxxx>; Ong Boon Leong
> <boon.leong.ong@xxxxxxxxx>; Voon Weifeng <weifeng.voon@xxxxxxxxx>; Mohammad Athari Bin Ismail
> <mohammad.athari.ismail@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH] net: stmmac: fix FPE events losing
>
>
> > -----Original Message-----
> > From: Russell King <linux@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, November 15, 2023 10:26 PM
> > To: Serge Semin <fancer.lancer@xxxxxxxxx>
> > Cc: Jianheng Zhang <jianheng@xxxxxxxxxxxx>; Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>;
> Jose
> > Abreu <joabreu@xxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> > <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>;
> > Maxime Coquelin <mcoquelin.stm32@xxxxxxxxx>; Tan Tee Min <tee.min.tan@xxxxxxxxx>; Ong Boon
> > Leong <boon.leong.ong@xxxxxxxxx>; Voon Weifeng <weifeng.voon@xxxxxxxxx>; Mohammad Athari
> Bin
> > Ismail <mohammad.athari.ismail@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> > linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] net: stmmac: fix FPE events losing
> >
> > 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.
> >
> > I've never heard of that problem with MMIO though.
> >
> > --
> > RMK's Patch system:
> >
> https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!A4F2R9G_pg!ZnsQc
> >
> 7LKbdCiIj_nLBkt44MRSL8W8YD4WvMmPNkHFlDMrjrVqEVOVA1-P-9BVE3Oa5bC1ymKXLNV5C9mJ_rfFtLW
> > Fg$
> > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> Thanks for all your mention about the potential risks of using readb and suggestions, I'll consider a new
> solution and update the patch.
>
> Best Regards,
> Jianheng

Thanks for Andrew's suggestion. I consider the solution of set
MAC_CSR_SW_Ctrl.RCWE carefully and found that the MAC_CSR_SW_Ctrl.RCWE
is efficient for many other interrupt status registers, and some are used
in current stmmac driver (LPI PMT MMC etc). Those registers work fine in
clear on read mode with current code. So, I think there is no need to
set MAC_CSR_SW_Ctrl.RCWE that will make a large impact to current
interrupt handler.

I try adding a new member variable(staged_fpe_state) to stmmac_fpe_cfg
that is used to stage the MAC_FPE_CTRL_STS value read in
dwmac5_fpe_send_mpacket(). And in dwmac5_fpe_irq_status(), use this staged
value and current value of MAC_FPE_CTRL_STS to check FPE status. The
draft patch is below.

Best Regards,
Jianheng

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

---
drivers/net/ethernet/stmicro/stmmac/dwmac5.c | 9 +++++++--
drivers/net/ethernet/stmicro/stmmac/dwmac5.h | 1 +
drivers/net/ethernet/stmicro/stmmac/hwif.h | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++-
include/linux/stmmac.h | 1 +
5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
index e95d35f..8917fd0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.c
@@ -736,12 +736,15 @@ void dwmac5_fpe_configure(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,

int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
{
+ struct stmmac_priv *priv = netdev_priv(dev);
+ struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
u32 value;
int status;

status = FPE_EVENT_UNKNOWN;

- value = readl(ioaddr + MAC_FPE_CTRL_STS);
+ value = readl(ioaddr + MAC_FPE_CTRL_STS) | fpe_cfg->staged_fpe_state;
+ fpe_cfg->staged_fpe_state = 0x0;

if (value & TRSP) {
status |= FPE_EVENT_TRSP;
@@ -766,11 +769,13 @@ 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;

value = readl(ioaddr + MAC_FPE_CTRL_STS);
+ cfg->staged_fpe_state = value;

if (type == MPACKET_VERIFY) {
value &= ~SRSP;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
index 53c138d..022c1a2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
@@ -156,6 +156,7 @@ 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,
bool enable);
void dwmac5_fpe_send_mpacket(void __iomem *ioaddr,
+ struct stmmac_fpe_cfg *cfg,
enum stmmac_mpacket_type type);
int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev);

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index b95d3e1..5496781 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -415,6 +415,7 @@ struct stmmac_ops {
void (*fpe_configure)(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,
bool enable);
void (*fpe_send_mpacket)(void __iomem *ioaddr,
+ struct stmmac_fpe_cfg *cfg,
enum stmmac_mpacket_type type);
int (*fpe_irq_status)(void __iomem *ioaddr, struct net_device *dev);
};
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e50fd5..6a29f55 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -964,7 +964,8 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
bool *hs_enable = &fpe_cfg->hs_enable;

if (is_up && *hs_enable) {
- stmmac_fpe_send_mpacket(priv, priv->ioaddr, MPACKET_VERIFY);
+ stmmac_fpe_send_mpacket(priv, priv->ioaddr, fpe_cfg,
+ MPACKET_VERIFY);
} else {
*lo_state = FPE_STATE_OFF;
*lp_state = FPE_STATE_OFF;
@@ -5838,6 +5839,7 @@ static void stmmac_fpe_event_status(struct stmmac_priv *priv, int status)
/* If user has requested FPE enable, quickly response */
if (*hs_enable)
stmmac_fpe_send_mpacket(priv, priv->ioaddr,
+ fpe_cfg,
MPACKET_RESPONSE);
}

@@ -7280,6 +7282,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
netdev_info(priv->dev, SEND_VERIFY_MPAKCET_FMT,
*lo_state, *lp_state);
stmmac_fpe_send_mpacket(priv, priv->ioaddr,
+ fpe_cfg,
MPACKET_VERIFY);
}
/* Sleep then retry */
@@ -7294,6 +7297,7 @@ void stmmac_fpe_handshake(struct stmmac_priv *priv, bool enable)
if (priv->plat->fpe_cfg->hs_enable != enable) {
if (enable) {
stmmac_fpe_send_mpacket(priv, priv->ioaddr,
+ priv->plat->fpe_cfg,
MPACKET_VERIFY);
} else {
priv->plat->fpe_cfg->lo_fpe_state = FPE_STATE_OFF;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 0b4658a..1322c78 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -175,6 +175,7 @@ struct stmmac_fpe_cfg {
bool hs_enable; /* FPE handshake enable */
enum stmmac_fpe_state lp_fpe_state; /* Link Partner FPE state */
enum stmmac_fpe_state lo_fpe_state; /* Local station FPE state */
+ u32 staged_fpe_state; /* Staged FPE state when read MAC_FPE_CTRL_STS */
};

struct stmmac_safety_feature_cfg {
--
1.8.3.1

Next message: Zhu Jun: "[PATCH v1] selftests/media_tests: fix a resource leak"
Previous message: Justin Lai: "RE: [PATCH net-next v11 09/13] net:ethernet:realtek:rtase: Implement pci_driver suspend and resume function"
Next in thread: Serge Semin: "Re: [PATCH] net: stmmac: fix FPE events losing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]