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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 21 Nov 2023 11:02:42 +0000
From: Simon Horman <horms@...nel.org>
To: Ryosuke Saito <ryosuke.saito@...aro.org>
Cc: jaswinder.singh@...aro.org, ilias.apalodimas@...aro.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, masahisa.kojima@...aro.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
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@...nel.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@...aro.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@...nel.org>