| Message ID | 77d635877e765a6b4b0935debfe5836e554572c0.1489015238.git.julia@ni.com |
|---|---|
| State | New |
| Headers | show |
Hi Julia/Linus, On 3/9/2017 8:21 AM, Julia Cartwright wrote: > The bcm-kona gpio driver currently implements an irq_chip for handling > interrupts; due to how irq_chip handling is done, it's necessary for the > irq_chip methods to be invoked from hardirq context, even on a a > real-time kernel. Because the spinlock_t type becomes a "sleeping" > spinlock w/ RT kernels, it is not suitable to be used with irq_chips. > > A quick audit of the operations under the lock reveal that they do only > minimal, bounded work, and are therefore safe to do under a raw spinlock. This is new to me. But it seems like, for the vast majority cases, user can still continue to use spin_lock as it is without needing to worry about the underlying difference between standard or RT kernels. But in certain cases, e.g., irq_chips, extra care needs to be done, i.e., switching to use raw spin lock to make sure that it is not blocking in the case of RT. Is such API use change well accepted by the open source community already? Thanks, Ray > > Signed-off-by: Julia Cartwright <julia@ni.com> > --- > drivers/gpio/gpio-bcm-kona.c | 48 ++++++++++++++++++++++---------------------- > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c > index 41d0ac142580..dfcf56ee3c61 100644 > --- a/drivers/gpio/gpio-bcm-kona.c > +++ b/drivers/gpio/gpio-bcm-kona.c > @@ -67,7 +67,7 @@ > struct bcm_kona_gpio { > void __iomem *reg_base; > int num_bank; > - spinlock_t lock; > + raw_spinlock_t lock; > struct gpio_chip gpio_chip; > struct irq_domain *irq_domain; > struct bcm_kona_gpio_bank *banks; > @@ -95,13 +95,13 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio, > unsigned long flags; > int bank_id = GPIO_BANK(gpio); > > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); > val |= BIT(gpio); > bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > } > > static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, > @@ -111,13 +111,13 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, > unsigned long flags; > int bank_id = GPIO_BANK(gpio); > > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); > val &= ~BIT(gpio); > bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > } > > static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio) > @@ -141,7 +141,7 @@ static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value) > > kona_gpio = gpiochip_get_data(chip); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > /* this function only applies to output pin */ > if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN) > @@ -154,7 +154,7 @@ static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value) > writel(val, reg_base + reg_offset); > > out: > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > } > > static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio) > @@ -168,7 +168,7 @@ static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio) > > kona_gpio = gpiochip_get_data(chip); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN) > reg_offset = GPIO_IN_STATUS(bank_id); > @@ -178,7 +178,7 @@ static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio) > /* read the GPIO bank status */ > val = readl(reg_base + reg_offset); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > > /* return the specified bit status */ > return !!(val & BIT(bit)); > @@ -208,14 +208,14 @@ static int bcm_kona_gpio_direction_input(struct gpio_chip *chip, unsigned gpio) > > kona_gpio = gpiochip_get_data(chip); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_CONTROL(gpio)); > val &= ~GPIO_GPCTR0_IOTR_MASK; > val |= GPIO_GPCTR0_IOTR_CMD_INPUT; > writel(val, reg_base + GPIO_CONTROL(gpio)); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > > return 0; > } > @@ -232,7 +232,7 @@ static int bcm_kona_gpio_direction_output(struct gpio_chip *chip, > > kona_gpio = gpiochip_get_data(chip); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_CONTROL(gpio)); > val &= ~GPIO_GPCTR0_IOTR_MASK; > @@ -244,7 +244,7 @@ static int bcm_kona_gpio_direction_output(struct gpio_chip *chip, > val |= BIT(bit); > writel(val, reg_base + reg_offset); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > > return 0; > } > @@ -288,7 +288,7 @@ static int bcm_kona_gpio_set_debounce(struct gpio_chip *chip, unsigned gpio, > } > > /* spin lock for read-modify-write of the GPIO register */ > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_CONTROL(gpio)); > val &= ~GPIO_GPCTR0_DBR_MASK; > @@ -303,7 +303,7 @@ static int bcm_kona_gpio_set_debounce(struct gpio_chip *chip, unsigned gpio, > > writel(val, reg_base + GPIO_CONTROL(gpio)); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > > return 0; > } > @@ -347,13 +347,13 @@ static void bcm_kona_gpio_irq_ack(struct irq_data *d) > > kona_gpio = irq_data_get_irq_chip_data(d); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_INT_STATUS(bank_id)); > val |= BIT(bit); > writel(val, reg_base + GPIO_INT_STATUS(bank_id)); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > } > > static void bcm_kona_gpio_irq_mask(struct irq_data *d) > @@ -368,13 +368,13 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d) > > kona_gpio = irq_data_get_irq_chip_data(d); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_INT_MASK(bank_id)); > val |= BIT(bit); > writel(val, reg_base + GPIO_INT_MASK(bank_id)); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > } > > static void bcm_kona_gpio_irq_unmask(struct irq_data *d) > @@ -389,13 +389,13 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d) > > kona_gpio = irq_data_get_irq_chip_data(d); > reg_base = kona_gpio->reg_base; > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_INT_MSKCLR(bank_id)); > val |= BIT(bit); > writel(val, reg_base + GPIO_INT_MSKCLR(bank_id)); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > } > > static int bcm_kona_gpio_irq_set_type(struct irq_data *d, unsigned int type) > @@ -431,14 +431,14 @@ static int bcm_kona_gpio_irq_set_type(struct irq_data *d, unsigned int type) > return -EINVAL; > } > > - spin_lock_irqsave(&kona_gpio->lock, flags); > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_CONTROL(gpio)); > val &= ~GPIO_GPCTR0_ITR_MASK; > val |= lvl_type << GPIO_GPCTR0_ITR_SHIFT; > writel(val, reg_base + GPIO_CONTROL(gpio)); > > - spin_unlock_irqrestore(&kona_gpio->lock, flags); > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > > return 0; > } > @@ -655,7 +655,7 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) > bank); > } > > - spin_lock_init(&kona_gpio->lock); > + raw_spin_lock_init(&kona_gpio->lock); > > return 0; > > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 10, 2017 at 09:28:08AM -0800, Ray Jui wrote: > Hi Julia/Linus, > > On 3/9/2017 8:21 AM, Julia Cartwright wrote: > > The bcm-kona gpio driver currently implements an irq_chip for handling > > interrupts; due to how irq_chip handling is done, it's necessary for the > > irq_chip methods to be invoked from hardirq context, even on a a > > real-time kernel. Because the spinlock_t type becomes a "sleeping" > > spinlock w/ RT kernels, it is not suitable to be used with irq_chips. > > > > A quick audit of the operations under the lock reveal that they do only > > minimal, bounded work, and are therefore safe to do under a raw spinlock. > > This is new to me. But it seems like, for the vast majority cases, user > can still continue to use spin_lock as it is without needing to worry > about the underlying difference between standard or RT kernels. If by "user" you mean, "driver developer", then yes. For most driver authors, the distinction between raw and non-raw spinlocks is irrelevant, they can use spinlocks and everything will work out just fine w/ mainline and on RT. > But in certain cases, e.g., irq_chips, extra care needs to be done, > i.e., swtching to use raw spin lock to make sure that it is not > blocking in the case of RT. Correct, on RT the goal is to push as much as possible into a preemptible context, including driver interrupts, etc. However, there are still codepaths which necessarily need to be executed in hardirq context, including anything necessary to support scheduling. This includes: interrupt-dispatching (irq_chips), timers, and the scheduler itself, which is why this "core" code must use the raw spinlock variants. > Is such API use change well accepted by the open source community > already? In what way is this an API change? The API isn't changing, what's changing in this patch is to fix what is an irq_chip implementation which is _currently broken_ on RT. Julia
On Thu, Mar 9, 2017 at 5:21 PM, Julia Cartwright <julia@ni.com> wrote: > The bcm-kona gpio driver currently implements an irq_chip for handling > interrupts; due to how irq_chip handling is done, it's necessary for the > irq_chip methods to be invoked from hardirq context, even on a a > real-time kernel. Because the spinlock_t type becomes a "sleeping" > spinlock w/ RT kernels, it is not suitable to be used with irq_chips. > > A quick audit of the operations under the lock reveal that they do only > minimal, bounded work, and are therefore safe to do under a raw spinlock. > > Signed-off-by: Julia Cartwright <julia@ni.com> Patch applied. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 10, 2017 at 6:28 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> Is such API use change well accepted by the open source community already?
The raw_*spinlock APIs are in the kernel header files and used at several
sites in the kernel, so yes.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c index 41d0ac142580..dfcf56ee3c61 100644 --- a/drivers/gpio/gpio-bcm-kona.c +++ b/drivers/gpio/gpio-bcm-kona.c @@ -67,7 +67,7 @@ struct bcm_kona_gpio { void __iomem *reg_base; int num_bank; - spinlock_t lock; + raw_spinlock_t lock; struct gpio_chip gpio_chip; struct irq_domain *irq_domain; struct bcm_kona_gpio_bank *banks; @@ -95,13 +95,13 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio, unsigned long flags; int bank_id = GPIO_BANK(gpio); - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); val |= BIT(gpio); bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); } static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, @@ -111,13 +111,13 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, unsigned long flags; int bank_id = GPIO_BANK(gpio); - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); val &= ~BIT(gpio); bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); } static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio) @@ -141,7 +141,7 @@ static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value) kona_gpio = gpiochip_get_data(chip); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); /* this function only applies to output pin */ if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN) @@ -154,7 +154,7 @@ static void bcm_kona_gpio_set(struct gpio_chip *chip, unsigned gpio, int value) writel(val, reg_base + reg_offset); out: - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); } static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio) @@ -168,7 +168,7 @@ static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio) kona_gpio = gpiochip_get_data(chip); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); if (bcm_kona_gpio_get_dir(chip, gpio) == GPIOF_DIR_IN) reg_offset = GPIO_IN_STATUS(bank_id); @@ -178,7 +178,7 @@ static int bcm_kona_gpio_get(struct gpio_chip *chip, unsigned gpio) /* read the GPIO bank status */ val = readl(reg_base + reg_offset); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); /* return the specified bit status */ return !!(val & BIT(bit)); @@ -208,14 +208,14 @@ static int bcm_kona_gpio_direction_input(struct gpio_chip *chip, unsigned gpio) kona_gpio = gpiochip_get_data(chip); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_CONTROL(gpio)); val &= ~GPIO_GPCTR0_IOTR_MASK; val |= GPIO_GPCTR0_IOTR_CMD_INPUT; writel(val, reg_base + GPIO_CONTROL(gpio)); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); return 0; } @@ -232,7 +232,7 @@ static int bcm_kona_gpio_direction_output(struct gpio_chip *chip, kona_gpio = gpiochip_get_data(chip); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_CONTROL(gpio)); val &= ~GPIO_GPCTR0_IOTR_MASK; @@ -244,7 +244,7 @@ static int bcm_kona_gpio_direction_output(struct gpio_chip *chip, val |= BIT(bit); writel(val, reg_base + reg_offset); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); return 0; } @@ -288,7 +288,7 @@ static int bcm_kona_gpio_set_debounce(struct gpio_chip *chip, unsigned gpio, } /* spin lock for read-modify-write of the GPIO register */ - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_CONTROL(gpio)); val &= ~GPIO_GPCTR0_DBR_MASK; @@ -303,7 +303,7 @@ static int bcm_kona_gpio_set_debounce(struct gpio_chip *chip, unsigned gpio, writel(val, reg_base + GPIO_CONTROL(gpio)); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); return 0; } @@ -347,13 +347,13 @@ static void bcm_kona_gpio_irq_ack(struct irq_data *d) kona_gpio = irq_data_get_irq_chip_data(d); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_INT_STATUS(bank_id)); val |= BIT(bit); writel(val, reg_base + GPIO_INT_STATUS(bank_id)); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); } static void bcm_kona_gpio_irq_mask(struct irq_data *d) @@ -368,13 +368,13 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d) kona_gpio = irq_data_get_irq_chip_data(d); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_INT_MASK(bank_id)); val |= BIT(bit); writel(val, reg_base + GPIO_INT_MASK(bank_id)); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); } static void bcm_kona_gpio_irq_unmask(struct irq_data *d) @@ -389,13 +389,13 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d) kona_gpio = irq_data_get_irq_chip_data(d); reg_base = kona_gpio->reg_base; - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_INT_MSKCLR(bank_id)); val |= BIT(bit); writel(val, reg_base + GPIO_INT_MSKCLR(bank_id)); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); } static int bcm_kona_gpio_irq_set_type(struct irq_data *d, unsigned int type) @@ -431,14 +431,14 @@ static int bcm_kona_gpio_irq_set_type(struct irq_data *d, unsigned int type) return -EINVAL; } - spin_lock_irqsave(&kona_gpio->lock, flags); + raw_spin_lock_irqsave(&kona_gpio->lock, flags); val = readl(reg_base + GPIO_CONTROL(gpio)); val &= ~GPIO_GPCTR0_ITR_MASK; val |= lvl_type << GPIO_GPCTR0_ITR_SHIFT; writel(val, reg_base + GPIO_CONTROL(gpio)); - spin_unlock_irqrestore(&kona_gpio->lock, flags); + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); return 0; } @@ -655,7 +655,7 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) bank); } - spin_lock_init(&kona_gpio->lock); + raw_spin_lock_init(&kona_gpio->lock); return 0;
The bcm-kona gpio driver currently implements an irq_chip for handling interrupts; due to how irq_chip handling is done, it's necessary for the irq_chip methods to be invoked from hardirq context, even on a a real-time kernel. Because the spinlock_t type becomes a "sleeping" spinlock w/ RT kernels, it is not suitable to be used with irq_chips. A quick audit of the operations under the lock reveal that they do only minimal, bounded work, and are therefore safe to do under a raw spinlock. Signed-off-by: Julia Cartwright <julia@ni.com> --- drivers/gpio/gpio-bcm-kona.c | 48 ++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-)