Re: [PATCH] Update core.c

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2817 bytes --]

On Mon, Sep 30, 2024 at 04:12:41PM -0600, Shuah Khan wrote:
> On 9/30/24 16:06, Okan Tumuklu wrote:
> > From: Okan Tümüklü <117488504+Okan-tumuklu@users.noreply.github.com>
> > 
> > 1:The control flow was simplified by using else if statements instead of goto structure.
> > 
> > 2:Error conditions are handled more clearly.
> > 
> > 3:The device_unlock call at the end of the function is guaranteed in all cases.
> 
> Write a paragraph - don't use bullet lists.
> 
> Please refer to submitting patches for details on how to
> write shortlogs and change logs.
> 
> "Update core.c" with what? Write a better short log.
> 
> Why do you this 117488504+Okan-tumuklu@users.noreply.github.com
> in the list? It will complain every time someone responds
> to this thread. This is not how patches are sent. Refer to
> documents in the kernel repo on how to send patches.
> 
> You are missing net maintainers and mailing lists.
> 
> Include all reviewers - run get_maintainers.pl

And consider whether the patch is a trip up the garden path, or
actually worthwhile.

Why would if/else be better than a goto?
What's unclear about the current error handling?
In what case is the device_unlock() call missed?

Maybe there's some value in using the scoped cleanup here (do netdev
folks even want scoped cleanup?), but this patch may not be worth the
time spent improving it. +CC Krzk and netdev, before more time is
potentially wasted here.

Cheers,
Conor.

> 
> > ---
> >   net/nfc/core.c | 28 ++++++++++------------------
> >   1 file changed, 10 insertions(+), 18 deletions(-)
> > 
> > diff --git a/net/nfc/core.c b/net/nfc/core.c
> > index e58dc6405054..4e8f01145c37 100644
> > --- a/net/nfc/core.c
> > +++ b/net/nfc/core.c
> > @@ -40,27 +40,19 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
> >   	if (dev->shutting_down) {
> >   		rc = -ENODEV;
> > -		goto error;
> > -	}
> > -
> > -	if (dev->dev_up) {
> > +	}else if (dev->dev_up) {
> >   		rc = -EBUSY;
> > -		goto error;
> > -	}
> 
> Did you run checkpack script on this patch? There are a few
> coding style errors.
> 
> > -
> > -	if (!dev->ops->fw_download) {
> > +	}else if (!dev->ops->fw_download) {
> >   		rc = -EOPNOTSUPP;
> > -		goto error;
> > -	}
> > -
> > -	dev->fw_download_in_progress = true;
> > -	rc = dev->ops->fw_download(dev, firmware_name);
> > -	if (rc)
> > -		dev->fw_download_in_progress = false;
> > +	}else{
> > +		dev->fw_download_in_progress = true;
> > +		rc = dev->ops->fw_download(dev, firmware_name);
> > +		if (rc)
> > +			dev->fw_download_in_progress = false;
> > +		}
> > -error:
> > -	device_unlock(&dev->dev);
> > -	return rc;
> > +		device_unlock(&dev->dev);
> > +		return rc;
> >   }
> >   /**
> 
> thanks,
> -- Shuah

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]