* [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
@ 2023-11-17 8:10 Ryosuke Saito
2023-11-19 18:53 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Ryosuke Saito @ 2023-11-17 8:10 UTC (permalink / raw)
To: jaswinder.singh, ilias.apalodimas, davem, edumazet, kuba, pabeni,
masahisa.kojima
Cc: netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
2023-11-17 8:10 [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks Ryosuke Saito
@ 2023-11-19 18:53 ` Simon Horman
2023-11-20 2:19 ` Ryosuke Saito
0 siblings, 1 reply; 4+ messages in thread
From: Simon Horman @ 2023-11-19 18:53 UTC (permalink / raw)
To: Ryosuke Saito
Cc: jaswinder.singh, ilias.apalodimas, davem, edumazet, kuba, pabeni,
masahisa.kojima, netdev, linux-kernel
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
2023-11-19 18:53 ` Simon Horman
@ 2023-11-20 2:19 ` Ryosuke Saito
2023-11-21 11:02 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Ryosuke Saito @ 2023-11-20 2:19 UTC (permalink / raw)
To: Simon Horman
Cc: jaswinder.singh, ilias.apalodimas, davem, edumazet, kuba, pabeni,
masahisa.kojima, netdev, linux-kernel
[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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks
2023-11-20 2:19 ` Ryosuke Saito
@ 2023-11-21 11:02 ` Simon Horman
0 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2023-11-21 11:02 UTC (permalink / raw)
To: Ryosuke Saito
Cc: jaswinder.singh, ilias.apalodimas, davem, edumazet, kuba, pabeni,
masahisa.kojima, netdev, linux-kernel
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-21 11:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-17 8:10 [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks Ryosuke Saito
2023-11-19 18:53 ` Simon Horman
2023-11-20 2:19 ` Ryosuke Saito
2023-11-21 11:02 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).