* [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback @ 2019-10-25 14:06 Andy Shevchenko 2019-10-25 16:07 ` Andy Shevchenko ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Andy Shevchenko @ 2019-10-25 14:06 UTC (permalink / raw) To: Linus Walleij, linux-gpio, Mika Westerberg, hdegoede; +Cc: Andy Shevchenko There is a logical continuation of the commit 5fbe5b5883f8 ("gpio: Initialize the irqchip valid_mask with a callback") to split IRQ initialization to hardware and valid mask setup parts. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pinctrl/intel/pinctrl-baytrail.c | 25 ++++++++++-------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index beb26550c25f..08e2b940cc11 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -1432,22 +1432,10 @@ static void byt_init_irq_valid_mask(struct gpio_chip *chip, unsigned long *valid_mask, unsigned int ngpios) { - /* - * FIXME: currently the valid_mask is filled in as part of - * initializing the irq_chip below in byt_gpio_irq_init_hw(). - * when converting this driver to the new way of passing the - * gpio_irq_chip along when adding the gpio_chip, move the - * mask initialization into this callback instead. Right now - * this callback is here to make sure the mask gets allocated. - */ -} - -static int byt_gpio_irq_init_hw(struct gpio_chip *gc) -{ - struct byt_gpio *vg = gpiochip_get_data(gc); + struct byt_gpio *vg = gpiochip_get_data(chip); struct device *dev = &vg->pdev->dev; void __iomem *reg; - u32 base, value; + u32 value; int i; /* @@ -1468,13 +1456,20 @@ static int byt_gpio_irq_init_hw(struct gpio_chip *gc) value = readl(reg); if (value & BYT_DIRECT_IRQ_EN) { - clear_bit(i, gc->irq.valid_mask); + clear_bit(i, valid_mask); dev_dbg(dev, "excluding GPIO %d from IRQ domain\n", i); } else if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i)) { byt_gpio_clear_triggering(vg, i); dev_dbg(dev, "disabling GPIO %d\n", i); } } +} + +static int byt_gpio_irq_init_hw(struct gpio_chip *gc) +{ + struct byt_gpio *vg = gpiochip_get_data(gc); + void __iomem *reg; + u32 base, value; /* clear interrupt status trigger registers */ for (base = 0; base < vg->soc_data->npins; base += 32) { -- 2.23.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-25 14:06 [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback Andy Shevchenko @ 2019-10-25 16:07 ` Andy Shevchenko 2019-10-28 11:29 ` Mika Westerberg 2019-10-30 9:30 ` Hans de Goede 2 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2019-10-25 16:07 UTC (permalink / raw) To: Linus Walleij, linux-gpio, Mika Westerberg, hdegoede On Fri, Oct 25, 2019 at 05:06:21PM +0300, Andy Shevchenko wrote: > There is a logical continuation of the commit 5fbe5b5883f8 ("gpio: Initialize > the irqchip valid_mask with a callback") to split IRQ initialization to > hardware and valid mask setup parts. > Hans, it will be nice if you would be able to test this as well. The better is to use our for-next branch with this applied on top. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-25 14:06 [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback Andy Shevchenko 2019-10-25 16:07 ` Andy Shevchenko @ 2019-10-28 11:29 ` Mika Westerberg 2019-10-30 9:30 ` Hans de Goede 2 siblings, 0 replies; 9+ messages in thread From: Mika Westerberg @ 2019-10-28 11:29 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Linus Walleij, linux-gpio, hdegoede On Fri, Oct 25, 2019 at 05:06:21PM +0300, Andy Shevchenko wrote: > There is a logical continuation of the commit 5fbe5b5883f8 ("gpio: Initialize > the irqchip valid_mask with a callback") to split IRQ initialization to > hardware and valid mask setup parts. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-25 14:06 [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback Andy Shevchenko 2019-10-25 16:07 ` Andy Shevchenko 2019-10-28 11:29 ` Mika Westerberg @ 2019-10-30 9:30 ` Hans de Goede 2019-10-30 12:42 ` Linus Walleij 2 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2019-10-30 9:30 UTC (permalink / raw) To: Andy Shevchenko, Linus Walleij, linux-gpio, Mika Westerberg Hi, On 25-10-2019 16:06, Andy Shevchenko wrote: > There is a logical continuation of the commit 5fbe5b5883f8 ("gpio: Initialize > the irqchip valid_mask with a callback") to split IRQ initialization to > hardware and valid mask setup parts. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> So I've been running some tests based on https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next + this patch. That combo causes several issues, so I tried reverting the patches one by one, if I drop this patch and use just: https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next Then the kernel does not even boot. I believe this is caused by: 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip") The problem here is that gpiochip_add_data_with_key() calls gpiochip_irqchip_init_hw() before it calls gpiochip_irqchip_init_valid_mask(), so after commit 88583e340a0e when byt_gpio_irq_init_hw runs gc->irq.valid_mask is NULL and we crash with a NULL pointer exception (or so I believe, the kernel never gets far enough to get any info out of it without extra work). Note that this ("[PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback") patch fixes this since it moves the gc->irq.valid_mask accesses to byt_init_irq_valid_mask. But this change itself triggers another variant of this ordering issue, it causes these 2 new errors to get logged: byt_gpio INT33FC:01: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x01800000 byt_gpio INT33FC:02: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x00400000 The problem is that before this change the code calculating the valid_mask would also disable interrupts on GPIOs which do not have their BYT_DIRECT_IRQ_EN bit set. This now happens after the check done in byt_gpio_irq_init_hw() causing these false-positive error messages. Even if we ignore the NULL pointer deref problem for now and we ignore these 2 new error messages for now. Things are still broken with the current changes in pinctrl/intel.git/for-next switching to letting devm_gpiochip_add_data register the irqchip means that acpi_gpiochip_request_interrupts() gets called before gpiochip_add_pin_range() is called from pinctrl-baytrail.c, causing the GPIO lookup of any ACPI _AEI handlers to fail, resulting in errors like this one: byt_gpio INT33FC:02: Failed to request GPIO for pin 0x13: -517 And none of the _AEI handlers working TL;DR: commit 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip") breaks a bunch of stuff and should be dropped from pinctrl/intel.git/for-next and this needs some more work before it is ready for mainline. Regards, Hans > --- > drivers/pinctrl/intel/pinctrl-baytrail.c | 25 ++++++++++-------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c > index beb26550c25f..08e2b940cc11 100644 > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c > @@ -1432,22 +1432,10 @@ static void byt_init_irq_valid_mask(struct gpio_chip *chip, > unsigned long *valid_mask, > unsigned int ngpios) > { > - /* > - * FIXME: currently the valid_mask is filled in as part of > - * initializing the irq_chip below in byt_gpio_irq_init_hw(). > - * when converting this driver to the new way of passing the > - * gpio_irq_chip along when adding the gpio_chip, move the > - * mask initialization into this callback instead. Right now > - * this callback is here to make sure the mask gets allocated. > - */ > -} > - > -static int byt_gpio_irq_init_hw(struct gpio_chip *gc) > -{ > - struct byt_gpio *vg = gpiochip_get_data(gc); > + struct byt_gpio *vg = gpiochip_get_data(chip); > struct device *dev = &vg->pdev->dev; > void __iomem *reg; > - u32 base, value; > + u32 value; > int i; > > /* > @@ -1468,13 +1456,20 @@ static int byt_gpio_irq_init_hw(struct gpio_chip *gc) > > value = readl(reg); > if (value & BYT_DIRECT_IRQ_EN) { > - clear_bit(i, gc->irq.valid_mask); > + clear_bit(i, valid_mask); > dev_dbg(dev, "excluding GPIO %d from IRQ domain\n", i); > } else if ((value & BYT_PIN_MUX) == byt_get_gpio_mux(vg, i)) { > byt_gpio_clear_triggering(vg, i); > dev_dbg(dev, "disabling GPIO %d\n", i); > } > } > +} > + > +static int byt_gpio_irq_init_hw(struct gpio_chip *gc) > +{ > + struct byt_gpio *vg = gpiochip_get_data(gc); > + void __iomem *reg; > + u32 base, value; > > /* clear interrupt status trigger registers */ > for (base = 0; base < vg->soc_data->npins; base += 32) { > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-30 9:30 ` Hans de Goede @ 2019-10-30 12:42 ` Linus Walleij 2019-10-30 13:11 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Linus Walleij @ 2019-10-30 12:42 UTC (permalink / raw) To: Hans de Goede; +Cc: Andy Shevchenko, open list:GPIO SUBSYSTEM, Mika Westerberg On Wed, Oct 30, 2019 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote: > The problem here is that gpiochip_add_data_with_key() calls gpiochip_irqchip_init_hw() > before it calls gpiochip_irqchip_init_valid_mask(), so after commit 88583e340a0e > when byt_gpio_irq_init_hw runs gc->irq.valid_mask is NULL and we crash with a NULL > pointer exception (or so I believe, the kernel never gets far enough to get > any info out of it without extra work). > > Note that this ("[PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback") > patch fixes this since it moves the gc->irq.valid_mask accesses to > byt_init_irq_valid_mask. OK so we have a halfway fix there. > But this change itself triggers another variant of this ordering issue, > it causes these 2 new errors to get logged: > > byt_gpio INT33FC:01: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x01800000 > byt_gpio INT33FC:02: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x00400000 > > The problem is that before this change the code calculating the valid_mask > would also disable interrupts on GPIOs which do not have their > BYT_DIRECT_IRQ_EN bit set. This now happens after the check done in > byt_gpio_irq_init_hw() causing these false-positive error messages. Isn't that easily fixed with something like this: diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 9afbc0612126..e865c889ba8d 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1411,11 +1411,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, machine_gpiochip_add(chip); - ret = gpiochip_irqchip_init_hw(chip); + ret = gpiochip_irqchip_init_valid_mask(chip); if (ret) goto err_remove_acpi_chip; - ret = gpiochip_irqchip_init_valid_mask(chip); + ret = gpiochip_irqchip_init_hw(chip); if (ret) goto err_remove_acpi_chip; (I sent a separate patch for this.) It isn't super-easy to know the right ordering semantics for init_hw vs init_valid_mask I think. Sadly we need to test it out in practice. > Even if we ignore the NULL pointer deref problem for now and we ignore > these 2 new error messages for now. Things are still broken with the > current changes in pinctrl/intel.git/for-next switching to letting > devm_gpiochip_add_data register the irqchip means that > acpi_gpiochip_request_interrupts() gets called before > gpiochip_add_pin_range() is called from pinctrl-baytrail.c, causing > the GPIO lookup of any ACPI _AEI handlers to fail, resulting in > errors like this one: > > byt_gpio INT33FC:02: Failed to request GPIO for pin 0x13: -517 > > And none of the _AEI handlers working I just vaguely understand this... If what you're saying is that the Baytrail driver is dependent on registering the pin ranges *before* registering the GPIO chip can we then: diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index beb26550c25f..b308567c5153 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg) girq->handler = handle_bad_irq; } - ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg); + /* + * Needs to happen first since the gpiochip is using pin + * control as back-end. + */ + ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev), + 0, 0, vg->soc_data->npins); if (ret) { - dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n"); + dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n"); return ret; } - ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev), - 0, 0, vg->soc_data->npins); + ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg); if (ret) { - dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n"); + dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n"); return ret; } (Tell me if I should send this as a separate patch.) It's not entirely logical to have this semantic ordering so the extra comment explains it, I hope, in case it actually works. > TL;DR: commit 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip") > breaks a bunch of stuff and should be dropped from pinctrl/intel.git/for-next > and this needs some more work before it is ready for mainline. I don't know if that is such a good idea if this is a global problem, like something that would potentially disturb any ACPI-based GPIO chip. We might leave something else broken even if we fix the issue locally. Yours, Linus Walleij ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-30 12:42 ` Linus Walleij @ 2019-10-30 13:11 ` Hans de Goede 2019-10-30 14:47 ` Andy Shevchenko 2019-10-30 14:51 ` Linus Walleij 0 siblings, 2 replies; 9+ messages in thread From: Hans de Goede @ 2019-10-30 13:11 UTC (permalink / raw) To: Linus Walleij; +Cc: Andy Shevchenko, open list:GPIO SUBSYSTEM, Mika Westerberg Hi, On 30-10-2019 13:42, Linus Walleij wrote: > On Wed, Oct 30, 2019 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> The problem here is that gpiochip_add_data_with_key() calls gpiochip_irqchip_init_hw() >> before it calls gpiochip_irqchip_init_valid_mask(), so after commit 88583e340a0e >> when byt_gpio_irq_init_hw runs gc->irq.valid_mask is NULL and we crash with a NULL >> pointer exception (or so I believe, the kernel never gets far enough to get >> any info out of it without extra work). >> >> Note that this ("[PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback") >> patch fixes this since it moves the gc->irq.valid_mask accesses to >> byt_init_irq_valid_mask. > > OK so we have a halfway fix there. > >> But this change itself triggers another variant of this ordering issue, >> it causes these 2 new errors to get logged: >> >> byt_gpio INT33FC:01: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x01800000 >> byt_gpio INT33FC:02: GPIO interrupt error, pins misconfigured. INT_STAT0: 0x00400000 >> >> The problem is that before this change the code calculating the valid_mask >> would also disable interrupts on GPIOs which do not have their >> BYT_DIRECT_IRQ_EN bit set. This now happens after the check done in >> byt_gpio_irq_init_hw() causing these false-positive error messages. > > Isn't that easily fixed with something like this: > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 9afbc0612126..e865c889ba8d 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1411,11 +1411,11 @@ int gpiochip_add_data_with_key(struct > gpio_chip *chip, void *data, > > machine_gpiochip_add(chip); > > - ret = gpiochip_irqchip_init_hw(chip); > + ret = gpiochip_irqchip_init_valid_mask(chip); > if (ret) > goto err_remove_acpi_chip; > > - ret = gpiochip_irqchip_init_valid_mask(chip); > + ret = gpiochip_irqchip_init_hw(chip); > if (ret) > goto err_remove_acpi_chip; > > (I sent a separate patch for this.) Yes I just replied to that patch, this seems like a good fix to me. > It isn't super-easy to know the right ordering semantics > for init_hw vs init_valid_mask I think. Sadly we need to > test it out in practice. Ack. >> Even if we ignore the NULL pointer deref problem for now and we ignore >> these 2 new error messages for now. Things are still broken with the >> current changes in pinctrl/intel.git/for-next switching to letting >> devm_gpiochip_add_data register the irqchip means that >> acpi_gpiochip_request_interrupts() gets called before >> gpiochip_add_pin_range() is called from pinctrl-baytrail.c, causing >> the GPIO lookup of any ACPI _AEI handlers to fail, resulting in >> errors like this one: >> >> byt_gpio INT33FC:02: Failed to request GPIO for pin 0x13: -517 >> >> And none of the _AEI handlers working > > I just vaguely understand this... > > If what you're saying is that the Baytrail driver is dependent > on registering the pin ranges *before* registering the GPIO > chip Yes I think so, I debugged the _AEI handlers not working a bit yesterday and the problem is that pinctrl_gpio_request() fails, first pinctrl_get_device_gpio_range fails with -EPROBEDEFER (*) and then pinctrl_match_gpio_range() also fails. I added some debug pr_err calls to pinctrl_match_gpio_range() and when it runs the range for the gpiochip for which acpi_gpiochip_request_interrupts() is processing _AEI event-handlers is not yet in the registered ranges. *) Which is not treated specially by acpi_gpiochip_request_interrupts() as that normally gets called from the gpiochip driver itself, so the device is expected to alreayd be probed. > can we then: > > diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c > b/drivers/pinctrl/intel/pinctrl-baytrail.c > index beb26550c25f..b308567c5153 100644 > --- a/drivers/pinctrl/intel/pinctrl-baytrail.c > +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c > @@ -1549,16 +1549,20 @@ static int byt_gpio_probe(struct byt_gpio *vg) > girq->handler = handle_bad_irq; > } > > - ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg); > + /* > + * Needs to happen first since the gpiochip is using pin > + * control as back-end. > + */ > + ret = gpiochip_add_pin_range(gc, dev_name(&vg->pdev->dev), > + 0, 0, vg->soc_data->npins); > if (ret) { > - dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n"); > + dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n"); > return ret; > } > > - ret = gpiochip_add_pin_range(&vg->chip, dev_name(&vg->pdev->dev), > - 0, 0, vg->soc_data->npins); > + ret = devm_gpiochip_add_data(&vg->pdev->dev, gc, vg); > if (ret) { > - dev_err(&vg->pdev->dev, "failed to add GPIO pin range\n"); > + dev_err(&vg->pdev->dev, "failed adding byt-gpio chip\n"); > return ret; > } > > (Tell me if I should send this as a separate patch.) If you want me to test if this fixes the issue, then yes please. > It's not entirely logical to have this semantic ordering so > the extra comment explains it, I hope, in case it actually > works. > >> TL;DR: commit 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip") >> breaks a bunch of stuff and should be dropped from pinctrl/intel.git/for-next >> and this needs some more work before it is ready for mainline. > > I don't know if that is such a good idea if this is a global problem, > like something that would potentially disturb any ACPI-based > GPIO chip. We might leave something else broken even if we > fix the issue locally. Right, I did a quick check and at least these x86 pinctrl drivers all 3 have this ordering problem once the irq chip registration is moved to the gpiochip_add_data() call. drivers/pinctrl/intel/pinctrl-baytrail.c drivers/pinctrl/intel/pinctrl-cherryview.c drivers/pinctrl/intel/pinctrl-intel.c drivers/pinctrl/pinctrl-amd.c And it seems that drivers/gpio/gpio-merrifield.c is already suffering from this problem in 5.4! So some more generic solution would be ideal... Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-30 13:11 ` Hans de Goede @ 2019-10-30 14:47 ` Andy Shevchenko 2019-10-30 15:03 ` Hans de Goede 2019-10-30 14:51 ` Linus Walleij 1 sibling, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2019-10-30 14:47 UTC (permalink / raw) To: Hans de Goede; +Cc: Linus Walleij, open list:GPIO SUBSYSTEM, Mika Westerberg On Wed, Oct 30, 2019 at 02:11:50PM +0100, Hans de Goede wrote: > On 30-10-2019 13:42, Linus Walleij wrote: > > On Wed, Oct 30, 2019 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote: > > > TL;DR: commit 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip") > > > breaks a bunch of stuff and should be dropped from pinctrl/intel.git/for-next > > > and this needs some more work before it is ready for mainline. > > > > I don't know if that is such a good idea if this is a global problem, > > like something that would potentially disturb any ACPI-based > > GPIO chip. We might leave something else broken even if we > > fix the issue locally. > > Right, I did a quick check and at least these x86 pinctrl drivers > all 3 have this ordering problem once the irq chip registration is > moved to the gpiochip_add_data() call. > > drivers/pinctrl/intel/pinctrl-baytrail.c > drivers/pinctrl/intel/pinctrl-cherryview.c > drivers/pinctrl/intel/pinctrl-intel.c > drivers/pinctrl/pinctrl-amd.c Hmm.. do we have cherryview broken in next / vanilla? > > And it seems that drivers/gpio/gpio-merrifield.c is already > suffering from this problem in 5.4! > > So some more generic solution would be ideal... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-30 14:47 ` Andy Shevchenko @ 2019-10-30 15:03 ` Hans de Goede 0 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2019-10-30 15:03 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Linus Walleij, open list:GPIO SUBSYSTEM, Mika Westerberg Hi, On 30-10-2019 15:47, Andy Shevchenko wrote: > On Wed, Oct 30, 2019 at 02:11:50PM +0100, Hans de Goede wrote: >> On 30-10-2019 13:42, Linus Walleij wrote: >>> On Wed, Oct 30, 2019 at 10:31 AM Hans de Goede <hdegoede@redhat.com> wrote: > >>>> TL;DR: commit 88583e340a0e ("pinctrl: intel: baytrail: Pass irqchip when adding gpiochip") >>>> breaks a bunch of stuff and should be dropped from pinctrl/intel.git/for-next >>>> and this needs some more work before it is ready for mainline. >>> >>> I don't know if that is such a good idea if this is a global problem, >>> like something that would potentially disturb any ACPI-based >>> GPIO chip. We might leave something else broken even if we >>> fix the issue locally. >> >> Right, I did a quick check and at least these x86 pinctrl drivers >> all 3 have this ordering problem once the irq chip registration is >> moved to the gpiochip_add_data() call. >> >> drivers/pinctrl/intel/pinctrl-baytrail.c >> drivers/pinctrl/intel/pinctrl-cherryview.c >> drivers/pinctrl/intel/pinctrl-intel.c >> drivers/pinctrl/pinctrl-amd.c > > Hmm.. do we have cherryview broken in next / vanilla? In 5.4-rc# the irq chip is still registered as a separate step after the gpiochip_add_pin_range calls. I'm also not seen any troublesome patches here: https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next >> And it seems that drivers/gpio/gpio-merrifield.c is already >> suffering from this problem in 5.4! >> >> So some more generic solution would be ideal... merrifield OTOH definitely seems broken wrt this (when looking at the code). Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback 2019-10-30 13:11 ` Hans de Goede 2019-10-30 14:47 ` Andy Shevchenko @ 2019-10-30 14:51 ` Linus Walleij 1 sibling, 0 replies; 9+ messages in thread From: Linus Walleij @ 2019-10-30 14:51 UTC (permalink / raw) To: Hans de Goede; +Cc: Andy Shevchenko, open list:GPIO SUBSYSTEM, Mika Westerberg On Wed, Oct 30, 2019 at 2:11 PM Hans de Goede <hdegoede@redhat.com> wrote: > > (Tell me if I should send this as a separate patch.) > > If you want me to test if this fixes the issue, then yes please. I sent a patch fixing all the instances we can immediately spot. > Right, I did a quick check and at least these x86 pinctrl drivers > all 3 have this ordering problem once the irq chip registration is > moved to the gpiochip_add_data() call. > > drivers/pinctrl/intel/pinctrl-baytrail.c > drivers/pinctrl/intel/pinctrl-cherryview.c > drivers/pinctrl/intel/pinctrl-intel.c > drivers/pinctrl/pinctrl-amd.c > > And it seems that drivers/gpio/gpio-merrifield.c is already > suffering from this problem in 5.4! I fixed all of these in my patch, let's see what happens. If we need to partially backport it to v5.4 then let's just do that. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-30 15:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-25 14:06 [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback Andy Shevchenko 2019-10-25 16:07 ` Andy Shevchenko 2019-10-28 11:29 ` Mika Westerberg 2019-10-30 9:30 ` Hans de Goede 2019-10-30 12:42 ` Linus Walleij 2019-10-30 13:11 ` Hans de Goede 2019-10-30 14:47 ` Andy Shevchenko 2019-10-30 15:03 ` Hans de Goede 2019-10-30 14:51 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.