| Message ID | 1615919650-4262-2-git-send-email-bupadhaya@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qede: fix ethernet self adapter and skb failure issue | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 3 blamed authors not CCed: sudarsana.kalluru@qlogic.com Yuval.Mintz@qlogic.com manish.chopra@qlogic.com; 4 maintainers not CCed: sudarsana.kalluru@qlogic.com GR-everest-linux-l2@marvell.com Yuval.Mintz@qlogic.com manish.chopra@qlogic.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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? > /* 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. > */
> -----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. > > */
On Wed, 17 Mar 2021 06:33:37 +0000 Bhaskar Upadhaya wrote: > > 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. I'm not talking about submitting more traffic. Consider the following order of events normal traffic test xmit() netif_tx_disable() IRQ NAPI netif_tx_wake_queue() <--- traffic running again ---> qede_netif_stop()
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); /* 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. */
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(-)