| Message ID | 20191009165056.76580-3-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] intel-gpio for 5.4-2 | expand |
Hi, On 09-10-2019 18:50, Andy Shevchenko wrote: > After changing the drivers to use GPIO core to add an IRQ chip > it appears that some of them requires a hardware initialization > before adding the IRQ chip. > > Add an optional callback ->init_hw() to allow that drivers > to initialize hardware if needed. > > This change is a part of the fix NULL pointer dereference > brought to the several drivers recently. > > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Hmm, IIRC Linus Walleij already added a callback for initializing the mask before the irchip gets initialized which is basically intended for what you want this callback for I think ? Regards. Hans > --- > drivers/gpio/gpiolib.c | 22 +++++++++++++++++++++- > include/linux/gpio/driver.h | 8 ++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index bdbc1649eafa..85601ad4fcd5 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -86,6 +86,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > struct lock_class_key *lock_key, > struct lock_class_key *request_key); > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); > +static int gpiochip_irqchip_init_hw(struct gpio_chip *gpiochip); > static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip); > static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip); > > @@ -1406,6 +1407,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, > > machine_gpiochip_add(chip); > > + ret = gpiochip_irqchip_init_hw(chip); > + if (ret) > + goto err_remove_acpi_chip; > + > ret = gpiochip_irqchip_init_valid_mask(chip); > if (ret) > goto err_remove_acpi_chip; > @@ -1622,6 +1627,16 @@ static struct gpio_chip *find_chip_by_name(const char *name) > * The following is irqchip helper code for gpiochips. > */ > > +static int gpiochip_irqchip_init_hw(struct gpio_chip *gc) > +{ > + struct gpio_irq_chip *girq = &gc->irq; > + > + if (!girq->init_hw) > + return 0; > + > + return girq->init_hw(gc); > +} > + > static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gc) > { > struct gpio_irq_chip *girq = &gc->irq; > @@ -2446,8 +2461,13 @@ static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > { > return 0; > } > - > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) {} > + > +static inline int gpiochip_irqchip_init_hw(struct gpio_chip *gpiochip) > +{ > + return 0; > +} > + > static inline int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip) > { > return 0; > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index f8245d67f070..5dd9c982e2cb 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -201,6 +201,14 @@ struct gpio_irq_chip { > */ > bool threaded; > > + /** > + * @init_hw: optional routine to initialize hardware before > + * an IRQ chip will be added. This is quite useful when > + * a particular driver wants to clear IRQ related registers > + * in order to avoid undesired events. > + */ > + int (*init_hw)(struct gpio_chip *chip); > + > /** > * @init_valid_mask: optional routine to initialize @valid_mask, to be > * used if not all GPIO lines are valid interrupts. Sometimes some >
On Wed, Oct 09, 2019 at 09:44:31PM +0200, Hans de Goede wrote: > On 09-10-2019 18:50, Andy Shevchenko wrote: > > After changing the drivers to use GPIO core to add an IRQ chip > > it appears that some of them requires a hardware initialization > > before adding the IRQ chip. > > > > Add an optional callback ->init_hw() to allow that drivers > > to initialize hardware if needed. > > > > This change is a part of the fix NULL pointer dereference > > brought to the several drivers recently. > > > > Cc: Hans de Goede <hdegoede@redhat.com> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Hmm, IIRC Linus Walleij already added a callback for initializing the > mask before the irchip gets initialized which is basically intended for > what you want this callback for I think ? This is not about the mask, it's about hardware to be prepared before enabling. Also init_valid_mask() will allocate memory which won't be needed.
On Thu, Oct 10, 2019 at 10:23:04AM +0300, Andy Shevchenko wrote: > On Wed, Oct 09, 2019 at 09:44:31PM +0200, Hans de Goede wrote: > > On 09-10-2019 18:50, Andy Shevchenko wrote: > > > After changing the drivers to use GPIO core to add an IRQ chip > > > it appears that some of them requires a hardware initialization > > > before adding the IRQ chip. > > > > > > Add an optional callback ->init_hw() to allow that drivers > > > to initialize hardware if needed. > > > > > > This change is a part of the fix NULL pointer dereference > > > brought to the several drivers recently. > > > > > > Cc: Hans de Goede <hdegoede@redhat.com> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Hmm, IIRC Linus Walleij already added a callback for initializing the > > mask before the irchip gets initialized which is basically intended for > > what you want this callback for I think ? > > This is not about the mask, it's about hardware to be prepared before enabling. > Also init_valid_mask() will allocate memory which won't be needed. If you think this is not a proper approach, we have to revert all three patches now (*) and think about better solution. *) They broke the boot on all affected machines.
Hi, On 10-10-2019 09:26, Andy Shevchenko wrote: > On Thu, Oct 10, 2019 at 10:23:04AM +0300, Andy Shevchenko wrote: >> On Wed, Oct 09, 2019 at 09:44:31PM +0200, Hans de Goede wrote: >>> On 09-10-2019 18:50, Andy Shevchenko wrote: >>>> After changing the drivers to use GPIO core to add an IRQ chip >>>> it appears that some of them requires a hardware initialization >>>> before adding the IRQ chip. >>>> >>>> Add an optional callback ->init_hw() to allow that drivers >>>> to initialize hardware if needed. >>>> >>>> This change is a part of the fix NULL pointer dereference >>>> brought to the several drivers recently. >>>> >>>> Cc: Hans de Goede <hdegoede@redhat.com> >>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> >>> Hmm, IIRC Linus Walleij already added a callback for initializing the >>> mask before the irchip gets initialized which is basically intended for >>> what you want this callback for I think ? >> >> This is not about the mask, it's about hardware to be prepared before enabling. >> Also init_valid_mask() will allocate memory which won't be needed. > > If you think this is not a proper approach, we have to revert all three patches > now (*) and think about better solution. If this is not about the valid-mask then this approach is probably fine, sorry for the confusion. Lets wait and see what Linus Walleij has to say. Regards, Hans
On Wed, Oct 9, 2019 at 6:51 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > After changing the drivers to use GPIO core to add an IRQ chip > it appears that some of them requires a hardware initialization > before adding the IRQ chip. > > Add an optional callback ->init_hw() to allow that drivers > to initialize hardware if needed. > > This change is a part of the fix NULL pointer dereference > brought to the several drivers recently. > > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Ouch! But a very nice fix. I will pull in the series. Thanks for fixing this up so nicely Andy! Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index bdbc1649eafa..85601ad4fcd5 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -86,6 +86,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, struct lock_class_key *lock_key, struct lock_class_key *request_key); static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); +static int gpiochip_irqchip_init_hw(struct gpio_chip *gpiochip); static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip); static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip); @@ -1406,6 +1407,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, machine_gpiochip_add(chip); + ret = gpiochip_irqchip_init_hw(chip); + if (ret) + goto err_remove_acpi_chip; + ret = gpiochip_irqchip_init_valid_mask(chip); if (ret) goto err_remove_acpi_chip; @@ -1622,6 +1627,16 @@ static struct gpio_chip *find_chip_by_name(const char *name) * The following is irqchip helper code for gpiochips. */ +static int gpiochip_irqchip_init_hw(struct gpio_chip *gc) +{ + struct gpio_irq_chip *girq = &gc->irq; + + if (!girq->init_hw) + return 0; + + return girq->init_hw(gc); +} + static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gc) { struct gpio_irq_chip *girq = &gc->irq; @@ -2446,8 +2461,13 @@ static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip, { return 0; } - static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) {} + +static inline int gpiochip_irqchip_init_hw(struct gpio_chip *gpiochip) +{ + return 0; +} + static inline int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip) { return 0; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index f8245d67f070..5dd9c982e2cb 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -201,6 +201,14 @@ struct gpio_irq_chip { */ bool threaded; + /** + * @init_hw: optional routine to initialize hardware before + * an IRQ chip will be added. This is quite useful when + * a particular driver wants to clear IRQ related registers + * in order to avoid undesired events. + */ + int (*init_hw)(struct gpio_chip *chip); + /** * @init_valid_mask: optional routine to initialize @valid_mask, to be * used if not all GPIO lines are valid interrupts. Sometimes some
After changing the drivers to use GPIO core to add an IRQ chip it appears that some of them requires a hardware initialization before adding the IRQ chip. Add an optional callback ->init_hw() to allow that drivers to initialize hardware if needed. This change is a part of the fix NULL pointer dereference brought to the several drivers recently. Cc: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib.c | 22 +++++++++++++++++++++- include/linux/gpio/driver.h | 8 ++++++++ 2 files changed, 29 insertions(+), 1 deletion(-)