| Message ID | 20230407071849.309516-1-den-plotnikov@yandex-team.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7573099e10ca69c3be33995c1fcd0d241226816d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] qlcnic: check pci_reset_function result | expand |
On Fri, Apr 07, 2023 at 10:18:49AM +0300, Denis Plotnikov wrote: > Static code analyzer complains to unchecked return value. > The result of pci_reset_function() is unchecked. > Despite, the issue is on the FLR supported code path and in that > case reset can be done with pcie_flr(), the patch uses less invasive > approach by adding the result check of pci_reset_function(). > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 7e2cf4feba05 ("qlcnic: change driver hardware interface mechanism") > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru> Thanks Denis, With reference to, Link: https://lore.kernel.org/all/20230405193708.GA3632282@bhelgaas/ I think this is the best approach in lieu of feedback from those with knowledge of the hardware / testing on the hardware. Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c > index 87f76bac2e463..eb827b86ecae8 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c > @@ -628,7 +628,13 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev) > int i, err, ring; > > if (dev->flags & QLCNIC_NEED_FLR) { > - pci_reset_function(dev->pdev); > + err = pci_reset_function(dev->pdev); > + if (err) { > + dev_err(&dev->pdev->dev, > + "Adapter reset failed (%d). Please reboot\n", > + err); > + return err; > + } > dev->flags &= ~QLCNIC_NEED_FLR; > } > > -- > 2.25.1 >
On Fri, Apr 07, 2023 at 11:01:14AM +0200, Simon Horman wrote: > On Fri, Apr 07, 2023 at 10:18:49AM +0300, Denis Plotnikov wrote: > > Static code analyzer complains to unchecked return value. > > The result of pci_reset_function() is unchecked. > > Despite, the issue is on the FLR supported code path and in that > > case reset can be done with pcie_flr(), the patch uses less invasive > > approach by adding the result check of pci_reset_function(). Text could possibly be smoothed out a bit, e.g.: Static code analyzer complains that the result of pci_reset_function() is unchecked. Check it and return error if it fails because the driver relies on the device being reset. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Fixes: 7e2cf4feba05 ("qlcnic: change driver hardware interface mechanism") > > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru> > > Thanks Denis, > > With reference to, > > Link: https://lore.kernel.org/all/20230405193708.GA3632282@bhelgaas/ > > I think this is the best approach in lieu of feedback from those > with knowledge of the hardware / testing on the hardware. Agreed, looks good to me, too. It doesn't look like there's much activity in this driver (except wider-scale cleanups, etc), so I doubt anybody could be confident that relying pcie_flr() would be safe. Seem a *little* weird that this reset is done in .ndo_open() instead of once at probe-time, but that's a much bigger question and not worth worrying about. Reviewed-by: Bjorn Helgaas <bhelgaas@google.com> > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > --- > > drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c > > index 87f76bac2e463..eb827b86ecae8 100644 > > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c > > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c > > @@ -628,7 +628,13 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev) > > int i, err, ring; > > > > if (dev->flags & QLCNIC_NEED_FLR) { > > - pci_reset_function(dev->pdev); > > + err = pci_reset_function(dev->pdev); > > + if (err) { > > + dev_err(&dev->pdev->dev, > > + "Adapter reset failed (%d). Please reboot\n", > > + err); > > + return err; > > + } > > dev->flags &= ~QLCNIC_NEED_FLR; > > } > > > > -- > > 2.25.1 > >
On Fri, 2023-04-07 at 10:18 +0300, Denis Plotnikov wrote: > Static code analyzer complains to unchecked return value. > The result of pci_reset_function() is unchecked. > Despite, the issue is on the FLR supported code path and in that > case reset can be done with pcie_flr(), the patch uses less invasive > approach by adding the result check of pci_reset_function(). > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 7e2cf4feba05 ("qlcnic: change driver hardware interface mechanism") > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru> Any special reason to target the net-next tree? This looks like a -net candidate to me? Thanks! Paolo
On Tue, Apr 11, 2023 at 01:24:33PM +0200, Paolo Abeni wrote: > On Fri, 2023-04-07 at 10:18 +0300, Denis Plotnikov wrote: > > Static code analyzer complains to unchecked return value. > > The result of pci_reset_function() is unchecked. > > Despite, the issue is on the FLR supported code path and in that > > case reset can be done with pcie_flr(), the patch uses less invasive > > approach by adding the result check of pci_reset_function(). > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Fixes: 7e2cf4feba05 ("qlcnic: change driver hardware interface mechanism") > > Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru> > > Any special reason to target the net-next tree? This looks like a -net > candidate to me? FWIIW, net would be fine by me. Sorry for not noticing this during earlier review.
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Fri, 7 Apr 2023 10:18:49 +0300 you wrote: > Static code analyzer complains to unchecked return value. > The result of pci_reset_function() is unchecked. > Despite, the issue is on the FLR supported code path and in that > case reset can be done with pcie_flr(), the patch uses less invasive > approach by adding the result check of pci_reset_function(). > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > [...] Here is the summary with links: - [net-next,v2] qlcnic: check pci_reset_function result https://git.kernel.org/netdev/net/c/7573099e10ca You are awesome, thank you!
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c index 87f76bac2e463..eb827b86ecae8 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c @@ -628,7 +628,13 @@ int qlcnic_fw_create_ctx(struct qlcnic_adapter *dev) int i, err, ring; if (dev->flags & QLCNIC_NEED_FLR) { - pci_reset_function(dev->pdev); + err = pci_reset_function(dev->pdev); + if (err) { + dev_err(&dev->pdev->dev, + "Adapter reset failed (%d). Please reboot\n", + err); + return err; + } dev->flags &= ~QLCNIC_NEED_FLR; }
Static code analyzer complains to unchecked return value. The result of pci_reset_function() is unchecked. Despite, the issue is on the FLR supported code path and in that case reset can be done with pcie_flr(), the patch uses less invasive approach by adding the result check of pci_reset_function(). Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 7e2cf4feba05 ("qlcnic: change driver hardware interface mechanism") Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru> --- drivers/net/ethernet/qlogic/qlcnic/qlcnic_ctx.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)