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

Message ID 20230804164932.40582-1-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v1,1/1] irqdomain: Refactor error path in __irq_domain_alloc_fwnode() |

Commit Message

Andy Shevchenko Aug. 4, 2023, 4:49 p.m. UTC
  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(-)
  

Comments

Marc Zyngier Aug. 4, 2023, 5:33 p.m. UTC | #1
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.
  
Andy Shevchenko Aug. 4, 2023, 8:12 p.m. UTC | #2
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.
  
Marc Zyngier Aug. 4, 2023, 10:24 p.m. UTC | #3
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
  
Andy Shevchenko Aug. 7, 2023, 3:06 p.m. UTC | #4
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
  

Patch

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;
 	}