* [PATCH] irqchip/apple-aic: application of sizeof() to a pointer @ 2022-03-10 5:02 Guo Zhengkui 2022-03-10 8:27 ` Marc Zyngier 0 siblings, 1 reply; 4+ messages in thread From: Guo Zhengkui @ 2022-03-10 5:02 UTC (permalink / raw) To: Hector Martin, Sven Peter, Alyssa Rosenzweig, Thomas Gleixner, Marc Zyngier, moderated list:ARM/APPLE MACHINE SUPPORT, open list:IRQCHIP DRIVERS Cc: zhengkui_guo, Guo Zhengkui `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%2FeUbdN1+ABFVWf@rowland.harvard.edu/ https://lore.kernel.org/all/YbBGGI9wQenI4kP7@kroah.com/ https://lore.kernel.org/all/20211209062441.9856-1-guozhengkui@vivo.com/ Signed-off-by: Guo Zhengkui <guozhengkui@vivo.com> --- 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] irqchip/apple-aic: application of sizeof() to a pointer 2022-03-10 5:02 [PATCH] irqchip/apple-aic: application of sizeof() to a pointer Guo Zhengkui @ 2022-03-10 8:27 ` Marc Zyngier 2022-03-10 8:48 ` Guo Zhengkui 0 siblings, 1 reply; 4+ messages in thread From: Marc Zyngier @ 2022-03-10 8:27 UTC (permalink / raw) To: Guo Zhengkui Cc: Hector Martin, Sven Peter, Alyssa Rosenzweig, Thomas Gleixner, moderated list:ARM/APPLE MACHINE SUPPORT, open list:IRQCHIP DRIVERS, zhengkui_guo On Thu, 10 Mar 2022 05:02:38 +0000, Guo Zhengkui <guozhengkui@vivo.com> 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%2FeUbdN1+ABFVWf@rowland.harvard.edu/ > https://lore.kernel.org/all/YbBGGI9wQenI4kP7@kroah.com/ > https://lore.kernel.org/all/20211209062441.9856-1-guozhengkui@vivo.com/ > > Signed-off-by: Guo Zhengkui <guozhengkui@vivo.com> > --- > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] irqchip/apple-aic: application of sizeof() to a pointer 2022-03-10 8:27 ` Marc Zyngier @ 2022-03-10 8:48 ` Guo Zhengkui 2022-03-10 9:00 ` Marc Zyngier 0 siblings, 1 reply; 4+ messages in thread From: Guo Zhengkui @ 2022-03-10 8:48 UTC (permalink / raw) To: Marc Zyngier Cc: Hector Martin, Sven Peter, Alyssa Rosenzweig, Thomas Gleixner, moderated list:ARM/APPLE MACHINE SUPPORT, open list:IRQCHIP DRIVERS, zhengkui_guo@outlook.com On 2022/3/10 16:27, Marc Zyngier wrote: > On Thu, 10 Mar 2022 05:02:38 +0000, > Guo Zhengkui <guozhengkui@vivo.com> 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%2FeUbdN1+ABFVWf@rowland.harvard.edu/ >> https://lore.kernel.org/all/YbBGGI9wQenI4kP7@kroah.com/ >> https://lore.kernel.org/all/20211209062441.9856-1-guozhengkui@vivo.com/ >> >> Signed-off-by: Guo Zhengkui <guozhengkui@vivo.com> >> --- >> 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; > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] irqchip/apple-aic: application of sizeof() to a pointer 2022-03-10 8:48 ` Guo Zhengkui @ 2022-03-10 9:00 ` Marc Zyngier 0 siblings, 0 replies; 4+ messages in thread From: Marc Zyngier @ 2022-03-10 9:00 UTC (permalink / raw) To: Guo Zhengkui Cc: Hector Martin, Sven Peter, Alyssa Rosenzweig, Thomas Gleixner, moderated list:ARM/APPLE MACHINE SUPPORT, open list:IRQCHIP DRIVERS, zhengkui_guo@outlook.com On Thu, 10 Mar 2022 08:48:07 +0000, Guo Zhengkui <guozhengkui@vivo.com> wrote: > > On 2022/3/10 16:27, Marc Zyngier wrote: > > On Thu, 10 Mar 2022 05:02:38 +0000, > > Guo Zhengkui <guozhengkui@vivo.com> 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%2FeUbdN1+ABFVWf@rowland.harvard.edu/ > >> https://lore.kernel.org/all/YbBGGI9wQenI4kP7@kroah.com/ > >> https://lore.kernel.org/all/20211209062441.9856-1-guozhengkui@vivo.com/ > >> > >> Signed-off-by: Guo Zhengkui <guozhengkui@vivo.com> > >> --- > >> 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-10 9:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-03-10 5:02 [PATCH] irqchip/apple-aic: application of sizeof() to a pointer Guo Zhengkui 2022-03-10 8:27 ` Marc Zyngier 2022-03-10 8:48 ` Guo Zhengkui 2022-03-10 9:00 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).