| Message ID | 20211129132829.16038-2-skalluru@marvell.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | aa1dcb5646fdf34a15763facf4bf5e482a2814ca |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: atlantic: 11-2021 fixes | expand |
On Mon, Nov 29, 2021 at 05:28:23AM -0800, Sudarsana Reddy Kalluru wrote: > From: Dmitry Bogdanov <dbezrukov@marvell.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@marvell.com> > Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com> > Signed-off-by: Igor Russkikh <irusskikh@marvell.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. > > 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. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, November 30, 2021 8:18 AM > To: Sudarsana Reddy Kalluru <skalluru@marvell.com> > Cc: davem@davemloft.net; netdev@vger.kernel.org; Igor Russkikh > <irusskikh@marvell.com>; Dmitrii Bezrukov <dbezrukov@marvell.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@marvell.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@marvell.com> > > Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com> > > Signed-off-by: Igor Russkikh <irusskikh@marvell.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
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); 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; #define AQ_SDELTA(_N_, _F_) (self->curr_stats._N_ += \ stats.msm._F_ - priv->last_stats.msm._F_)