Messages in this thread | | | Date | Tue, 21 Nov 2023 11:02:42 +0000 | From | Simon Horman <> | Subject | Re: [PATCH] net: netsec: replace cpu_relax() with timeout handling for register checks |
| |
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>
| |