1. Patchew
  2. linux
  3. View series

[PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks

Ryosuke Saito posted 1 patch 2 years, 2 months ago 
drivers/net/ethernet/socionext/netsec.c | 35 ++++++++++++++++---------
1 file changed, 23 insertions(+), 12 deletions(-)
[PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
Posted by Ryosuke Saito 2 years, 2 months ago 
The cpu_relax() loops have the potential to hang if the specified
register bits are not met on condition. The patch replaces it with
usleep_range() and netsec_wait_while_busy() which includes timeout
logic.

Additionally, if the error condition is met during interrupting DMA
transfer, there's no recovery mechanism available. In that case, any
frames being sent or received will be discarded, which leads to
potential frame loss as indicated in the comments.

Signed-off-by: Ryosuke Saito <ryosuke.saito@linaro.org>
---
 drivers/net/ethernet/socionext/netsec.c | 35 ++++++++++++++++---------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 0dcd6a568b06..6f9127d30a9a 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1410,21 +1410,28 @@ static int netsec_reset_hardware(struct netsec_priv *priv,
 		netsec_write(priv, NETSEC_REG_DMA_MH_CTRL,
 			     NETSEC_DMA_CTRL_REG_STOP);
 
-		while (netsec_read(priv, NETSEC_REG_DMA_HM_CTRL) &
-		       NETSEC_DMA_CTRL_REG_STOP)
-			cpu_relax();
-
-		while (netsec_read(priv, NETSEC_REG_DMA_MH_CTRL) &
-		       NETSEC_DMA_CTRL_REG_STOP)
-			cpu_relax();
+		if (netsec_wait_while_busy(priv, NETSEC_REG_DMA_HM_CTRL,
+					   NETSEC_DMA_CTRL_REG_STOP) ||
+		    netsec_wait_while_busy(priv, NETSEC_REG_DMA_MH_CTRL,
+					   NETSEC_DMA_CTRL_REG_STOP)) {
+			dev_warn(priv->dev,
+				 "%s: DMA transfer cannot be stopped.\n",
+				 __func__);
+			/* There is no recovery mechanism in place if this
+			 * error occurs. Frames may be lost.
+			 */
+		}
 	}
 
 	netsec_write(priv, NETSEC_REG_SOFT_RST, NETSEC_SOFT_RST_REG_RESET);
 	netsec_write(priv, NETSEC_REG_SOFT_RST, NETSEC_SOFT_RST_REG_RUN);
 	netsec_write(priv, NETSEC_REG_COM_INIT, NETSEC_COM_INIT_REG_ALL);
 
-	while (netsec_read(priv, NETSEC_REG_COM_INIT) != 0)
-		cpu_relax();
+	if (netsec_wait_while_busy(priv, NETSEC_REG_COM_INIT, 1)) {
+		dev_err(priv->dev,
+			"%s: failed to reset NETSEC.\n", __func__);
+		return -ETIMEDOUT;
+	}
 
 	/* set desc_start addr */
 	netsec_write(priv, NETSEC_REG_NRM_RX_DESC_START_UP,
@@ -1476,9 +1483,13 @@ static int netsec_reset_hardware(struct netsec_priv *priv,
 	netsec_write(priv, NETSEC_REG_DMA_MH_CTRL, MH_CTRL__MODE_TRANS);
 	netsec_write(priv, NETSEC_REG_PKT_CTRL, value);
 
-	while ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
-		NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0)
-		cpu_relax();
+	usleep_range(100000, 120000);
+
+	if ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
+			 NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0) {
+		dev_warn(priv->dev,
+			 "%s: trans comp timeout.\n", __func__);
+	}
 
 	/* clear any pending EMPTY/ERR irq status */
 	netsec_write(priv, NETSEC_REG_NRM_TX_STATUS, ~0);
-- 
2.34.1
Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
Posted by Simon Horman 2 years, 2 months ago 
On Fri, Nov 17, 2023 at 05:10:02PM +0900, Ryosuke Saito wrote:
> The cpu_relax() loops have the potential to hang if the specified
> register bits are not met on condition. The patch replaces it with
> usleep_range() and netsec_wait_while_busy() which includes timeout
> logic.
> 
> Additionally, if the error condition is met during interrupting DMA
> transfer, there's no recovery mechanism available. In that case, any
> frames being sent or received will be discarded, which leads to
> potential frame loss as indicated in the comments.
> 
> Signed-off-by: Ryosuke Saito <ryosuke.saito@linaro.org>
> ---
>  drivers/net/ethernet/socionext/netsec.c | 35 ++++++++++++++++---------
>  1 file changed, 23 insertions(+), 12 deletions(-)

...

> @@ -1476,9 +1483,13 @@ static int netsec_reset_hardware(struct netsec_priv *priv,
>  	netsec_write(priv, NETSEC_REG_DMA_MH_CTRL, MH_CTRL__MODE_TRANS);
>  	netsec_write(priv, NETSEC_REG_PKT_CTRL, value);
>  
> -	while ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
> -		NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0)
> -		cpu_relax();
> +	usleep_range(100000, 120000);
> +
> +	if ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
> +			 NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0) {
> +		dev_warn(priv->dev,
> +			 "%s: trans comp timeout.\n", __func__);
> +	}

Hi Saito-san,

could you add some colour to how the new code satisfies the
requirements of the hardware?  In particular, the use of
usleep_range(), and the values passed to it.

>  
>  	/* clear any pending EMPTY/ERR irq status */
>  	netsec_write(priv, NETSEC_REG_NRM_TX_STATUS, ~0);
> -- 
> 2.34.1
>
Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
Posted by Ryosuke Saito 2 years, 2 months ago 
[Resend again after removing an HTML format. Sorry for that.]

Hi Simon-san,

On Mon, Nov 20, 2023 at 3:53 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Nov 17, 2023 at 05:10:02PM +0900, Ryosuke Saito wrote:
> > The cpu_relax() loops have the potential to hang if the specified
> > register bits are not met on condition. The patch replaces it with
> > usleep_range() and netsec_wait_while_busy() which includes timeout
> > logic.
> >
> > Additionally, if the error condition is met during interrupting DMA
> > transfer, there's no recovery mechanism available. In that case, any
> > frames being sent or received will be discarded, which leads to
> > potential frame loss as indicated in the comments.
> >
> > Signed-off-by: Ryosuke Saito <ryosuke.saito@linaro.org>
> > ---
> >  drivers/net/ethernet/socionext/netsec.c | 35 ++++++++++++++++---------
> >  1 file changed, 23 insertions(+), 12 deletions(-)
>
> ...
>
> > @@ -1476,9 +1483,13 @@ static int netsec_reset_hardware(struct netsec_priv 
*priv,
> >       netsec_write(priv, NETSEC_REG_DMA_MH_CTRL, MH_CTRL__MODE_TRANS);
> >       netsec_write(priv, NETSEC_REG_PKT_CTRL, value);
> >
> > -     while ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
> > -             NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0)
> > -             cpu_relax();
> > +     usleep_range(100000, 120000);
> > +
> > +     if ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
> > +                      NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0) {
> > +             dev_warn(priv->dev,
> > +                      "%s: trans comp timeout.\n", __func__);
> > +     }
>
> Hi Saito-san,
>
> could you add some colour to how the new code satisfies the
> requirements of the hardware?  In particular, the use of
> usleep_range(), and the values passed to it.


