cxl/port: automate cleanup with __free()

[PATCH] cxl/port: automate cleanup with __free()

Posted by Pranav Tyagi 3 months, 2 weeks ago

Use the scope based resource management (defined in linux/cleanup.h) to
automate the lifetime control of struct cxl_endpoint_decoder. This
eliminates the explicit kfree() call and makes the code more robust and
maintainable in presence of early returns.

Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
---
 drivers/cxl/core/port.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index eb46c6764d20..c35946882b20 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -10,6 +10,7 @@
 #include <linux/slab.h>
 #include <linux/idr.h>
 #include <linux/node.h>
+#include <linux/cleanup.h>
 #include <cxl/einj.h>
 #include <cxlmem.h>
 #include <cxlpci.h>
@@ -1888,14 +1889,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, "CXL");
  */
 struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
 {
-	struct cxl_endpoint_decoder *cxled;
 	struct cxl_decoder *cxld;
 	int rc;
 
 	if (!is_cxl_endpoint(port))
 		return ERR_PTR(-EINVAL);
 
-	cxled = kzalloc(sizeof(*cxled), GFP_KERNEL);
+	struct cxl_endpoint_decoder *cxled __free(kfree) =
+		kzalloc(sizeof(*cxled), GFP_KERNEL);
 	if (!cxled)
 		return ERR_PTR(-ENOMEM);
 
@@ -1904,7 +1905,6 @@ struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
 	cxld = &cxled->cxld;
 	rc = cxl_decoder_init(port, cxld);
 	if (rc)	 {
-		kfree(cxled);
 		return ERR_PTR(rc);
 	}
 
-- 
2.49.0

Re: [PATCH] cxl/port: automate cleanup with __free()

Posted by Dan Williams 3 months, 2 weeks ago

Pranav Tyagi wrote:
> Use the scope based resource management (defined in linux/cleanup.h) to
> automate the lifetime control of struct cxl_endpoint_decoder. This
> eliminates the explicit kfree() call and makes the code more robust and
> maintainable in presence of early returns.

I do not want to review small standalone conversions of individual
functions for no other reason than vague claims of improvement. The
reasons to convert an existing function to cleanup.h helpers are:

1/ to fix an actual bug

2/ in the course of refactoring the code for other reasons

3/ to eliminate goto trickiness and bulk

This patch does not make the code "more robust and maintainable", it is
just churn given how easy it is to verify that the kfree() is correctly
paired.

One quick way to identify code churn is when the diffstat does not
result in a net reduction in code lines:

This patch is "3 insertions(+), 3 deletions(-)" == "churn".

Re: [PATCH] cxl/port: automate cleanup with __free()

Posted by Pranav Tyagi 3 months, 2 weeks ago

On Tue, Jun 24, 2025 at 10:42 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Pranav Tyagi wrote:
> > Use the scope based resource management (defined in linux/cleanup.h) to
> > automate the lifetime control of struct cxl_endpoint_decoder. This
> > eliminates the explicit kfree() call and makes the code more robust and
> > maintainable in presence of early returns.
>
> I do not want to review small standalone conversions of individual
> functions for no other reason than vague claims of improvement. The
> reasons to convert an existing function to cleanup.h helpers are:
>
> 1/ to fix an actual bug
>
> 2/ in the course of refactoring the code for other reasons
>
> 3/ to eliminate goto trickiness and bulk
>
> This patch does not make the code "more robust and maintainable", it is
> just churn given how easy it is to verify that the kfree() is correctly
> paired.
>
> One quick way to identify code churn is when the diffstat does not
> result in a net reduction in code lines:
>
> This patch is "3 insertions(+), 3 deletions(-)" == "churn".

Thank you for the feedback. I understand your concerns and completely
agree with your reasoning. Please pardon my misjudgment in sending this
patch. I am still a beginner with kernel development and learning to
better assess what makes a meaningful contribution.

Regards
Pranav Tyagi

Re: [PATCH] cxl/port: automate cleanup with __free()

Posted by Greg KH 3 months, 2 weeks ago

On Mon, Jun 23, 2025 at 02:49:29PM +0530, Pranav Tyagi wrote:
> Use the scope based resource management (defined in linux/cleanup.h) to
> automate the lifetime control of struct cxl_endpoint_decoder. This
> eliminates the explicit kfree() call and makes the code more robust and
> maintainable in presence of early returns.
> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> ---
>  drivers/cxl/core/port.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index eb46c6764d20..c35946882b20 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -10,6 +10,7 @@
>  #include <linux/slab.h>
>  #include <linux/idr.h>
>  #include <linux/node.h>
> +#include <linux/cleanup.h>
>  #include <cxl/einj.h>
>  #include <cxlmem.h>
>  #include <cxlpci.h>
> @@ -1888,14 +1889,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, "CXL");
>   */
>  struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
>  {
> -	struct cxl_endpoint_decoder *cxled;
>  	struct cxl_decoder *cxld;
>  	int rc;
>  
>  	if (!is_cxl_endpoint(port))
>  		return ERR_PTR(-EINVAL);
>  
> -	cxled = kzalloc(sizeof(*cxled), GFP_KERNEL);
> +	struct cxl_endpoint_decoder *cxled __free(kfree) =
> +		kzalloc(sizeof(*cxled), GFP_KERNEL);
>  	if (!cxled)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -1904,7 +1905,6 @@ struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
>  	cxld = &cxled->cxld;
>  	rc = cxl_decoder_init(port, cxld);
>  	if (rc)	 {
> -		kfree(cxled);
>  		return ERR_PTR(rc);
>  	}
>  
> -- 
> 2.49.0

Note, I can't speak for the maintainers of this subsystem, but
generally, making changes like this for no real good reason, for code
that has been around for years, is really not needed at all.

If you fix a bug with it, sure, but changes for the sake of "let's use
this new feature" in here really might not be necessary.

Why not add cleanup.h support to code paths that actually fix existing
bugs instead?

Also, you have added some coding style errors to the code now with this
patch, which also is generally not considered a good idea :)

