netdev - RE: [PATCH net 1/7] atlantic: Increase delay for fw transactions

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, 30 Nov 2021 04:18:23 +0000
From:   Sudarsana Reddy Kalluru <skalluru@...vell.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Igor Russkikh <irusskikh@...vell.com>,
        Dmitrii Bezrukov <dbezrukov@...vell.com>
Subject: RE: [PATCH net 1/7] atlantic: Increase delay for fw transactions



> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Tuesday, November 30, 2021 8:18 AM
> To: Sudarsana Reddy Kalluru <skalluru@...vell.com>
> Cc: davem@...emloft.net; netdev@...r.kernel.org; Igor Russkikh
> <irusskikh@...vell.com>; Dmitrii Bezrukov <dbezrukov@...vell.com>
> Subject: Re: [PATCH net 1/7] atlantic: Increase delay for fw transactions
> 
> On Mon, Nov 29, 2021 at 05:28:23AM -0800, Sudarsana Reddy Kalluru wrote:
> > From: Dmitry Bogdanov <dbezrukov@...vell.com>
> >
> > The max waiting period (of 1 ms) while reading the data from FW shared
> > buffer is too small for certain types of data (e.g., stats). There's a
> > chance that FW could be updating buffer at the same time and driver
> > would be unsuccessful in reading data. Firmware manual recommends to
> > have 1 sec timeout to fix this issue.
> >
> > Fixes: 5cfd54d7dc186 ("net: atlantic: minimal A2 fw_ops")
> > Signed-off-by: Dmitry Bogdanov <dbezrukov@...vell.com>
> > Signed-off-by: Sudarsana Reddy Kalluru <skalluru@...vell.com>
> > Signed-off-by: Igor Russkikh <irusskikh@...vell.com>
> > ---
> >  .../ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c  | 7
> > +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> > b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> > index dd259c8f2f4f..b0e4119b9883 100644
> > ---
> > a/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.c
> > +++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl2/hw_atl2_utils_fw.
> > +++ c
> > @@ -84,7 +84,7 @@ static int hw_atl2_shared_buffer_read_block(struct
> aq_hw_s *self,
> >  			if (cnt > AQ_A2_FW_READ_TRY_MAX)
> >  				return -ETIME;
> >  			if (tid1.transaction_cnt_a != tid1.transaction_cnt_b)
> > -				udelay(1);
> > +				mdelay(1);
> >  		} while (tid1.transaction_cnt_a != tid1.transaction_cnt_b);
> 
> This change is the 1 second timeout.
Yes,  FW manual suggests 1 sec timeout value. Hence the timeout period is updated from 1ms to 1sec.

> 
> >
> >  		hw_atl2_mif_shared_buf_read(self, offset, (u32 *)data,
> dwords); @@
> > -339,8 +339,11 @@ static int aq_a2_fw_update_stats(struct aq_hw_s
> > *self)  {
> >  	struct hw_atl2_priv *priv = (struct hw_atl2_priv *)self->priv;
> >  	struct statistics_s stats;
> > +	int err;
> >
> > -	hw_atl2_shared_buffer_read_safe(self, stats, &stats);
> > +	err = hw_atl2_shared_buffer_read_safe(self, stats, &stats);
> > +	if (err)
> > +		return err;
> 
> This change however does not seem to be explained in the commit message.
> Not discarding an error is a good change, but it needs commenting on.
> 
> Also, looking at hw_atl2_shared_buffer_read_block() i notice it returns -
> ETIME. It should be -ETIMEDOUT.
Thanks for your inputs. Will discuss about this internally and send the patch to handle all such error paths.

> 
> 	Andrew