| Message ID | 1580189061-14091-1-git-send-email-yash.shah@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio/sifive: fix static checker warning | expand |
On 2020-01-28 05:24, Yash Shah wrote: > Typcasting "irq_state" leads to the below static checker warning: > The fix is to declare "irq_state" as unsigned long instead of u32. > > drivers/gpio/gpio-sifive.c:97 sifive_gpio_irq_enable() > warn: passing casted pointer '&chip->irq_state' to > 'assign_bit()' 32 vs 64. > > Fixes: 96868dce644d ("gpio/sifive: Add GPIO driver for SiFive SoCs") > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Yash Shah <yash.shah@sifive.com> > --- > drivers/gpio/gpio-sifive.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c > index 147a1bd..c54dd08 100644 > --- a/drivers/gpio/gpio-sifive.c > +++ b/drivers/gpio/gpio-sifive.c > @@ -35,7 +35,7 @@ struct sifive_gpio { > void __iomem *base; > struct gpio_chip gc; > struct regmap *regs; > - u32 irq_state; > + unsigned long irq_state; > unsigned int trigger[SIFIVE_GPIO_MAX]; > unsigned int irq_parent[SIFIVE_GPIO_MAX]; > }; > @@ -94,7 +94,7 @@ static void sifive_gpio_irq_enable(struct irq_data > *d) > spin_unlock_irqrestore(&gc->bgpio_lock, flags); > > /* Enable interrupts */ > - assign_bit(offset, (unsigned long *)&chip->irq_state, 1); > + assign_bit(offset, &chip->irq_state, 1); > sifive_gpio_set_ie(chip, offset); > } > > @@ -104,7 +104,7 @@ static void sifive_gpio_irq_disable(struct irq_data > *d) > struct sifive_gpio *chip = gpiochip_get_data(gc); > int offset = irqd_to_hwirq(d) % SIFIVE_GPIO_MAX; > > - assign_bit(offset, (unsigned long *)&chip->irq_state, 0); > + assign_bit(offset, &chip->irq_state, 0); > sifive_gpio_set_ie(chip, offset); > irq_chip_disable_parent(d); > } Yup, nice one. Should have spotted it. Reviewed-by: Marc Zyngier <maz@kernel.org> Linus, do you want me to queue this via the irqchip tree (given that it is where the original bug came from)? Or would you rather take it? M.
Because SiFive FU540 GPIO Registers are aligned 32-bit, I think that irq_state is good "unsigned int" than "unsigned long". I refer to SiFive FU540-C000 Manual v1p0 (GPIO Register Table 103) as "Only naturally aligned 32-bit memory accesses are supported" when you use assign_bit(offset, &chip->irq_state, 1), offset is 32-bit. I prefer to use bit operation instead of assign_bit(). u32 bit = BIT(offset); chip->irq_state |= bit; If you use this code, you do not use the assign_bit() and need not change irq_state data type. There are many improvements in my works for drivers/gpio/gpio-sifive.c. I hope to check my attached source file. On Tue, 28 Jan 2020 at 22:21, Marc Zyngier <maz@kernel.org> wrote: > > On 2020-01-28 05:24, Yash Shah wrote: > > Typcasting "irq_state" leads to the below static checker warning: > > The fix is to declare "irq_state" as unsigned long instead of u32. > > > > drivers/gpio/gpio-sifive.c:97 sifive_gpio_irq_enable() > > warn: passing casted pointer '&chip->irq_state' to > > 'assign_bit()' 32 vs 64. > > > > Fixes: 96868dce644d ("gpio/sifive: Add GPIO driver for SiFive SoCs") > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Yash Shah <yash.shah@sifive.com> > > --- > > drivers/gpio/gpio-sifive.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c > > index 147a1bd..c54dd08 100644 > > --- a/drivers/gpio/gpio-sifive.c > > +++ b/drivers/gpio/gpio-sifive.c > > @@ -35,7 +35,7 @@ struct sifive_gpio { > > void __iomem *base; > > struct gpio_chip gc; > > struct regmap *regs; > > - u32 irq_state; > > + unsigned long irq_state; > > unsigned int trigger[SIFIVE_GPIO_MAX]; > > unsigned int irq_parent[SIFIVE_GPIO_MAX]; > > }; > > @@ -94,7 +94,7 @@ static void sifive_gpio_irq_enable(struct irq_data > > *d) > > spin_unlock_irqrestore(&gc->bgpio_lock, flags); > > > > /* Enable interrupts */ > > - assign_bit(offset, (unsigned long *)&chip->irq_state, 1); > > + assign_bit(offset, &chip->irq_state, 1); > > sifive_gpio_set_ie(chip, offset); > > } > > > > @@ -104,7 +104,7 @@ static void sifive_gpio_irq_disable(struct irq_data > > *d) > > struct sifive_gpio *chip = gpiochip_get_data(gc); > > int offset = irqd_to_hwirq(d) % SIFIVE_GPIO_MAX; > > > > - assign_bit(offset, (unsigned long *)&chip->irq_state, 0); > > + assign_bit(offset, &chip->irq_state, 0); > > sifive_gpio_set_ie(chip, offset); > > irq_chip_disable_parent(d); > > } > > Yup, nice one. Should have spotted it. > > Reviewed-by: Marc Zyngier <maz@kernel.org> > > Linus, do you want me to queue this via the irqchip tree (given that > it is where the original bug came from)? Or would you rather take it? > > M. > -- > Jazz is not dead. It just smells funny... >
JaeJoon, On 2020-01-29 01:27, JaeJoon Jung wrote: > Because SiFive FU540 GPIO Registers are aligned 32-bit, > I think that irq_state is good "unsigned int" than "unsigned long". > > I refer to SiFive FU540-C000 Manual v1p0 (GPIO Register Table 103) > as "Only naturally aligned 32-bit memory accesses are supported" You realize that we're talking about variables here, and not hardware registers, right? > when you use assign_bit(offset, &chip->irq_state, 1), > offset is 32-bit. And? assign_bit takes an "unsigned long *" as a parameter. irrespective of the size of long on a given architecture, by the way. > I prefer to use bit operation instead of assign_bit(). > > u32 bit = BIT(offset); > chip->irq_state |= bit; which is not what assign_bit() does. > If you use this code, you do not use the assign_bit() and > need not change irq_state data type. Or we can use the correct API and not introduce additional bugs, which your suggestion above would lead to. > There are many improvements in my works for drivers/gpio/gpio-sifive.c. > I hope to check my attached source file. That's not how we submit patches to the Linux kernel. I suggest you read the documentation on how to do this. Thanks, M.
On Tue, Jan 28, 2020 at 2:21 PM Marc Zyngier <maz@kernel.org> wrote: > Linus, do you want me to queue this via the irqchip tree (given that > it is where the original bug came from)? Or would you rather take it? I can take it, I just need to get my own changes for GPIO in first so I'll apply this past v5.6-rc1. Yours, Linus Walleij
On Tue, Jan 28, 2020 at 6:24 AM Yash Shah <yash.shah@sifive.com> wrote: > Typcasting "irq_state" leads to the below static checker warning: > The fix is to declare "irq_state" as unsigned long instead of u32. > > drivers/gpio/gpio-sifive.c:97 sifive_gpio_irq_enable() > warn: passing casted pointer '&chip->irq_state' to > 'assign_bit()' 32 vs 64. > > Fixes: 96868dce644d ("gpio/sifive: Add GPIO driver for SiFive SoCs") > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Yash Shah <yash.shah@sifive.com> Patch applied for GPIO fixes. Yours, Linus Walleij
On Mon, 27 Jan 2020 21:24:21 PST (-0800), yash.shah@sifive.com wrote: > Typcasting "irq_state" leads to the below static checker warning: > The fix is to declare "irq_state" as unsigned long instead of u32. > > drivers/gpio/gpio-sifive.c:97 sifive_gpio_irq_enable() > warn: passing casted pointer '&chip->irq_state' to > 'assign_bit()' 32 vs 64. > > Fixes: 96868dce644d ("gpio/sifive: Add GPIO driver for SiFive SoCs") > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Yash Shah <yash.shah@sifive.com> > --- > drivers/gpio/gpio-sifive.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c > index 147a1bd..c54dd08 100644 > --- a/drivers/gpio/gpio-sifive.c > +++ b/drivers/gpio/gpio-sifive.c > @@ -35,7 +35,7 @@ struct sifive_gpio { > void __iomem *base; > struct gpio_chip gc; > struct regmap *regs; > - u32 irq_state; > + unsigned long irq_state; > unsigned int trigger[SIFIVE_GPIO_MAX]; > unsigned int irq_parent[SIFIVE_GPIO_MAX]; > }; > @@ -94,7 +94,7 @@ static void sifive_gpio_irq_enable(struct irq_data *d) > spin_unlock_irqrestore(&gc->bgpio_lock, flags); > > /* Enable interrupts */ > - assign_bit(offset, (unsigned long *)&chip->irq_state, 1); > + assign_bit(offset, &chip->irq_state, 1); > sifive_gpio_set_ie(chip, offset); > } > > @@ -104,7 +104,7 @@ static void sifive_gpio_irq_disable(struct irq_data *d) > struct sifive_gpio *chip = gpiochip_get_data(gc); > int offset = irqd_to_hwirq(d) % SIFIVE_GPIO_MAX; > > - assign_bit(offset, (unsigned long *)&chip->irq_state, 0); > + assign_bit(offset, &chip->irq_state, 0); > sifive_gpio_set_ie(chip, offset); > irq_chip_disable_parent(d); > } Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> I'm assuming this is going to go in via some other tree (as I don't even have gpio-sifive.c yet), but LMK if you want it via the RISC-V tree. Thanks!
diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c index 147a1bd..c54dd08 100644 --- a/drivers/gpio/gpio-sifive.c +++ b/drivers/gpio/gpio-sifive.c @@ -35,7 +35,7 @@ struct sifive_gpio { void __iomem *base; struct gpio_chip gc; struct regmap *regs; - u32 irq_state; + unsigned long irq_state; unsigned int trigger[SIFIVE_GPIO_MAX]; unsigned int irq_parent[SIFIVE_GPIO_MAX]; }; @@ -94,7 +94,7 @@ static void sifive_gpio_irq_enable(struct irq_data *d) spin_unlock_irqrestore(&gc->bgpio_lock, flags); /* Enable interrupts */ - assign_bit(offset, (unsigned long *)&chip->irq_state, 1); + assign_bit(offset, &chip->irq_state, 1); sifive_gpio_set_ie(chip, offset); } @@ -104,7 +104,7 @@ static void sifive_gpio_irq_disable(struct irq_data *d) struct sifive_gpio *chip = gpiochip_get_data(gc); int offset = irqd_to_hwirq(d) % SIFIVE_GPIO_MAX; - assign_bit(offset, (unsigned long *)&chip->irq_state, 0); + assign_bit(offset, &chip->irq_state, 0); sifive_gpio_set_ie(chip, offset); irq_chip_disable_parent(d); }
Typcasting "irq_state" leads to the below static checker warning: The fix is to declare "irq_state" as unsigned long instead of u32. drivers/gpio/gpio-sifive.c:97 sifive_gpio_irq_enable() warn: passing casted pointer '&chip->irq_state' to 'assign_bit()' 32 vs 64. Fixes: 96868dce644d ("gpio/sifive: Add GPIO driver for SiFive SoCs") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Yash Shah <yash.shah@sifive.com> --- drivers/gpio/gpio-sifive.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)