Re: [PATCH v1] pinctrl: baytrail: Move IRQ valid mask initialization to a dedicated callback

[Hans de Goede <hdegoede@redhat.com>]

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