Re: [PATCH] gpio/sifive: fix static checker warning

From: JaeJoon Jung
Date: Tue Jan 28 2020 - 20:28:22 EST

Next message: Dan Williams: "Re: [PATCH][RFC] PCI: Add "pci=blacklist_dev=" parameter to blacklist specific devices"
Previous message: Stephen Rothwell: "Re: linux-next: manual merge of the selinux tree with the keys tree"
In reply to: Marc Zyngier: "Re: [PATCH] gpio/sifive: fix static checker warning"
Next in thread: Marc Zyngier: "Re: [PATCH] gpio/sifive: fix static checker warning"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Yash Shah <yash.shah@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> 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...
>
/*
* SiFive GPIO driver
*
* Copyright (C) 2018 SiFive, Inc.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
*
* References:
* SiFive FU540-C000 manual v1p0, Chapter 17 "GPIO"
*
* 2020 Editted by JaeJoon Jung <rgbi3307@xxxxxxxxx>
*
*/
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/errno.h>
#include <linux/gpio/driver.h>
#include <linux/irqchip/chained_irq.h>
#include <linux/io.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/of_irq.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/raid/pq.h>

#define GPIO_INPUT_VAL 0x00
#define GPIO_INPUT_EN 0x04
#define GPIO_OUTPUT_EN 0x08
#define GPIO_OUTPUT_VAL 0x0C
#define GPIO_PULLUP_EN 0x10
#define GPIO_PIN_DS 0x14
#define GPIO_RISE_IE 0x18
#define GPIO_RISE_IP 0x1C
#define GPIO_FALL_IE 0x20
#define GPIO_FALL_IP 0x24
#define GPIO_HIGH_IE 0x28
#define GPIO_HIGH_IP 0x2C
#define GPIO_LOW_IE 0x30
#define GPIO_LOW_IP 0x34
#define GPIO_OUTPUT_XOR 0x40

#define GPIO_MAX_CNT 32
#define GPIO_ENABLE_BITS 0x83FF

//#define GPIO_SIFIVE_DEBUG
#ifdef GPIO_SIFIVE_DEBUG
#define gpio_sifive_debug(...) printk("GPIO: " __VA_ARGS__)
#else
#define gpio_sifive_debug(...)
#endif

struct sifive_gpio {
raw_spinlock_t lock;
void __iomem *base;
struct gpio_chip gc;
unsigned int irq_enable;
unsigned int irq_type[GPIO_MAX_CNT];
unsigned int irq_parent[GPIO_MAX_CNT];
struct sifive_gpio *self_ptr[GPIO_MAX_CNT];
};

static void gpio_sifive_debug_reg(struct sifive_gpio *chip)
{
#ifdef GPIO_SIFIVE_DEBUG
u32 value;
int reg;

if (!chip->base) return;
gpio_sifive_debug("registers values ---------------------------\n");
for (reg=GPIO_INPUT_VAL; reg<=GPIO_OUTPUT_XOR; reg+=4) {
value = readl(chip->base + reg);
gpio_sifive_debug("reg=[%02X], value=[%08X]\n", reg, value);
}
gpio_sifive_debug("irq_enable=[%08X]\n", chip->irq_enable);
gpio_sifive_debug("irq_type=%d\n", chip->irq_type[0]);
gpio_sifive_debug("irq_parent=%d\n", chip->irq_parent[0]);
gpio_sifive_debug("\n");
#endif
}

static void gpio_sifive_assign_bit(void __iomem *ptr, int offset, int value)
{
// It's frustrating that we are not allowed to use the device atomics
// which are GUARANTEED to be supported by this device on RISC-V
u32 bit = BIT(offset);
u32 old = readl(ptr);

bit = (value) ? old | bit : old & ~bit;
writel(bit, ptr);
}

static int gpio_sifive_direction_input(struct gpio_chip *gc, unsigned offset)
{
struct sifive_gpio *chip = gpiochip_get_data(gc);
unsigned long flags;

if (offset >= gc->ngpio)
return -EINVAL;

raw_spin_lock_irqsave(&chip->lock, flags);
gpio_sifive_assign_bit(chip->base + GPIO_OUTPUT_EN, offset, 0);
gpio_sifive_assign_bit(chip->base + GPIO_INPUT_EN, offset, 1);
raw_spin_unlock_irqrestore(&chip->lock, flags);

return 0;
}

