[PATCH v1 1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode()

[PATCH v1 1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode()

[Andy Shevchenko]

First of all, there is no need to call kasprintf() if the previous
allocation failed. Second, there is no need to call for kfree()
when we know that its parameter is NULL. Refactor the code accordingly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/irq/irqdomain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 0bdef4fe925b..d2bbba46c808 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -81,6 +81,8 @@ struct fwnode_handle *__irq_domain_alloc_fwnode(unsigned int type, int id,
 	char *n;
 
 	fwid = kzalloc(sizeof(*fwid), GFP_KERNEL);
+	if (!fwid)
+		return NULL;
 
 	switch (type) {
 	case IRQCHIP_FWNODE_NAMED:
@@ -93,10 +95,8 @@ struct fwnode_handle *__irq_domain_alloc_fwnode(unsigned int type, int id,
 		n = kasprintf(GFP_KERNEL, "irqchip@%pa", pa);
 		break;
 	}
-
-	if (!fwid || !n) {
+	if (!n) {
 		kfree(fwid);
-		kfree(n);
 		return NULL;
 	}
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode()

[Marc Zyngier]

On 2023-08-04 17:49, Andy Shevchenko wrote:
> First of all, there is no need to call kasprintf() if the previous
> allocation failed. Second, there is no need to call for kfree()
> when we know that its parameter is NULL. Refactor the code accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  kernel/irq/irqdomain.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 0bdef4fe925b..d2bbba46c808 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -81,6 +81,8 @@ struct fwnode_handle
> *__irq_domain_alloc_fwnode(unsigned int type, int id,
>  	char *n;
> 
>  	fwid = kzalloc(sizeof(*fwid), GFP_KERNEL);
> +	if (!fwid)
> +		return NULL;
> 
>  	switch (type) {
>  	case IRQCHIP_FWNODE_NAMED:
> @@ -93,10 +95,8 @@ struct fwnode_handle
> *__irq_domain_alloc_fwnode(unsigned int type, int id,
>  		n = kasprintf(GFP_KERNEL, "irqchip@%pa", pa);
>  		break;
>  	}
> -
> -	if (!fwid || !n) {
> +	if (!n) {
>  		kfree(fwid);
> -		kfree(n);
>  		return NULL;
>  	}

What are you trying to fix?

We have a common error handling path, which makes it easy to
track the memory management. I don't think this sort of bike
shedding adds much to the maintainability of this code.

Now if you have spotted an actual bug, I'm all ears.

       M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v1 1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode()

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 06:33:07PM +0100, Marc Zyngier wrote:
> On 2023-08-04 17:49, Andy Shevchenko wrote:
> > First of all, there is no need to call kasprintf() if the previous
> > allocation failed. Second, there is no need to call for kfree()
> > when we know that its parameter is NULL. Refactor the code accordingly.

...

> >  		n = kasprintf(GFP_KERNEL, "irqchip@%pa", pa);
> >  		break;
> >  	}
> > -
> > -	if (!fwid || !n) {
> > +	if (!n) {
> >  		kfree(fwid);
> > -		kfree(n);
> >  		return NULL;
> >  	}
> 
> What are you trying to fix?

I'm not trying to fix anything (there is no such statement from me),
but I would think of some micro-optimization (speedup boot for
unnoticeable time? Dunno.).

> We have a common error handling path, which makes it easy to
> track the memory management. I don't think this sort of bike
> shedding adds much to the maintainability of this code.

Your call, of course, but I not often see in the kernel two or three attempts
to allocate some memory and have grouped check for the failure.

> Now if you have spotted an actual bug, I'm all ears.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode()

[Marc Zyngier]

On Fri, 04 Aug 2023 21:12:11 +0100,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> On Fri, Aug 04, 2023 at 06:33:07PM +0100, Marc Zyngier wrote:
> > On 2023-08-04 17:49, Andy Shevchenko wrote:
> > > First of all, there is no need to call kasprintf() if the previous
> > > allocation failed. Second, there is no need to call for kfree()
> > > when we know that its parameter is NULL. Refactor the code accordingly.
> 
> ...
> 
> > >  		n = kasprintf(GFP_KERNEL, "irqchip@%pa", pa);
> > >  		break;
> > >  	}
> > > -
> > > -	if (!fwid || !n) {
> > > +	if (!n) {
> > >  		kfree(fwid);
> > > -		kfree(n);
> > >  		return NULL;
> > >  	}
> > 
> > What are you trying to fix?
> 
> I'm not trying to fix anything (there is no such statement from me),
> but I would think of some micro-optimization (speedup boot for
> unnoticeable time? Dunno.).

Error handling paths rarely qualify as an optimisation.

> 
> > We have a common error handling path, which makes it easy to
> > track the memory management. I don't think this sort of bike
> > shedding adds much to the maintainability of this code.
> 
> Your call, of course, but I not often see in the kernel two or three attempts
> to allocate some memory and have grouped check for the failure.

Things like this[1]? Well, this is a pattern I use often enough. Maybe
it isn't everybody's taste, but it suits me.

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-v3-its.c#n3438

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v1 1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode()

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 11:24:02PM +0100, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 21:12:11 +0100,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 04, 2023 at 06:33:07PM +0100, Marc Zyngier wrote:
> > > On 2023-08-04 17:49, Andy Shevchenko wrote:

...

> > > >  		n = kasprintf(GFP_KERNEL, "irqchip@%pa", pa);
> > > >  		break;
> > > >  	}
> > > > -
> > > > -	if (!fwid || !n) {
> > > > +	if (!n) {
> > > >  		kfree(fwid);
> > > > -		kfree(n);
> > > >  		return NULL;
> > > >  	}
> > > 
> > > What are you trying to fix?
> > 
> > I'm not trying to fix anything (there is no such statement from me),
> > but I would think of some micro-optimization (speedup boot for
> > unnoticeable time? Dunno.).
> 
> Error handling paths rarely qualify as an optimisation.

OK.

...

> > > We have a common error handling path, which makes it easy to
> > > track the memory management. I don't think this sort of bike
> > > shedding adds much to the maintainability of this code.
> > 
> > Your call, of course, but I not often see in the kernel two or three attempts
> > to allocate some memory and have grouped check for the failure.
> 
> Things like this[1]?

Yes.

> Well, this is a pattern I use often enough. Maybe
> it isn't everybody's taste, but it suits me.

Understand. Thanks for review!

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-v3-its.c#n3438

-- 
With Best Regards,
Andy Shevchenko