| Message ID | 20230222170935.1820939-1-seanga2@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: sunhme: Return an error when we are out of slots | expand |
On Wed, Feb 22, 2023 at 12:09:35PM -0500, Sean Anderson wrote: > We only allocate enough space for four devices when the parent is a QFE. If > we couldn't find a spot (because five devices were created for whatever > reason), we would not return an error from probe(). Return ENODEV, which > was what we did before. > > Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()") I think the hash for that commit is acb3f35f920b. However, I also think this problem was introduced by the first hunk of 5b3dc6dda6b1 ("sunhme: Regularize probe errors"). Which is: --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -2945,7 +2945,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, if (err) goto err_out; pci_set_master(pdev); - err = -ENODEV; if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) { qp = quattro_pci_find(pdev); Which leads me to wonder if simpler fixes would be either: 1) Reverting the hunk above 2) Or, more in keeping with the rest of that patch, explicitly setting err before branching to err_out, as you your patch does, but without other logic changes. Something like this (*compile tested only!*: diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index 1c16548415cd..2409e7d6c29e 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 = -ENOMEM; goto err_out; + } } dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); Also, I am curious why happy_meal_pci_probe() doesn't just return instaed of branching to err_out. As err_out only returns err. I guess there is a reason for it. But simply returning would probably simplify error handling. (I'm not suggesting that approach for this fix.) > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <error27@gmail.com> > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > drivers/net/ethernet/sun/sunhme.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index 1c16548415cd..523e26653ec8 100644 > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -2861,12 +2861,13 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > > for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) > if (!qp->happy_meals[qfe_slot]) > - break; > + goto found_slot; > > - if (qfe_slot == 4) > - goto err_out; > + err = -ENODEV; > + goto err_out; > } > > +found_slot: > dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); > if (!dev) { > err = -ENOMEM; > -- > 2.37.1 >
On 2/22/23 15:59, Simon Horman wrote: > On Wed, Feb 22, 2023 at 12:09:35PM -0500, Sean Anderson wrote: >> We only allocate enough space for four devices when the parent is a QFE. If >> we couldn't find a spot (because five devices were created for whatever >> reason), we would not return an error from probe(). Return ENODEV, which >> was what we did before. >> >> Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()") > > I think the hash for that commit is acb3f35f920b. Ah, sorry that's my local copy. The upstream commit is as you noted. > > However, I also think this problem was introduced by the first hunk of > 5b3dc6dda6b1 ("sunhme: Regularize probe errors"). > > Which is: > > --- a/drivers/net/ethernet/sun/sunhme.c > +++ b/drivers/net/ethernet/sun/sunhme.c > @@ -2945,7 +2945,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > if (err) > goto err_out; > pci_set_master(pdev); > - err = -ENODEV; > > if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) { > qp = quattro_pci_find(pdev); > Yes. That's the one I should have blamed. > Which leads me to wonder if simpler fixes would be either: > > 1) Reverting the hunk above > 2) Or, more in keeping with the rest of that patch, > explicitly setting err before branching to err_out, > as you your patch does, but without other logic changes. > Something like this (*compile tested only!*: > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > index 1c16548415cd..2409e7d6c29e 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 = -ENOMEM; > goto err_out; > + } > } > > dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); That's of course simpler, but this also does some cleanup to make it more obvious what's going on. > Also, I am curious why happy_meal_pci_probe() doesn't just return instaed > of branching to err_out. As err_out only returns err. I guess there is a > reason for it. But simply returning would probably simplify error handling. > (I'm not suggesting that approach for this fix.) I think it's because there used to be cleanup in err_out. But you're right, we can just return directly and avoid a goto. --Sean >> Reported-by: kernel test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <error27@gmail.com> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> drivers/net/ethernet/sun/sunhme.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c >> index 1c16548415cd..523e26653ec8 100644 >> --- a/drivers/net/ethernet/sun/sunhme.c >> +++ b/drivers/net/ethernet/sun/sunhme.c >> @@ -2861,12 +2861,13 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, >> >> for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) >> if (!qp->happy_meals[qfe_slot]) >> - break; >> + goto found_slot; >> >> - if (qfe_slot == 4) >> - goto err_out; >> + err = -ENODEV; >> + goto err_out; >> } >> >> +found_slot: >> dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); >> if (!dev) { >> err = -ENOMEM; >> -- >> 2.37.1 >>
On Wed, Feb 22, 2023 at 04:07:15PM -0500, Sean Anderson wrote: > On 2/22/23 15:59, Simon Horman wrote: > > On Wed, Feb 22, 2023 at 12:09:35PM -0500, Sean Anderson wrote: > > > We only allocate enough space for four devices when the parent is a QFE. If > > > we couldn't find a spot (because five devices were created for whatever > > > reason), we would not return an error from probe(). Return ENODEV, which > > > was what we did before. > > > > > > Fixes: 96c6e9faecf1 ("sunhme: forward the error code from pci_enable_device()") > > > > I think the hash for that commit is acb3f35f920b. > > Ah, sorry that's my local copy. The upstream commit is as you noted. > > > > > However, I also think this problem was introduced by the first hunk of > > 5b3dc6dda6b1 ("sunhme: Regularize probe errors"). > > > > Which is: > > > > --- a/drivers/net/ethernet/sun/sunhme.c > > +++ b/drivers/net/ethernet/sun/sunhme.c > > @@ -2945,7 +2945,6 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > > if (err) > > goto err_out; > > pci_set_master(pdev); > > - err = -ENODEV; > > if (!strcmp(prom_name, "SUNW,qfe") || !strcmp(prom_name, "qfe")) { > > qp = quattro_pci_find(pdev); > > > > Yes. That's the one I should have blamed. > > > Which leads me to wonder if simpler fixes would be either: > > > > 1) Reverting the hunk above > > 2) Or, more in keeping with the rest of that patch, > > explicitly setting err before branching to err_out, > > as you your patch does, but without other logic changes. > > > Something like this (*compile tested only!*: > > > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > > index 1c16548415cd..2409e7d6c29e 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 = -ENOMEM; > > goto err_out; > > + } > > } > > dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); > > That's of course simpler, but this also does some cleanup to make it more > obvious what's going on. Sure. I do think there is merit in a minimal change as a fix. But I don't feel strongly about it. > > Also, I am curious why happy_meal_pci_probe() doesn't just return instaed > > of branching to err_out. As err_out only returns err. I guess there is a > > reason for it. But simply returning would probably simplify error handling. > > (I'm not suggesting that approach for this fix.) > > I think it's because there used to be cleanup in err_out. But you're right, > we can just return directly and avoid a goto. Makes sense, thanks. > --Sean > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Reported-by: Dan Carpenter <error27@gmail.com> > > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > > > --- > > > > > > drivers/net/ethernet/sun/sunhme.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c > > > index 1c16548415cd..523e26653ec8 100644 > > > --- a/drivers/net/ethernet/sun/sunhme.c > > > +++ b/drivers/net/ethernet/sun/sunhme.c > > > @@ -2861,12 +2861,13 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, > > > for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) > > > if (!qp->happy_meals[qfe_slot]) > > > - break; > > > + goto found_slot; > > > - if (qfe_slot == 4) > > > - goto err_out; > > > + err = -ENODEV; > > > + goto err_out; > > > } > > > +found_slot: > > > dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); > > > if (!dev) { > > > err = -ENOMEM; > > > -- > > > 2.37.1 > > > >
diff --git a/drivers/net/ethernet/sun/sunhme.c b/drivers/net/ethernet/sun/sunhme.c index 1c16548415cd..523e26653ec8 100644 --- a/drivers/net/ethernet/sun/sunhme.c +++ b/drivers/net/ethernet/sun/sunhme.c @@ -2861,12 +2861,13 @@ static int happy_meal_pci_probe(struct pci_dev *pdev, for (qfe_slot = 0; qfe_slot < 4; qfe_slot++) if (!qp->happy_meals[qfe_slot]) - break; + goto found_slot; - if (qfe_slot == 4) - goto err_out; + err = -ENODEV; + goto err_out; } +found_slot: dev = devm_alloc_etherdev(&pdev->dev, sizeof(struct happy_meal)); if (!dev) { err = -ENOMEM;
We only allocate enough space for four devices when the parent is a QFE. If we couldn't find a spot (because five devices were created for whatever reason), we would not return an error from probe(). Return ENODEV, which was what we did before. Fixes: 96c6e9faecf1 ("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> --- drivers/net/ethernet/sun/sunhme.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)