gpiolib: Switch order of valid mask and hw init

Message ID 20191030122914.967-1-linus.walleij@linaro.org
State Accepted
Commit 504369cd6d2ce34c1225063071ac7e0a5a4d5e30
Headers show
Series
  • gpiolib: Switch order of valid mask and hw init
Related show

Commit Message

Linus Walleij Oct. 30, 2019, 12:29 p.m.
The GPIO irqchip needs to initialize the valid mask
before initializing the IRQ hardware, because sometimes
the latter require the former to be executed first.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
Thinking of applying this for fixes to fix some part
of the problems that Hans is seeing.
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.21.0

Comments

Hans de Goede Oct. 30, 2019, 12:43 p.m. | #1
Hi,

On 30-10-2019 13:29, Linus Walleij wrote:
> The GPIO irqchip needs to initialize the valid mask

> before initializing the IRQ hardware, because sometimes

> the latter require the former to be executed first.

> 

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

> Reported-by: Hans de Goede <hdegoede@redhat.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Ack, I was thinking along these lines myself too, but I was
not sure if this would be an acceptable solution:

Acked-by: Hans de Goede <hdegoede@redhat.com>


> ---

> Thinking of applying this for fixes to fix some part

> of the problems that Hans is seeing.


So you want to get this into 5.4, so that when
"pinctrl: intel: baytrail: Pass irqchip when adding gpiochip"
lands in 5.5 this is already in place.

Ok, I've just checked all the existing users if the
init_hw callback and none of them use init_valid_mask
so for them to order should not matter.

So yes getting this into 5.4 would be good.

This fixes 2 of the 3 issues I mentioned in my other mail,
the NULL pointer deref and the false_positive error messages
from byt_gpio_irq_init_hw().

But as I guess you are aware, that still leaves us with the third
problem: "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"

Regards,

Hans


> ---

>   drivers/gpio/gpiolib.c | 4 ++--

>   1 file changed, 2 insertions(+), 2 deletions(-)

> 

> 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;

>   

>
Mika Westerberg Oct. 30, 2019, 12:48 p.m. | #2
On Wed, Oct 30, 2019 at 01:29:14PM +0100, Linus Walleij wrote:
> The GPIO irqchip needs to initialize the valid mask

> before initializing the IRQ hardware, because sometimes

> the latter require the former to be executed first.

> 

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>


Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Andy Shevchenko Oct. 30, 2019, 1:38 p.m. | #3
On Wed, Oct 30, 2019 at 01:29:14PM +0100, Linus Walleij wrote:
> The GPIO irqchip needs to initialize the valid mask

> before initializing the IRQ hardware, because sometimes

> the latter require the former to be executed first.

> 


Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

> Reported-by: Hans de Goede <hdegoede@redhat.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> Thinking of applying this for fixes to fix some part

> of the problems that Hans is seeing.

> ---

>  drivers/gpio/gpiolib.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> 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;

>  

> -- 

> 2.21.0

> 


-- 
With Best Regards,
Andy Shevchenko
Andy Shevchenko Oct. 30, 2019, 1:41 p.m. | #4
On Wed, Oct 30, 2019 at 01:43:38PM +0100, Hans de Goede wrote:
> On 30-10-2019 13:29, Linus Walleij wrote:


> But as I guess you are aware, that still leaves us with the third

> problem: "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 think we may postpone the Baytrail patches, if we don't find a solution for
the above soon.

Nevertheless, this change on its own good to have.

Hans, thanks for testing!

-- 
With Best Regards,
Andy Shevchenko

Patch

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;