static int gpio_sifive_direction_output(struct gpio_chip *gc, unsigned offset
, int value)
{
struct sifive_gpio *chip = gpiochip_get_data(gc);
unsigned long flags;

if (offset >= gc->ngpio)
return -EINVAL;

raw_spin_lock_irqsave(&chip->lock, flags);
gpio_sifive_assign_bit(chip->base + GPIO_INPUT_EN, offset, 0);
gpio_sifive_assign_bit(chip->base + GPIO_OUTPUT_EN, offset, 1);
gpio_sifive_assign_bit(chip->base + GPIO_OUTPUT_VAL, offset, value);
raw_spin_unlock_irqrestore(&chip->lock, flags);

return 0;
}

static int gpio_sifive_get_direction(struct gpio_chip *gc, unsigned offset)
{
struct sifive_gpio *chip = gpiochip_get_data(gc);
int value;

if (offset >= gc->ngpio)
return -EINVAL;

value = readl(chip->base + GPIO_OUTPUT_EN) & BIT(offset);
return !value;
}

static int gpio_sifive_get_value(struct gpio_chip *gc, unsigned offset)
{
struct sifive_gpio *chip = gpiochip_get_data(gc);
int index, value;

if (offset >= gc->ngpio)
return -EINVAL;

index = gpio_sifive_get_direction(gc, offset) ?
GPIO_INPUT_VAL : GPIO_OUTPUT_VAL;
value = readl(chip->base + index) & BIT(offset);
return value;
}

static void gpio_sifive_set_value(struct gpio_chip *gc, unsigned offset, int value)
{
struct sifive_gpio *chip = gpiochip_get_data(gc);
unsigned long flags;
int index;

if (offset >= gc->ngpio)
return;

index = gpio_sifive_get_direction(gc, offset) ?
GPIO_INPUT_VAL : GPIO_OUTPUT_VAL;
raw_spin_lock_irqsave(&chip->lock, flags);
gpio_sifive_assign_bit(chip->base + index, offset, value);
raw_spin_unlock_irqrestore(&chip->lock, flags);
}

static void gpio_sifive_set_default(struct sifive_gpio *chip, u32 bits)
{
if (bits == GPIO_ENABLE_BITS) {
//Input Enable/Disable
writel(bits, chip->base + GPIO_INPUT_EN);
return;
}
//Interrupts Enable/Disable
writel(bits, chip->base + GPIO_RISE_IE);
writel(bits, chip->base + GPIO_FALL_IE);
writel(bits, chip->base + GPIO_HIGH_IE);
writel(bits, chip->base + GPIO_LOW_IE);

writel(bits, chip->base + GPIO_RISE_IP);
writel(bits, chip->base + GPIO_FALL_IP);
writel(bits, chip->base + GPIO_HIGH_IP);
writel(bits, chip->base + GPIO_LOW_IP);

chip->irq_enable = bits;
}

static void gpio_sifive_set_ie(struct sifive_gpio *chip, int offset)
{
unsigned long flags;
unsigned irq_type;

raw_spin_lock_irqsave(&chip->lock, flags);
irq_type = (chip->irq_enable & BIT(offset)) ? chip->irq_type[offset] : 0;
gpio_sifive_assign_bit(chip->base + GPIO_RISE_IE, offset, irq_type & IRQ_TYPE_EDGE_RISING);
gpio_sifive_assign_bit(chip->base + GPIO_FALL_IE, offset, irq_type & IRQ_TYPE_EDGE_FALLING);
gpio_sifive_assign_bit(chip->base + GPIO_HIGH_IE, offset, irq_type & IRQ_TYPE_LEVEL_HIGH);
gpio_sifive_assign_bit(chip->base + GPIO_LOW_IE, offset, irq_type & IRQ_TYPE_LEVEL_LOW);
raw_spin_unlock_irqrestore(&chip->lock, flags);
}

static int gpio_sifive_irq_set_type(struct irq_data *d, unsigned irq_type)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct sifive_gpio *chip = gpiochip_get_data(gc);
int offset = irqd_to_hwirq(d);

if (offset < 0 || offset >= gc->ngpio)
return -EINVAL;

