Hi Kishon and Lorenzo,
On Wed, 28 Feb 2018, Lorenzo Pieralisi wrote:
> On Wed, Feb 28, 2018 at 02:07:19PM +0100, Rolf Evers-Fischer wrote:
> > From: Rolf Evers-Fischer <rolf.evers.fischer@xxxxxxxxx>
> >
> > Removes the goto labels completely, handles the errors at the
> > respective call site and just returns instead of jumping around.
> >
> > Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@xxxxxxxxx>
> > ---
> > drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++---------------------
> > 1 file changed, 9 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > index 1878a6776519..cf2c4f6590a0 100644
> > --- a/drivers/pci/endpoint/pci-epf-core.c
> > +++ b/drivers/pci/endpoint/pci-epf-core.c
> > @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name)
> > int len;
> >
> > epf = kzalloc(sizeof(*epf), GFP_KERNEL);
> > - if (!epf) {
> > - ret = -ENOMEM;
> > - goto err_ret;
> > - }
> > + if (!epf)
> > + return ERR_PTR(-ENOMEM);
> >
> > len = strchrnul(name, '.') - name;
> > epf->name = kstrndup(name, len, GFP_KERNEL);
> > if (!epf->name) {
> > - ret = -ENOMEM;
> > - goto free_epf;
> > + kfree(epf);
> > + return ERR_PTR(-ENOMEM);
> > }
> >
> > dev = &epf->dev;
> > @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name)
> > dev->type = &pci_epf_type;
> >
> > ret = dev_set_name(dev, "%s", name);
> > - if (ret)
> > - goto put_dev;
> > -
> > - ret = device_add(dev);
> > - if (ret)
> > - goto put_dev;
> > -
> > - return epf;
> > + if (!ret) {
> > + ret = device_add(dev);
> > + if (!ret)
> > + return epf;
> > + }
>
> This is ugly, sorry. I should have been more explicit, I prefer
> this construct:
>
> ret = dev_set_name(dev, "%s", name);
> if (ret) {
> kfree(epf->name);
> kfree(epf);
> return ERR_PTR(ret);
> }
>
> ret = device_add(dev);
> if (ret) {
> put_device(dev);
> return ERR_PTR(ret);
> }
>
> return epf;
>
> Please send a v5 and let's be done with it.
>
Thank you for your feedback. I agree with you. Hiding the correct return
value under two levels of 'if' is not the best idea.
I will send a v5 containing the proposal from Lorenzo. There is only one
difference: We must call 'put_device()' in both error cases, because
we have already initialized the device before. We had also jumped into
'put_dev:' in both situations before.
The modified code will then look like this:
ret = dev_set_name(dev, "%s", name);
if (ret) {
put_device(dev);
return ERR_PTR(ret);
}
ret = device_add(dev);
if (ret) {
put_device(dev);
return ERR_PTR(ret);
}
return epf;
Best regards,
Rolf
> > -put_dev:
> > put_device(dev);
> > return ERR_PTR(ret);
> > -
> > -free_epf:
> > - kfree(epf);
> > -
> > -err_ret:
> > - return ERR_PTR(ret);
> > }
> > EXPORT_SYMBOL_GPL(pci_epf_create);
> >
> > --
> > 2.16.2
> >
>