For the h/w requirements, I followed U-Boot upstream:
https://elixir.bootlin.com/u-boot/latest/source/drivers/net/sni_netsec.c

It has the same function as well, netsec_reset_hardware(), and the 
corresponding potion is the following read-check loop:

1012         value = 100;
1013         while ((netsec_read_reg(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) 
&
1014                 NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0) {
1015                 udelay(1000);
1016                 if (--value == 0) {
1017                         value = netsec_read_reg(priv, 
NETSEC_REG_MODE_TRANS_COMP_STATUS);
1018                         pr_err("%s:%d timeout! val=%x\n", __func__, 
__LINE__, value);
1019                         break;
1020                 }
1021         }

The maximum t/o = 1000us * 100 + read time

Regards,
Ryo
Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
Posted by Simon Horman 2 years, 2 months ago 
On Mon, Nov 20, 2023 at 11:19:48AM +0900, Ryosuke Saito wrote:
> [Resend again after removing an HTML format. Sorry for that.]
> 
> Hi Simon-san,
> 
> On Mon, Nov 20, 2023 at 3:53 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Fri, Nov 17, 2023 at 05:10:02PM +0900, Ryosuke Saito wrote:
> > > The cpu_relax() loops have the potential to hang if the specified
> > > register bits are not met on condition. The patch replaces it with
> > > usleep_range() and netsec_wait_while_busy() which includes timeout
> > > logic.
> > >
> > > Additionally, if the error condition is met during interrupting DMA
> > > transfer, there's no recovery mechanism available. In that case, any
> > > frames being sent or received will be discarded, which leads to
> > > potential frame loss as indicated in the comments.
> > >
> > > Signed-off-by: Ryosuke Saito <ryosuke.saito@linaro.org>
> > > ---
> > >  drivers/net/ethernet/socionext/netsec.c | 35 ++++++++++++++++---------
> > >  1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > ...
> >
> > > @@ -1476,9 +1483,13 @@ static int netsec_reset_hardware(struct netsec_priv 
> *priv,
> > >       netsec_write(priv, NETSEC_REG_DMA_MH_CTRL, MH_CTRL__MODE_TRANS);
> > >       netsec_write(priv, NETSEC_REG_PKT_CTRL, value);
> > >
> > > -     while ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
> > > -             NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0)
> > > -             cpu_relax();
> > > +     usleep_range(100000, 120000);
> > > +
> > > +     if ((netsec_read(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) &
> > > +                      NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0) {
> > > +             dev_warn(priv->dev,
> > > +                      "%s: trans comp timeout.\n", __func__);
> > > +     }
> >
> > Hi Saito-san,
> >
> > could you add some colour to how the new code satisfies the
> > requirements of the hardware?  In particular, the use of
> > usleep_range(), and the values passed to it.
> 
> 
> For the h/w requirements, I followed U-Boot upstream:
> https://elixir.bootlin.com/u-boot/latest/source/drivers/net/sni_netsec.c
> 
> It has the same function as well, netsec_reset_hardware(), and the 
> corresponding potion is the following read-check loop:
> 
> 1012         value = 100;
> 1013         while ((netsec_read_reg(priv, NETSEC_REG_MODE_TRANS_COMP_STATUS) 
> &
> 1014                 NETSEC_MODE_TRANS_COMP_IRQ_T2N) == 0) {
> 1015                 udelay(1000);
> 1016                 if (--value == 0) {
> 1017                         value = netsec_read_reg(priv, 
> NETSEC_REG_MODE_TRANS_COMP_STATUS);
> 1018                         pr_err("%s:%d timeout! val=%x\n", __func__, 
> __LINE__, value);
> 1019                         break;
> 1020                 }
> 1021         }
> 
> The maximum t/o = 1000us * 100 + read time

Hi Saito-san,

Thanks for the clarification.

I think that in lieu of more information about the hw, modeling the
code on a known working (or at least thought to be working) implementation
is good.

Reviewed-by: Simon Horman <horms@kernel.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.