Re: [PATCH] Update core.c

Re: [PATCH] Update core.c

[Conor Dooley]

[-- 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 --]

Re: [PATCH] Update core.c

[Jakub Kicinski]

On Mon, 30 Sep 2024 23:20:45 +0100 Conor Dooley wrote:
> (do netdev folks even want scoped cleanup?),

Since I have it handy... :)

Quoting documentation:

  Using device-managed and cleanup.h constructs
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  
  Netdev remains skeptical about promises of all "auto-cleanup" APIs,
  including even ``devm_`` helpers, historically. They are not the preferred
  style of implementation, merely an acceptable one.
  
  Use of ``guard()`` is discouraged within any function longer than 20 lines,
  ``scoped_guard()`` is considered more readable. Using normal lock/unlock is
  still (weakly) preferred.
  
  Low level cleanup constructs (such as ``__free()``) can be used when building
  APIs and helpers, especially scoped iterators. However, direct use of
  ``__free()`` within networking core and drivers is discouraged.
  Similar guidance applies to declaring variables mid-function.
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

Re: [PATCH] Update core.c

[Shuah Khan]

On 10/2/24 07:27, Jakub Kicinski wrote:
> On Mon, 30 Sep 2024 23:20:45 +0100 Conor Dooley wrote:
>> (do netdev folks even want scoped cleanup?),
> 
> Since I have it handy... :)
> 
> Quoting documentation:
> 
>    Using device-managed and cleanup.h constructs
>    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    
>    Netdev remains skeptical about promises of all "auto-cleanup" APIs,
>    including even ``devm_`` helpers, historically. They are not the preferred
>    style of implementation, merely an acceptable one.
>    
>    Use of ``guard()`` is discouraged within any function longer than 20 lines,
>    ``scoped_guard()`` is considered more readable. Using normal lock/unlock is
>    still (weakly) preferred.
>    
>    Low level cleanup constructs (such as ``__free()``) can be used when building
>    APIs and helpers, especially scoped iterators. However, direct use of
>    ``__free()`` within networking core and drivers is discouraged.
>    Similar guidance applies to declaring variables mid-function.
>    
> See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

Thank you. This will be helpful for new developers such as this
patch submitter to understand the scope of cleanup patches.

thanks,
-- Shuah

Re: [PATCH] Update core.c

[Al Viro]

On Wed, Oct 02, 2024 at 06:27:51AM -0700, Jakub Kicinski wrote:
> On Mon, 30 Sep 2024 23:20:45 +0100 Conor Dooley wrote:
> > (do netdev folks even want scoped cleanup?),
> 
> Since I have it handy... :)
> 
> Quoting documentation:
> 
>   Using device-managed and cleanup.h constructs
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   
>   Netdev remains skeptical about promises of all "auto-cleanup" APIs,
>   including even ``devm_`` helpers, historically. They are not the preferred
>   style of implementation, merely an acceptable one.
>   
>   Use of ``guard()`` is discouraged within any function longer than 20 lines,
>   ``scoped_guard()`` is considered more readable. Using normal lock/unlock is
>   still (weakly) preferred.
>   
>   Low level cleanup constructs (such as ``__free()``) can be used when building
>   APIs and helpers, especially scoped iterators. However, direct use of
>   ``__free()`` within networking core and drivers is discouraged.
>   Similar guidance applies to declaring variables mid-function.
>   
> See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

Bravo.  Mind if that gets stolen for VFS as well?

Re: [PATCH] Update core.c

[Jakub Kicinski]

On Wed, 2 Oct 2024 23:21:45 +0100 Al Viro wrote:
> > Quoting documentation:
> > 
> >   Using device-managed and cleanup.h constructs
> >   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   
> >   Netdev remains skeptical about promises of all "auto-cleanup" APIs,
> >   including even ``devm_`` helpers, historically. They are not the preferred
> >   style of implementation, merely an acceptable one.
> >   
> >   Use of ``guard()`` is discouraged within any function longer than 20 lines,
> >   ``scoped_guard()`` is considered more readable. Using normal lock/unlock is
> >   still (weakly) preferred.
> >   
> >   Low level cleanup constructs (such as ``__free()``) can be used when building
> >   APIs and helpers, especially scoped iterators. However, direct use of
> >   ``__free()`` within networking core and drivers is discouraged.
> >   Similar guidance applies to declaring variables mid-function.
> >   
> > See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs  
> 
> Bravo.  Mind if that gets stolen for VFS as well?

Not at all. Slight preference towards not mentioning that you got it
from us, tho, lest we attract unwanted attention :)