2023-11-28 05:58:03

by Jianheng Zhang

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

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 <[email protected]>
Signed-off-by: Jianheng Zhang <[email protected]>
---
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);
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
index 53c138d..34e6207 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
@@ -153,9 +153,11 @@ int dwmac5_est_configure(void __iomem *ioaddr, struct stmmac_est *cfg,
unsigned int ptp_rate);
void dwmac5_est_irq_status(void __iomem *ioaddr, struct net_device *dev,
struct stmmac_extra_stats *x, u32 txqcnt);
-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);
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/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index 453e88b..a74e71d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -1484,7 +1484,8 @@ static int dwxgmac3_est_configure(void __iomem *ioaddr, struct stmmac_est *cfg,
return 0;
}

-static void dwxgmac3_fpe_configure(void __iomem *ioaddr, u32 num_txq,
+static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+ u32 num_txq,
u32 num_rxq, bool enable)
{
u32 value;
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index b95d3e1..68aa2d5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -412,9 +412,11 @@ struct stmmac_ops {
unsigned int ptp_rate);
void (*est_irq_status)(void __iomem *ioaddr, struct net_device *dev,
struct stmmac_extra_stats *x, u32 txqcnt);
- void (*fpe_configure)(void __iomem *ioaddr, u32 num_txq, u32 num_rxq,
+ void (*fpe_configure)(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+ 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..7791e9b 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);
}

@@ -7262,6 +7264,7 @@ static void stmmac_fpe_lp_task(struct work_struct *work)
if (*lo_state == FPE_STATE_ENTERING_ON &&
*lp_state == FPE_STATE_ENTERING_ON) {
stmmac_fpe_configure(priv, priv->ioaddr,
+ fpe_cfg,
priv->plat->tx_queues_to_use,
priv->plat->rx_queues_to_use,
*enable);
@@ -7280,6 +7283,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 +7298,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;
@@ -7754,6 +7759,7 @@ int stmmac_suspend(struct device *dev)
if (priv->dma_cap.fpesel) {
/* Disable FPE */
stmmac_fpe_configure(priv, priv->ioaddr,
+ priv->plat->fpe_cfg,
priv->plat->tx_queues_to_use,
priv->plat->rx_queues_to_use, false);

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index ac41ef4..6ad3e0a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1079,6 +1079,7 @@ static int tc_setup_taprio(struct stmmac_priv *priv,

priv->plat->fpe_cfg->enable = false;
stmmac_fpe_configure(priv, priv->ioaddr,
+ priv->plat->fpe_cfg,
priv->plat->tx_queues_to_use,
priv->plat->rx_queues_to_use,
false);
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 0b4658a..dee5ad6 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 fpe_csr; /* MAC_FPE_CTRL_STS reg cache */
};

struct stmmac_safety_feature_cfg {
--
1.8.3.1

2023-11-30 09:56:08

by Paolo Abeni

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

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 <[email protected]>
> Signed-off-by: Jianheng Zhang <[email protected]>
> ---
> 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?

Thanks

Paolo

2023-11-30 13:10:01

by Serge Semin

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

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 <[email protected]>
> > Signed-off-by: Jianheng Zhang <[email protected]>
> > ---
> > 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
>
>

2023-11-30 14:10:08

by Paolo Abeni

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

On Thu, 2023-11-30 at 16:09 +0300, Serge Semin wrote:
> 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 <[email protected]>
> > > Signed-off-by: Jianheng Zhang <[email protected]>
> > > ---
> > > 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.

Thanks for the explanation, it clarifies the things to me. I agree
there is no need for a patch change.

Cheers,

Paolo

2023-11-30 14:13:59

by Paolo Abeni

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

On Thu, 2023-11-30 at 15:09 +0100, Paolo Abeni wrote:
> On Thu, 2023-11-30 at 16:09 +0300, Serge Semin wrote:
> > 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 <[email protected]>
> > > > Signed-off-by: Jianheng Zhang <[email protected]>
> > > > ---
> > > > 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.
>
> Thanks for the explanation, it clarifies the things to me. I agree
> there is no need for a patch change.

I'm sorry, I have to take back the last sentence: the submitter and SoB
email address still don't match. @Jianheng: please fix it for good at
the next iteration.

Cheers,

Paolo

2023-12-01 02:51:09

by Jianheng Zhang

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

> -----Original Message-----
> From: Paolo Abeni <[email protected]>
> Sent: Thursday, November 30, 2023 10:14 PM
> To: Serge Semin <[email protected]>
> Cc: Jianheng Zhang <[email protected]>; Alexandre Torgue <[email protected]>; Jose
> Abreu <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Maxime Coquelin
> <[email protected]>; Simon Horman <[email protected]>; Andrew Halaney
> <[email protected]>; Bartosz Golaszewski <[email protected]>; Shenwei Wang
> <[email protected]>; Johannes Zink <[email protected]>; Russell King (Oracle
> <[email protected]>; Jochen Henneberg <[email protected]>; Voon Weifeng
> <[email protected]>; Mohammad Athari Bin Ismail <[email protected]>; Ong
> Boon Leong <[email protected]>; Tan Tee Min <[email protected]>; open list:STMMAC
> ETHERNET DRIVER <[email protected]>; moderated list:ARM/STM32 ARCHITECTURE
> <[email protected]>; moderated list:ARM/STM32 ARCHITECTURE
> <[email protected]>; open list <[email protected]>; James Li
> <[email protected]>; Martin McKenny <[email protected]>
> Subject: Re: [PATCH v3] net: stmmac: fix FPE events losing
>
> On Thu, 2023-11-30 at 15:09 +0100, Paolo Abeni wrote:
> > On Thu, 2023-11-30 at 16:09 +0300, Serge Semin wrote:
> > > 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 <[email protected]>
> > > > > Signed-off-by: Jianheng Zhang <[email protected]>
> > > > > ---
> > > > > 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.

Thanks for Serge’s explanation.
> >
> > Thanks for the explanation, it clarifies the things to me. I agree
> > there is no need for a patch change.
>
> I'm sorry, I have to take back the last sentence: the submitter and SoB
> email address still don't match. @Jianheng: please fix it for good at
> the next iteration.

OH, sorry~ I mixed up my internet email address with the internal one. I will fix it and resend the patch.

Best Regards,
Jianheng
>
> Cheers,
>
> Paolo