chip->irq_type[offset] = irq_type;
gpio_sifive_set_ie(chip, offset);

return 0;
}

/* chained_irq_{enter,exit} already mask the parent */
static void gpio_sifive_irq_mask(struct irq_data *d) { }
static void gpio_sifive_irq_unmask(struct irq_data *d) { }

static void gpio_sifive_irq_enable(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct sifive_gpio *chip = gpiochip_get_data(gc);
int offset = irqd_to_hwirq(d) % GPIO_MAX_CNT; // must not fail
u32 bit = BIT(offset);

/* Switch to input */
gpio_sifive_direction_input(gc, offset);

/* Clear any sticky pending interrupts */
writel(bit, chip->base + GPIO_RISE_IP);
writel(bit, chip->base + GPIO_FALL_IP);
writel(bit, chip->base + GPIO_HIGH_IP);
writel(bit, chip->base + GPIO_LOW_IP);

/* Enable interrupts */
chip->irq_enable |= bit;
gpio_sifive_set_ie(chip, offset);
}

static void gpio_sifive_irq_disable(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct sifive_gpio *chip = gpiochip_get_data(gc);
int offset = irqd_to_hwirq(d) % GPIO_MAX_CNT; // must not fail
u32 bit = BIT(offset);

/* Disable interrupts */
chip->irq_enable &= ~bit;
gpio_sifive_set_ie(chip, offset);
}

static struct irq_chip gpio_sifive_irqchip = {
.name = "sifive-gpio",
.irq_set_type = gpio_sifive_irq_set_type,
.irq_mask = gpio_sifive_irq_mask,
.irq_unmask = gpio_sifive_irq_unmask,
.irq_enable = gpio_sifive_irq_enable,
.irq_disable = gpio_sifive_irq_disable,
};

static void gpio_sifive_irq_handler(struct irq_desc *desc)
{
struct irq_chip *irqchip = irq_desc_get_chip(desc);
struct sifive_gpio **self_ptr = irq_desc_get_handler_data(desc);
struct sifive_gpio *chip = *self_ptr;
int offset = self_ptr - &chip->self_ptr[0];
//int offset = desc->irq_data.irq - chip->irq_parent[0];
u32 bit = BIT(offset);

chained_irq_enter(irqchip, desc);

/* Re-arm the edge irq_types so don't miss the next one */
writel(bit, chip->base + GPIO_RISE_IP);
writel(bit, chip->base + GPIO_FALL_IP);

generic_handle_irq(irq_find_mapping(chip->gc.irq.domain, offset));

/* Re-arm the level irq_types after handling to prevent spurious refire */
writel(bit, chip->base + GPIO_HIGH_IP);
writel(bit, chip->base + GPIO_LOW_IP);

chained_irq_exit(irqchip, desc);

gpio_sifive_debug("irq handler: offset=%d\n", offset);
}

#ifdef GPIO_SIFIVE_DEBUG
static void gpio_sifive_set_irq_enable(struct sifive_gpio *chip, unsigned offset)
{
u32 bit = BIT(offset);

/* Switch to input */
gpio_sifive_direction_input(&chip->gc, offset);

/* Clear any sticky pending interrupts */
writel(bit, chip->base + GPIO_RISE_IP);
writel(bit, chip->base + GPIO_FALL_IP);
writel(bit, chip->base + GPIO_HIGH_IP);
writel(bit, chip->base + GPIO_LOW_IP);

/* Enable interrupts */
chip->irq_enable |= bit;
gpio_sifive_set_ie(chip, offset);
}

static void gpio_sifive_set_irq_disable(struct sifive_gpio *chip, unsigned offset)
{
u32 bit = BIT(offset);
chip->irq_enable &= ~bit;
gpio_sifive_set_ie(chip, offset);
}
#endif