thanks,

greg k-h

Re: [PATCH] cxl/port: automate cleanup with __free()

Posted by Pranav Tyagi 3 months, 2 weeks ago

On Mon, Jun 23, 2025 at 2:57 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 23, 2025 at 02:49:29PM +0530, Pranav Tyagi wrote:
> > Use the scope based resource management (defined in linux/cleanup.h) to
> > automate the lifetime control of struct cxl_endpoint_decoder. This
> > eliminates the explicit kfree() call and makes the code more robust and
> > maintainable in presence of early returns.
> >
> > Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> > ---
> >  drivers/cxl/core/port.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index eb46c6764d20..c35946882b20 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/idr.h>
> >  #include <linux/node.h>
> > +#include <linux/cleanup.h>
> >  #include <cxl/einj.h>
> >  #include <cxlmem.h>
> >  #include <cxlpci.h>
> > @@ -1888,14 +1889,14 @@ EXPORT_SYMBOL_NS_GPL(cxl_switch_decoder_alloc, "CXL");
> >   */
> >  struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
> >  {
> > -     struct cxl_endpoint_decoder *cxled;
> >       struct cxl_decoder *cxld;
> >       int rc;
> >
> >       if (!is_cxl_endpoint(port))
> >               return ERR_PTR(-EINVAL);
> >
> > -     cxled = kzalloc(sizeof(*cxled), GFP_KERNEL);
> > +     struct cxl_endpoint_decoder *cxled __free(kfree) =
> > +             kzalloc(sizeof(*cxled), GFP_KERNEL);
> >       if (!cxled)
> >               return ERR_PTR(-ENOMEM);
> >
> > @@ -1904,7 +1905,6 @@ struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port)
> >       cxld = &cxled->cxld;
> >       rc = cxl_decoder_init(port, cxld);
> >       if (rc)  {
> > -             kfree(cxled);
> >               return ERR_PTR(rc);
> >       }
> >
> > --
> > 2.49.0
>
> Note, I can't speak for the maintainers of this subsystem, but
> generally, making changes like this for no real good reason, for code
> that has been around for years, is really not needed at all.
>
> If you fix a bug with it, sure, but changes for the sake of "let's use
> this new feature" in here really might not be necessary.
>
> Why not add cleanup.h support to code paths that actually fix existing
> bugs instead?
>
> Also, you have added some coding style errors to the code now with this
> patch, which also is generally not considered a good idea :)
>
> thanks,
>
> greg k-h

Hi Greg,

Thanks for the feedback.

This patch was intended as a cleanup to improve consistency and
readability, particularly around handling early returns using
cleanup.h. I followed up on similar prior patches that introduced
automatic cleanup in this subsystem and aimed to extend that style
more consistently.

That said, I understand your concern about changing long-standing
code without a strong functional reason. I’ll be more careful going
forward and focus such changes around actual bug fixes.

Regards
Pranav Tyagi