[PATCH] gpiolib: Switch order of valid mask and hw init

[PATCH] gpiolib: Switch order of valid mask and hw init

[Linus Walleij]

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(-)

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

Re: [PATCH] gpiolib: Switch order of valid mask and hw init

[Hans de Goede]

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

Re: [PATCH] gpiolib: Switch order of valid mask and hw init

[Andy Shevchenko]

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

Re: [PATCH] gpiolib: Switch order of valid mask and hw init

[Mika Westerberg]

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>

Re: [PATCH] gpiolib: Switch order of valid mask and hw init

[Andy Shevchenko]

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