gpio/sifive: fix static checker warning
diff mbox series

Message ID 1580189061-14091-1-git-send-email-yash.shah@sifive.com
State Accepted
Commit a924eae75106258c0797706a0578c5af499c9ff5
Headers show
Series
  • gpio/sifive: fix static checker warning
Related show

Commit Message

Yash Shah Jan. 28, 2020, 5:24 a.m. UTC
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(-)

Comments

Marc Zyngier Jan. 28, 2020, 1:21 p.m. UTC | #1
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.
JaeJoon Jung Jan. 29, 2020, 1:27 a.m. UTC | #2
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...
>
Marc Zyngier Jan. 29, 2020, 9:12 a.m. UTC | #3
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.
Linus Walleij Jan. 29, 2020, 10:02 a.m. UTC | #4
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
Linus Walleij Feb. 10, 2020, 12:55 p.m. UTC | #5
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
Palmer Dabbelt Feb. 10, 2020, 5:59 p.m. UTC | #6
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!

Patch
diff mbox series

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