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 <[email protected]>
> ---
> 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...
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
On Fri, 04 Aug 2023 21:12:11 +0100,
Andy Shevchenko <[email protected]> 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.
On Fri, Aug 04, 2023 at 11:24:02PM +0100, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 21:12:11 +0100,
> Andy Shevchenko <[email protected]> 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