| Message ID | 20230324175136.321588-2-seanga2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d61157414d0a591d10d27d0ce5873916614e5e31 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sunhme: Probe/IRQ cleanups | expand |
Hi Sean, On Fri, Mar 24, 2023 at 01:51:27PM -0400, Sean Anderson wrote: > Fix an uninitialized return code if we never found a qfe slot. It would be > a bug if we ever got into this situation, but it's good to return something > tracable. > > Fixes: acb3f35f920b ("sunhme: forward the error code from pci_enable_device()") I think it might be, Fixes: 5b3dc6dda6b1 ("sunhme: Regularize probe errors") > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> Checkpatch requests a Link tag after Reported-by tags. Link: https://lore.kernel.org/oe-kbuild/20230222135715.hjXBN9H5dr7nCnI_Ye2s5H--HsnWom4o9AMScThhDRM@z/T/ > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v4: > - Move this fix to its own commit > > drivers/net/ethernet/sun/sunhme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index b0c7ab74a82e..7cf8210ebbec 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -2834,7 +2834,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > int i, qfe_slot = -1; > char prom_name[64]; > u8 addr[ETH_ALEN]; > - int err; > + int err = -ENODEV; > > /* Now make sure pci_dev cookie is there. */ > #ifdef CONFIG_SPARC Unfortunately I don't think this is the right fix, and indeed smatch still complains with it applied. The reason is that a few lines further down there is: err = pcim_enable_device(pdev); if (err) goto err_out; Which overrides the initialisation of err. Before getting to the line that smatch highlights, correctly as far as I can tell, as having a missing error code. if (qfe_slot == 4) goto err_out; As err_out simply calls 'return err' one could just return here. And perhaps that is a nice cleanup to roll out at some point. But to be in keeping with the style of the function, as a minimal fix, I think the following might be appropriate. Note, with this applied err doesn't need to be initialised at the top of the function (as far as my invocation of smatch is concerned). diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index b0c7ab74a82e..d6df778a0052 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -2863,8 +2863,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, if (!qp->happy_meals[qfe_slot]) break; - if (qfe_slot == 4) + if (qfe_slot == 4) { + err = -ENODEV; goto err_out; + } } dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal));
On Sat, Mar 25, 2023 at 09:17:03AM +0100, Simon Horman wrote: > Hi Sean, > > On Fri, Mar 24, 2023 at 01:51:27PM -0400, Sean Anderson wrote: > > Fix an uninitialized return code if we never found a qfe slot. It would be > > a bug if we ever got into this situation, but it's good to return something > > tracable. > > > > Fixes: acb3f35f920b ("sunhme: forward the error code from pci_enable_device()") > > I think it might be, > > Fixes: 5b3dc6dda6b1 ("sunhme: Regularize probe errors") > > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <error27@gmail.com> > > Checkpatch requests a Link tag after Reported-by tags. > > Link: https://lore.kernel.org/oe-kbuild/20230222135715.hjXBN9H5dr7nCnI_Ye2s5H--HsnWom4o9AMScThhDRM@z/T/ > > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > > --- > > > > Changes in v4: > > - Move this fix to its own commit > > > > drivers/net/ethernet/sun/sunhme.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > > index b0c7ab74a82e..7cf8210ebbec 100644 > > --- a/drivers/net/ethernet/sun/sunhme.c > > +++ b/drivers/net/ethernet/sun/sunhme.c > > @@ -2834,7 +2834,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > > int i, qfe_slot = -1; > > char prom_name[64]; > > u8 addr[ETH_ALEN]; > > - int err; > > + int err = -ENODEV; > > > > /* Now make sure pci_dev cookie is there. */ > > #ifdef CONFIG_SPARC > > Unfortunately I don't think this is the right fix, > and indeed smatch still complains with it applied. > > The reason is that a few lines further down there is: > > err = pcim_enable_device(pdev); > if (err) > goto err_out; > > Which overrides the initialisation of err. > Before getting to the line that smatch highlights, correctly as far > as I can tell, as having a missing error code. > > if (qfe_slot == 4) > goto err_out; > > As err_out simply calls 'return err' one could just return here. > And perhaps that is a nice cleanup to roll out at some point. I see that point is patch 9/10, which also fixes the bug reported by smatch :) > But to be in keeping with the style of the function, as a minimal fix, > I think the following might be appropriate. > > Note, with this applied err doesn't need to be initialised at the top of > the function (as far as my invocation of smatch is concerned). > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index b0c7ab74a82e..d6df778a0052 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -2863,8 +2863,10 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > if (!qp->happy_meals[qfe_slot]) > break; > > - if (qfe_slot == 4) > + if (qfe_slot == 4) { > + err = -ENODEV; > goto err_out; > + } > } > > dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); >
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index b0c7ab74a82e..7cf8210ebbec 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -2834,7 +2834,7 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, int i, qfe_slot = -1; char prom_name[64]; u8 addr[ETH_ALEN]; - int err; + int err = -ENODEV; /* Now make sure pci_dev cookie is there. */ #ifdef CONFIG_SPARC
Fix an uninitialized return code if we never found a qfe slot. It would be a bug if we ever got into this situation, but it's good to return something tracable. Fixes: acb3f35f920b ("sunhme: forward the error code from pci_enable_device()") Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> Signed-off-by: Sean Anderson <seanga2@gmail.com> --- Changes in v4: - Move this fix to its own commit drivers/net/ethernet/sun/sunhme.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)