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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM5PR18MB21529A16BF342B1DBC344403AB6A9@DM5PR18MB2152.namprd18.prod.outlook.com>
Date:   Wed, 17 Mar 2021 06:33:37 +0000
From:   Bhaskar Upadhaya <bupadhaya@...vell.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <aelior@...vell.com>,
        Igor Russkikh <irusskikh@...vell.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [EXT] Re: [PATCH net 1/2] qede: fix to disable start_xmit
 functionality during self adapter test



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, March 17, 2021 3:30 AM
> To: Bhaskar Upadhaya <bupadhaya@...vell.com>
> Cc: netdev@...r.kernel.org; Ariel Elior <aelior@...vell.com>; Igor Russkikh
> <irusskikh@...vell.com>; davem@...emloft.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@...vell.com>
> > Signed-off-by: Igor Russkikh <irusskikh@...vell.com>
> > Signed-off-by: Ariel Elior <aelior@...vell.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.
> >  	 */