static int gpio_sifive_chip_setup(struct platform_device *pdev
, struct sifive_gpio *chip, int ngpio)
{
struct device *dev = &pdev->dev;
int gpio, irq, ret;

raw_spin_lock_init(&chip->lock);
chip->gc.direction_input = gpio_sifive_direction_input;
chip->gc.direction_output = gpio_sifive_direction_output;
chip->gc.get_direction = gpio_sifive_get_direction;
chip->gc.get = gpio_sifive_get_value;
chip->gc.set = gpio_sifive_set_value;
chip->gc.base = -1;
chip->gc.ngpio = ngpio;
chip->gc.label = dev_name(dev);
chip->gc.parent = dev;
chip->gc.owner = THIS_MODULE;

ret = gpiochip_add_data(&chip->gc, chip);
if (ret)
return ret;

/* Disable all GPIO interrupts before enabling parent interrupts */
gpio_sifive_set_default(chip, 0);

ret = gpiochip_irqchip_add(&chip->gc, &gpio_sifive_irqchip, 0
, handle_simple_irq, IRQ_TYPE_NONE);
if (ret) {
dev_err(dev, "GPIO: could not add irqchip\n");
gpiochip_remove(&chip->gc);
return ret;
}

chip->gc.irq.num_parents = ngpio;
chip->gc.irq.parents = &chip->irq_parent[0];
chip->gc.irq.map = &chip->irq_parent[0];

for (gpio = 0; gpio < ngpio; ++gpio) {
irq = platform_get_irq(pdev, gpio);
if (irq < 0) {
dev_err(dev, "GPIO: invalid IRQ\n");
gpiochip_remove(&chip->gc);
return -ENODEV;
}
chip->self_ptr[gpio] = chip;
chip->irq_parent[gpio] = irq;
chip->irq_type[gpio] = IRQ_TYPE_LEVEL_HIGH;
}
for (gpio = 0; gpio < ngpio; ++gpio) {
irq = chip->irq_parent[gpio];
irq_set_chained_handler_and_data(irq, gpio_sifive_irq_handler
, &chip->self_ptr[gpio]);
irq_set_parent(irq_find_mapping(chip->gc.irq.domain, gpio), irq);
}

//Enable GPIO Input for default
gpio_sifive_set_default(chip, GPIO_ENABLE_BITS);
return 0;
}

static int gpio_sifive_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *node = pdev->dev.of_node;
struct sifive_gpio *chip;
struct resource *res;
int ngpio;

chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip) {
dev_err(dev, "out of memory\n");
return -ENOMEM;
}

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
chip->base = devm_ioremap_resource(dev, res);
if (IS_ERR(chip->base)) {
dev_err(dev, "failed to allocate device memory\n");
return PTR_ERR(chip->base);
}
gpio_sifive_debug_reg(chip);

if(of_property_read_u32(node, "ngpios", &ngpio))
ngpio = of_irq_count(node);

if (ngpio >= GPIO_MAX_CNT) {
dev_err(dev, "too many ngpios.\n");
return -EINVAL;
}

if (gpio_sifive_chip_setup(pdev, chip, ngpio) < 0) {
dev_err(dev, "failed to gpio sifive setup.\n");
return -EINVAL;
}

platform_set_drvdata(pdev, chip);
dev_info(dev, "GPIO SiFive driver registered %d GPIOs\n", ngpio);

#ifdef GPIO_SIFIVE_DEBUG
gpio_sifive_set_irq_enable(chip, 7); ///GPIO7 interrupt enabled for test
gpio_sifive_set_irq_disable(chip, 9); ///GPIO9 interrupt disabled for test
#endif
gpio_sifive_debug_reg(chip);
return 0;
}

static const struct of_device_id gpio_sifive_match[] = {
{
.compatible = "sifive,gpio0",
},
{ },
};
MODULE_DEVICE_TABLE(of, gpio_sifive_match);

static struct platform_driver gpio_sifive_driver = {
.probe = gpio_sifive_probe,
.driver = {
.name = "gpio-sifive",
.of_match_table = of_match_ptr(gpio_sifive_match),
},
};
module_platform_driver(gpio_sifive_driver);

MODULE_DESCRIPTION("SiFive GPIO driver");
MODULE_LICENSE("GPL v2");

Next message: Dan Williams: "Re: [PATCH][RFC] PCI: Add "pci=blacklist_dev=" parameter to blacklist specific devices"
Previous message: Stephen Rothwell: "Re: linux-next: manual merge of the selinux tree with the keys tree"
In reply to: Marc Zyngier: "Re: [PATCH] gpio/sifive: fix static checker warning"
Next in thread: Marc Zyngier: "Re: [PATCH] gpio/sifive: fix static checker warning"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]