| Message ID | 20220607103302.37558-1-stefan.wahren@i2se.com |
|---|---|
| State | Accepted |
| Commit | 08752e0749ba3c3d830d1899ea4ca8cf1980d584 |
| Headers | show |
| Series | [RFC] pinctrl: bcm2835: Make the irqchip immutable | expand |
On 2022-06-07 11:33, Stefan Wahren wrote: > Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as > immutable") added a warning to indicate if the gpiolib is altering the > internals of irqchips. The bcm2835 pinctrl is also affected by this > warning. > > Fix this by making the irqchip in the bcm2835 pinctrl driver immutable. > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > > Hi, not sure about this change because irq_mask/unmask also uses the > enable/disable callbacks. Yeah, that's totally broken. If you have both enable and mask, they *must* be doing something different (the names are pretty unambiguous...). If they do the same thing, then enable/disable has to go. > > drivers/pinctrl/bcm/pinctrl-bcm2835.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > index dad453054776..f754f7ed9eb9 100644 > --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > @@ -516,6 +516,8 @@ static void bcm2835_gpio_irq_enable(struct irq_data > *data) > unsigned bank = GPIO_REG_OFFSET(gpio); > unsigned long flags; > > + gpiochip_enable_irq(chip, gpio); > + > raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); > set_bit(offset, &pc->enabled_irq_map[bank]); > bcm2835_gpio_irq_config(pc, gpio, true); > @@ -537,6 +539,8 @@ static void bcm2835_gpio_irq_disable(struct > irq_data *data) > bcm2835_gpio_set_bit(pc, GPEDS0, gpio); > clear_bit(offset, &pc->enabled_irq_map[bank]); > raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); > + > + gpiochip_disable_irq(chip, gpio); > } > > static int __bcm2835_gpio_irq_set_type_disabled(struct bcm2835_pinctrl > *pc, > @@ -693,7 +697,7 @@ static int bcm2835_gpio_irq_set_wake(struct > irq_data *data, unsigned int on) > return ret; > } > > -static struct irq_chip bcm2835_gpio_irq_chip = { > +static const struct irq_chip bcm2835_gpio_irq_chip = { > .name = MODULE_NAME, > .irq_enable = bcm2835_gpio_irq_enable, > .irq_disable = bcm2835_gpio_irq_disable, > @@ -702,7 +706,7 @@ static struct irq_chip bcm2835_gpio_irq_chip = { > .irq_mask = bcm2835_gpio_irq_disable, > .irq_unmask = bcm2835_gpio_irq_enable, > .irq_set_wake = bcm2835_gpio_irq_set_wake, > - .flags = IRQCHIP_MASK_ON_SUSPEND, > + .flags = (IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE), You are missing the callbacks into the resource management code (GPIOCHIP_IRQ_RESOURCE_HELPERS). Thanks, M.
Hi Marc, Am 10.06.22 um 10:51 schrieb Marc Zyngier: > On 2022-06-07 11:33, Stefan Wahren wrote: >> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as >> immutable") added a warning to indicate if the gpiolib is altering the >> internals of irqchips. The bcm2835 pinctrl is also affected by this >> warning. >> >> Fix this by making the irqchip in the bcm2835 pinctrl driver immutable. >> >> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >> --- >> >> Hi, not sure about this change because irq_mask/unmask also uses the >> enable/disable callbacks. > > Yeah, that's totally broken. > > If you have both enable and mask, they *must* be doing something > different (the names are pretty unambiguous...). If they do the > same thing, then enable/disable has to go. i will send a seperate fix for this. > >> >> drivers/pinctrl/bcm/pinctrl-bcm2835.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> b/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> index dad453054776..f754f7ed9eb9 100644 >> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c >> @@ -516,6 +516,8 @@ static void bcm2835_gpio_irq_enable(struct >> irq_data *data) >> unsigned bank = GPIO_REG_OFFSET(gpio); >> unsigned long flags; >> >> + gpiochip_enable_irq(chip, gpio); >> + >> raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); >> set_bit(offset, &pc->enabled_irq_map[bank]); >> bcm2835_gpio_irq_config(pc, gpio, true); >> @@ -537,6 +539,8 @@ static void bcm2835_gpio_irq_disable(struct >> irq_data *data) >> bcm2835_gpio_set_bit(pc, GPEDS0, gpio); >> clear_bit(offset, &pc->enabled_irq_map[bank]); >> raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); >> + >> + gpiochip_disable_irq(chip, gpio); >> } >> >> static int __bcm2835_gpio_irq_set_type_disabled(struct >> bcm2835_pinctrl *pc, >> @@ -693,7 +697,7 @@ static int bcm2835_gpio_irq_set_wake(struct >> irq_data *data, unsigned int on) >> return ret; >> } >> >> -static struct irq_chip bcm2835_gpio_irq_chip = { >> +static const struct irq_chip bcm2835_gpio_irq_chip = { >> .name = MODULE_NAME, >> .irq_enable = bcm2835_gpio_irq_enable, >> .irq_disable = bcm2835_gpio_irq_disable, >> @@ -702,7 +706,7 @@ static struct irq_chip bcm2835_gpio_irq_chip = { >> .irq_mask = bcm2835_gpio_irq_disable, >> .irq_unmask = bcm2835_gpio_irq_enable, >> .irq_set_wake = bcm2835_gpio_irq_set_wake, >> - .flags = IRQCHIP_MASK_ON_SUSPEND, >> + .flags = (IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE), > > You are missing the callbacks into the resource management > code (GPIOCHIP_IRQ_RESOURCE_HELPERS). Thanks > > Thanks, > > M.
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index dad453054776..f754f7ed9eb9 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -516,6 +516,8 @@ static void bcm2835_gpio_irq_enable(struct irq_data *data) unsigned bank = GPIO_REG_OFFSET(gpio); unsigned long flags; + gpiochip_enable_irq(chip, gpio); + raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); set_bit(offset, &pc->enabled_irq_map[bank]); bcm2835_gpio_irq_config(pc, gpio, true); @@ -537,6 +539,8 @@ static void bcm2835_gpio_irq_disable(struct irq_data *data) bcm2835_gpio_set_bit(pc, GPEDS0, gpio); clear_bit(offset, &pc->enabled_irq_map[bank]); raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); + + gpiochip_disable_irq(chip, gpio); } static int __bcm2835_gpio_irq_set_type_disabled(struct bcm2835_pinctrl *pc, @@ -693,7 +697,7 @@ static int bcm2835_gpio_irq_set_wake(struct irq_data *data, unsigned int on) return ret; } -static struct irq_chip bcm2835_gpio_irq_chip = { +static const struct irq_chip bcm2835_gpio_irq_chip = { .name = MODULE_NAME, .irq_enable = bcm2835_gpio_irq_enable, .irq_disable = bcm2835_gpio_irq_disable, @@ -702,7 +706,7 @@ static struct irq_chip bcm2835_gpio_irq_chip = { .irq_mask = bcm2835_gpio_irq_disable, .irq_unmask = bcm2835_gpio_irq_enable, .irq_set_wake = bcm2835_gpio_irq_set_wake, - .flags = IRQCHIP_MASK_ON_SUSPEND, + .flags = (IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE), }; static int bcm2835_pctl_get_groups_count(struct pinctrl_dev *pctldev) @@ -1280,7 +1284,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range); girq = &pc->gpio_chip.irq; - girq->chip = &bcm2835_gpio_irq_chip; + gpio_irq_chip_set_chip(girq, &bcm2835_gpio_irq_chip); girq->parent_handler = bcm2835_gpio_irq_handler; girq->num_parents = BCM2835_NUM_IRQS; girq->parents = devm_kcalloc(dev, BCM2835_NUM_IRQS,
Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as immutable") added a warning to indicate if the gpiolib is altering the internals of irqchips. The bcm2835 pinctrl is also affected by this warning. Fix this by making the irqchip in the bcm2835 pinctrl driver immutable. Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- Hi, not sure about this change because irq_mask/unmask also uses the enable/disable callbacks. drivers/pinctrl/bcm/pinctrl-bcm2835.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)