RE: [EXT] Re: [PATCH net 1/2] qede: fix to disable start_xmit functionality during self adapter test

[Bhaskar Upadhaya <bupadhaya@marvell.com>]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, March 17, 2021 3:30 AM
> To: Bhaskar Upadhaya <bupadhaya@marvell.com>
> Cc: netdev@vger.kernel.org; Ariel Elior <aelior@marvell.com>; Igor Russkikh
> <irusskikh@marvell.com>; davem@davemloft.net
> Subject: [EXT] Re: [PATCH net 1/2] qede: fix to disable start_xmit functionality
> during self adapter test
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Tue, 16 Mar 2021 11:34:09 -0700 Bhaskar Upadhaya wrote:
> > start_xmit function should not be called during the execution of self
> > adapter test, netif_tx_disable() gives this guarantee, since it takes
> > the transmit queue lock while marking the queue stopped.  This will
> > wait for the transmit function to complete before returning.
> >
> > Fixes: 16f46bf054f8 ("qede: add implementation for internal loopback
> > test.")
> > Signed-off-by: Bhaskar Upadhaya <bupadhaya@marvell.com>
> > Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > ---
> >  drivers/net/ethernet/qlogic/qede/qede_ethtool.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> > b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> > index 1560ad3d9290..f9702cc7bc55 100644
> > --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> > +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> > @@ -1611,7 +1611,7 @@ static int qede_selftest_run_loopback(struct
> qede_dev *edev, u32 loopback_mode)
> >  		return -EINVAL;
> >  	}
> >
> > -	qede_netif_stop(edev);
> > +	netif_tx_disable(edev->ndev);
> 
> But an interrupt can come in after and enable Tx again.
> I think you should keep the qede_netif_stop() here instead of moving it
> down, no?

Hi Jakub,
Normal Traffic flow is enabled by qede_netif_start(edev) and which is placed at the end of this qede_selftest_run_loopback()
qede_netif_stop(edev) is called prior to the call to qede_netif_start(edev), so unless qede_netif_start(edev)  is called Normal traffic flow will not
be operational. 
> 
> >  	/* Bring up the link in Loopback mode */
> >  	memset(&link_params, 0, sizeof(link_params)); @@ -1623,6 +1623,8
> @@
> > static int qede_selftest_run_loopback(struct qede_dev *edev, u32
> loopback_mode)
> >  	/* Wait for loopback configuration to apply */
> >  	msleep_interruptible(500);
> >
> > +	qede_netif_stop(edev);
> > +
> >  	/* Setting max packet size to 1.5K to avoid data being split over
> >  	 * multiple BDs in cases where MTU > PAGE_SIZE.
> >  	 */