`ic->fiq_aff[fiq]` is a pointer to the unnamed struct.
`sizeof(ic->fiq_aff[fiq])` intends to get the size of this pointer. But
readers may get confused. `sizeof(unsigned long)` makes more sense because
`unsigned long` has the same size of pointer.
reference:
https://lore.kernel.org/all/Ya%[email protected]/
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
Signed-off-by: Guo Zhengkui <[email protected]>
---
drivers/irqchip/irq-apple-aic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
index b40199c6625e..23098b469b1a 100644
--- a/drivers/irqchip/irq-apple-aic.c
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
if (WARN_ON(n < 0))
return;
- ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]), GFP_KERNEL);
+ ic->fiq_aff[fiq] = kzalloc(sizeof(unsigned long), GFP_KERNEL);
if (!ic->fiq_aff[fiq])
return;
--
2.20.1
On Thu, 10 Mar 2022 05:02:38 +0000,
Guo Zhengkui <[email protected]> wrote:
>
> `ic->fiq_aff[fiq]` is a pointer to the unnamed struct.
> `sizeof(ic->fiq_aff[fiq])` intends to get the size of this pointer. But
> readers may get confused. `sizeof(unsigned long)` makes more sense because
> `unsigned long` has the same size of pointer.
More sense? It really depends on who reads the code.
>
> reference:
> https://lore.kernel.org/all/Ya%[email protected]/
> https://lore.kernel.org/all/[email protected]/
> https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Guo Zhengkui <[email protected]>
> ---
> drivers/irqchip/irq-apple-aic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
> index b40199c6625e..23098b469b1a 100644
> --- a/drivers/irqchip/irq-apple-aic.c
> +++ b/drivers/irqchip/irq-apple-aic.c
> @@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
> if (WARN_ON(n < 0))
> return;
>
> - ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]), GFP_KERNEL);
> + ic->fiq_aff[fiq] = kzalloc(sizeof(unsigned long), GFP_KERNEL);
And by doing that, you are making even more difficult to spot the
glaring bug that is in front of your eyes: we're not trying to
allocate a pointer, but to allocate the full data structure.
Nothing went wrong as a 64bit allocation is plenty when you need at
most 10 bits, but jeez, what a howler. I'm stashing the fixlet below
on top.
Thanks,
M.
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
index b40199c6625e..3f1d2f3ccb7f 100644
--- a/drivers/irqchip/irq-apple-aic.c
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
if (WARN_ON(n < 0))
return;
- ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]), GFP_KERNEL);
+ ic->fiq_aff[fiq] = kzalloc(sizeof(*ic->fiq_aff[fiq]), GFP_KERNEL);
if (!ic->fiq_aff[fiq])
return;
--
Without deviation from the norm, progress is not possible.
On 2022/3/10 16:27, Marc Zyngier wrote:
> On Thu, 10 Mar 2022 05:02:38 +0000,
> Guo Zhengkui <[email protected]> wrote:
>>
>> `ic->fiq_aff[fiq]` is a pointer to the unnamed struct.
>> `sizeof(ic->fiq_aff[fiq])` intends to get the size of this pointer. But
>> readers may get confused. `sizeof(unsigned long)` makes more sense because
>> `unsigned long` has the same size of pointer.
>
> More sense? It really depends on who reads the code.
>
>>
>> reference:
>> https://lore.kernel.org/all/Ya%[email protected]/
>> https://lore.kernel.org/all/[email protected]/
>> https://lore.kernel.org/all/[email protected]/
>>
>> Signed-off-by: Guo Zhengkui <[email protected]>
>> ---
>> drivers/irqchip/irq-apple-aic.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
>> index b40199c6625e..23098b469b1a 100644
>> --- a/drivers/irqchip/irq-apple-aic.c
>> +++ b/drivers/irqchip/irq-apple-aic.c
>> @@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
>> if (WARN_ON(n < 0))
>> return;
>>
>> - ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]), GFP_KERNEL);
>> + ic->fiq_aff[fiq] = kzalloc(sizeof(unsigned long), GFP_KERNEL);
>
> And by doing that, you are making even more difficult to spot the
> glaring bug that is in front of your eyes: we're not trying to
> allocate a pointer, but to allocate the full data structure.
>
Oh, I surely made a big mistake...
> Nothing went wrong as a 64bit allocation is plenty when you need at
> most 10 bits, but jeez, what a howler. I'm stashing the fixlet below
> on top.
>
So, will you send this new patch by yourself?
Zhengkui
> Thanks,
>
> M.
>
> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
> index b40199c6625e..3f1d2f3ccb7f 100644
> --- a/drivers/irqchip/irq-apple-aic.c
> +++ b/drivers/irqchip/irq-apple-aic.c
> @@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
> if (WARN_ON(n < 0))
> return;
>
> - ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]), GFP_KERNEL);
> + ic->fiq_aff[fiq] = kzalloc(sizeof(*ic->fiq_aff[fiq]), GFP_KERNEL);
> if (!ic->fiq_aff[fiq])
> return;
>
>
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: dc29812dbc873ae00bf19a4b8661984d7cce7a2e
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/dc29812dbc873ae00bf19a4b8661984d7cce7a2e
Author: Marc Zyngier <[email protected]>
AuthorDate: Thu, 10 Mar 2022 08:34:58
Committer: Marc Zyngier <[email protected]>
CommitterDate: Thu, 10 Mar 2022 08:58:34
irqchip/apple-aic: Fix cpumask allocation for FIQs
An emparassing typo: allocating a pointer instead of the object
pointed to. No harm done, as the pointer is large enough for
what we are using the object for, but still...
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-apple-aic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
index b40199c..3f1d2f3 100644
--- a/drivers/irqchip/irq-apple-aic.c
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
if (WARN_ON(n < 0))
return;
- ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]), GFP_KERNEL);
+ ic->fiq_aff[fiq] = kzalloc(sizeof(*ic->fiq_aff[fiq]), GFP_KERNEL);
if (!ic->fiq_aff[fiq])
return;
On Thu, 10 Mar 2022 08:48:07 +0000,
Guo Zhengkui <[email protected]> wrote:
>
> On 2022/3/10 16:27, Marc Zyngier wrote:
> > On Thu, 10 Mar 2022 05:02:38 +0000,
> > Guo Zhengkui <[email protected]> wrote:
> >>
> >> `ic->fiq_aff[fiq]` is a pointer to the unnamed struct.
> >> `sizeof(ic->fiq_aff[fiq])` intends to get the size of this pointer. But
> >> readers may get confused. `sizeof(unsigned long)` makes more sense because
> >> `unsigned long` has the same size of pointer.
> >
> > More sense? It really depends on who reads the code.
> >
> >>
> >> reference:
> >> https://lore.kernel.org/all/Ya%[email protected]/
> >> https://lore.kernel.org/all/[email protected]/
> >> https://lore.kernel.org/all/[email protected]/
> >>
> >> Signed-off-by: Guo Zhengkui <[email protected]>
> >> ---
> >> drivers/irqchip/irq-apple-aic.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
> >> index b40199c6625e..23098b469b1a 100644
> >> --- a/drivers/irqchip/irq-apple-aic.c
> >> +++ b/drivers/irqchip/irq-apple-aic.c
> >> @@ -810,7 +810,7 @@ static void build_fiq_affinity(struct aic_irq_chip *ic, struct device_node *aff)
> >> if (WARN_ON(n < 0))
> >> return;
> >> - ic->fiq_aff[fiq] = kzalloc(sizeof(ic->fiq_aff[fiq]),
> >> GFP_KERNEL);
> >> + ic->fiq_aff[fiq] = kzalloc(sizeof(unsigned long), GFP_KERNEL);
> >
> > And by doing that, you are making even more difficult to spot the
> > glaring bug that is in front of your eyes: we're not trying to
> > allocate a pointer, but to allocate the full data structure.
> >
>
> Oh, I surely made a big mistake...
Big mistake? No. You patch didn't change the behaviour of the code.
But in the future, you want to pay more attention to what the author
is trying to achieve: how is the data used, for example. Blindly
generalising an advice given out of context often leads to bad
patches.
>
> > Nothing went wrong as a 64bit allocation is plenty when you need at
> > most 10 bits, but jeez, what a howler. I'm stashing the fixlet below
> > on top.
> >
>
> So, will you send this new patch by yourself?
I've directly applied it[1].
Thanks,
M.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/irqchip-next&id=dc29812dbc873ae00bf19a4b8661984d7cce7a2e
--
Without deviation from the norm, progress is not possible.