[PATCH] gpio: pca953x: Use irqchip template

[PATCH] gpio: pca953x: Use irqchip template

[Linus Walleij]

This makes the driver use the irqchip template to assign
properties to the gpio_irq_chip instead of using the
explicit calls to gpiochip_irqchip_add_nested() and
gpiochip_set_nested_irqchip(). The irqchip is instead
added while adding the gpiochip.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Adam Ford <aford173@gmail.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-pca953x.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 9c90cf3aac5a..ab22152bf3e8 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -834,6 +834,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	struct irq_chip *irq_chip = &chip->irq_chip;
 	DECLARE_BITMAP(reg_direction, MAX_LINE);
 	DECLARE_BITMAP(irq_stat, MAX_LINE);
+	struct gpio_irq_chip *girq;
 	int ret;
 
 	if (dmi_first_match(pca953x_dmi_acpi_irq_info)) {
@@ -883,16 +884,16 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
 	irq_chip->irq_set_type = pca953x_irq_set_type;
 	irq_chip->irq_shutdown = pca953x_irq_shutdown;
 
-	ret = gpiochip_irqchip_add_nested(&chip->gpio_chip, irq_chip,
-					  irq_base, handle_simple_irq,
-					  IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(&client->dev,
-			"could not connect irqchip to gpiochip\n");
-		return ret;
-	}
-
-	gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
+	girq = &chip->gpio_chip.irq;
+	girq->chip = irq_chip;
+	/* This will let us handle the parent IRQ in the driver */
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->parents = NULL;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_simple_irq;
+	girq->threaded = true;
+	girq->first = irq_base; /* FIXME: get rid of this */
 
 	return 0;
 }
@@ -1080,11 +1081,11 @@ static int pca953x_probe(struct i2c_client *client,
 	if (ret)
 		goto err_exit;
 
-	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
+	ret = pca953x_irq_setup(chip, irq_base);
 	if (ret)
 		goto err_exit;
 
-	ret = pca953x_irq_setup(chip, irq_base);
+	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
 	if (ret)
 		goto err_exit;
 
-- 
2.26.2

Re: [PATCH] gpio: pca953x: Use irqchip template

[Andy Shevchenko]

On Fri, Jul 17, 2020 at 04:40:40PM +0200, Linus Walleij wrote:
> This makes the driver use the irqchip template to assign
> properties to the gpio_irq_chip instead of using the
> explicit calls to gpiochip_irqchip_add_nested() and
> gpiochip_set_nested_irqchip(). The irqchip is instead
> added while adding the gpiochip.

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

> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpio-pca953x.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 9c90cf3aac5a..ab22152bf3e8 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -834,6 +834,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
>  	struct irq_chip *irq_chip = &chip->irq_chip;
>  	DECLARE_BITMAP(reg_direction, MAX_LINE);
>  	DECLARE_BITMAP(irq_stat, MAX_LINE);
> +	struct gpio_irq_chip *girq;
>  	int ret;
>  
>  	if (dmi_first_match(pca953x_dmi_acpi_irq_info)) {
> @@ -883,16 +884,16 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
>  	irq_chip->irq_set_type = pca953x_irq_set_type;
>  	irq_chip->irq_shutdown = pca953x_irq_shutdown;
>  
> -	ret = gpiochip_irqchip_add_nested(&chip->gpio_chip, irq_chip,
> -					  irq_base, handle_simple_irq,
> -					  IRQ_TYPE_NONE);
> -	if (ret) {
> -		dev_err(&client->dev,
> -			"could not connect irqchip to gpiochip\n");
> -		return ret;
> -	}
> -
> -	gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
> +	girq = &chip->gpio_chip.irq;
> +	girq->chip = irq_chip;
> +	/* This will let us handle the parent IRQ in the driver */
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_simple_irq;
> +	girq->threaded = true;
> +	girq->first = irq_base; /* FIXME: get rid of this */
>  
>  	return 0;
>  }
> @@ -1080,11 +1081,11 @@ static int pca953x_probe(struct i2c_client *client,
>  	if (ret)
>  		goto err_exit;
>  
> -	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
> +	ret = pca953x_irq_setup(chip, irq_base);
>  	if (ret)
>  		goto err_exit;
>  
> -	ret = pca953x_irq_setup(chip, irq_base);
> +	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
>  	if (ret)
>  		goto err_exit;
>  
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: pca953x: Use irqchip template

[Nikhil Devshatwar]

On 16:40-20200717, Linus Walleij wrote:
> This makes the driver use the irqchip template to assign
> properties to the gpio_irq_chip instead of using the
> explicit calls to gpiochip_irqchip_add_nested() and
> gpiochip_set_nested_irqchip(). The irqchip is instead
> added while adding the gpiochip.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Adam Ford <aford173@gmail.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Hi,
I am getting a kernel crash on K3 j721e common processor board
when HDMI is plugged in. Following is the full log with crash
for NULL pointer derefence

https://pastebin.ubuntu.com/p/wBPS2ymmqR/

Upon inspection, I found that the "irq_find_mapping" call
in the "pca953x_irq_handler" returns 0 and the same is passed
to "handle_nested_irq"


> ---
>  drivers/gpio/gpio-pca953x.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 9c90cf3aac5a..ab22152bf3e8 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -834,6 +834,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
>  	struct irq_chip *irq_chip = &chip->irq_chip;
>  	DECLARE_BITMAP(reg_direction, MAX_LINE);
>  	DECLARE_BITMAP(irq_stat, MAX_LINE);
> +	struct gpio_irq_chip *girq;
>  	int ret;
>  
>  	if (dmi_first_match(pca953x_dmi_acpi_irq_info)) {
> @@ -883,16 +884,16 @@ static int pca953x_irq_setup(struct pca953x_chip *chip, int irq_base)
>  	irq_chip->irq_set_type = pca953x_irq_set_type;
>  	irq_chip->irq_shutdown = pca953x_irq_shutdown;
>  
> -	ret = gpiochip_irqchip_add_nested(&chip->gpio_chip, irq_chip,
> -					  irq_base, handle_simple_irq,
> -					  IRQ_TYPE_NONE);
> -	if (ret) {
> -		dev_err(&client->dev,
> -			"could not connect irqchip to gpiochip\n");
> -		return ret;
> -	}
> -
> -	gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
> +	girq = &chip->gpio_chip.irq;
> +	girq->chip = irq_chip;
> +	/* This will let us handle the parent IRQ in the driver */
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_simple_irq;
> +	girq->threaded = true;
> +	girq->first = irq_base; /* FIXME: get rid of this */
>  
>  	return 0;
>  }
> @@ -1080,11 +1081,11 @@ static int pca953x_probe(struct i2c_client *client,
>  	if (ret)
>  		goto err_exit;
>  
> -	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
> +	ret = pca953x_irq_setup(chip, irq_base);
>  	if (ret)
>  		goto err_exit;
>  
> -	ret = pca953x_irq_setup(chip, irq_base);
> +	ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);
>  	if (ret)
>  		goto err_exit;
>  
> -- 
> 2.26.2
> 

Re: [PATCH] gpio: pca953x: Use irqchip template

[Andy Shevchenko]

On Wed, Sep 30, 2020 at 04:17:29PM +0530, Nikhil Devshatwar wrote:
> On 16:40-20200717, Linus Walleij wrote:
> > This makes the driver use the irqchip template to assign
> > properties to the gpio_irq_chip instead of using the
> > explicit calls to gpiochip_irqchip_add_nested() and
> > gpiochip_set_nested_irqchip(). The irqchip is instead
> > added while adding the gpiochip.

> I am getting a kernel crash on K3 j721e common processor board
> when HDMI is plugged in. Following is the full log with crash
> for NULL pointer derefence
> 
> https://pastebin.ubuntu.com/p/wBPS2ymmqR/
> 
> Upon inspection, I found that the "irq_find_mapping" call
> in the "pca953x_irq_handler" returns 0 and the same is passed
> to "handle_nested_irq"

Thanks for the report!

I would like to clarify the following:
- with the above patch reverted everything is fine, correct?
- v5.9-rc7 doesn't make any differences, correct?
- patch [1] doesn't help, correct?

[1]: https://lore.kernel.org/linux-gpio/1600851824-4608-1-git-send-email-ye.li@nxp.com/T/#u

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: pca953x: Use irqchip template

[Linus Walleij]

On Wed, Sep 30, 2020 at 12:47 PM Nikhil Devshatwar <nikhil.nd@ti.com> wrote:

> I am getting a kernel crash on K3 j721e common processor board
> when HDMI is plugged in. Following is the full log with crash
> for NULL pointer derefence
>
> https://pastebin.ubuntu.com/p/wBPS2ymmqR/
>
> Upon inspection, I found that the "irq_find_mapping" call
> in the "pca953x_irq_handler" returns 0 and the same is passed
> to "handle_nested_irq"

This would typically happen if the driver using an IRQ
from the PCA953x does not properly request it.

Is this caused by this IRQ 504 from your /proc/interrupts:

504:          0          0    4-0021  10 Edge      HPD

This seems to be requested directly by the HDMI bridge
and not by the TI display controller.

I look in the k3-am654-base-board.dts in the upstream
kernel and I find this:

&main_i2c0 {
        pinctrl-names = "default";
        pinctrl-0 = <&main_i2c0_pins_default>;
        clock-frequency = <400000>;

        pca9555: gpio@21 {
                compatible = "nxp,pca9555";
                reg = <0x21>;
                gpio-controller;
                #gpio-cells = <2>;
        };
};

This is the GPIO controller used here, right?

I notice the following:
- There are no HDMI bridges using this GPIO controller in
  the upstream kernel.
- The PCA9555 here lacks necessary attributes such as
  parent IRQ (another GPIO) and the "interrupt-controller"
  and "#interrupt-cells".

So this can not be the device tree you are using.

Can you point us to:
- The actual device tree you are booting from?
- The actual bridge that is requesting the HPD IRQ?
- The upstream code for this bridge?

Yours,
Linus Walleij

Re: [PATCH] gpio: pca953x: Use irqchip template

[Grygorii Strashko]

hi Linus,

On 30/09/2020 16:51, Linus Walleij wrote:
> On Wed, Sep 30, 2020 at 12:47 PM Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> 
>> I am getting a kernel crash on K3 j721e common processor board
>> when HDMI is plugged in. Following is the full log with crash
>> for NULL pointer derefence
>>
>> https://pastebin.ubuntu.com/p/wBPS2ymmqR/
>>
>> Upon inspection, I found that the "irq_find_mapping" call
>> in the "pca953x_irq_handler" returns 0 and the same is passed
>> to "handle_nested_irq"
> 
> This would typically happen if the driver using an IRQ
> from the PCA953x does not properly request it.
> 
> Is this caused by this IRQ 504 from your /proc/interrupts:
> 
> 504:          0          0    4-0021  10 Edge      HPD
> 
> This seems to be requested directly by the HDMI bridge
> and not by the TI display controller.
> 
> I look in the k3-am654-base-board.dts in the upstream
> kernel and I find this:
> 
> &main_i2c0 {
>          pinctrl-names = "default";
>          pinctrl-0 = <&main_i2c0_pins_default>;
>          clock-frequency = <400000>;
> 
>          pca9555: gpio@21 {
>                  compatible = "nxp,pca9555";
>                  reg = <0x21>;
>                  gpio-controller;
>                  #gpio-cells = <2>;
>          };
> };
> 
> This is the GPIO controller used here, right?
> 
> I notice the following:
> - There are no HDMI bridges using this GPIO controller in
>    the upstream kernel.
> - The PCA9555 here lacks necessary attributes such as
>    parent IRQ (another GPIO) and the "interrupt-controller"
>    and "#interrupt-cells".
> 
> So this can not be the device tree you are using.
> 
> Can you point us to:
> - The actual device tree you are booting from?
> - The actual bridge that is requesting the HPD IRQ?
> - The upstream code for this bridge?

We've just got another similar report, don't know the root cause, but it's not HDMI

There is one i see with this patch

-       ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);

[GS] before: GPIO chip fully initialized

+       ret = pca953x_irq_setup(chip, irq_base);

[GS] after: IRQ chip related data initialized and Parent IRQ requested

         if (ret)
                 goto err_exit;
  
-       ret = pca953x_irq_setup(chip, irq_base);
+       ret = devm_gpiochip_add_data(&client->dev, &chip->gpio_chip, chip);

[GS] after: But GPIO chip and IRQ chip are fully initialized only here, so any IRQ before devm_gpiochip_add_data may crash.

         if (ret)
                 goto err_exit;


-- 
Best regards,
grygorii

Re: [PATCH] gpio: pca953x: Use irqchip template

[Andy Shevchenko]

On Fri, Oct 02, 2020 at 12:07:50AM +0300, Grygorii Strashko wrote:
> On 30/09/2020 16:51, Linus Walleij wrote:
> > On Wed, Sep 30, 2020 at 12:47 PM Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> > 
> > > I am getting a kernel crash on K3 j721e common processor board
> > > when HDMI is plugged in. Following is the full log with crash
> > > for NULL pointer derefence
> > > 
> > > https://pastebin.ubuntu.com/p/wBPS2ymmqR/
> > > 
> > > Upon inspection, I found that the "irq_find_mapping" call
> > > in the "pca953x_irq_handler" returns 0 and the same is passed
> > > to "handle_nested_irq"
> > 
> > This would typically happen if the driver using an IRQ
> > from the PCA953x does not properly request it.

...

> We've just got another similar report, don't know the root cause, but it's not HDMI

Does
 e43c26e12dd4 ("gpio: pca953x: Fix uninitialized pending variable")
help?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpio: pca953x: Use irqchip template

[Grygorii Strashko]

On 02/10/2020 13:40, Andy Shevchenko wrote:
> On Fri, Oct 02, 2020 at 12:07:50AM +0300, Grygorii Strashko wrote:
>> On 30/09/2020 16:51, Linus Walleij wrote:
>>> On Wed, Sep 30, 2020 at 12:47 PM Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
>>>
>>>> I am getting a kernel crash on K3 j721e common processor board
>>>> when HDMI is plugged in. Following is the full log with crash
>>>> for NULL pointer derefence
>>>>
>>>> https://pastebin.ubuntu.com/p/wBPS2ymmqR/
>>>>
>>>> Upon inspection, I found that the "irq_find_mapping" call
>>>> in the "pca953x_irq_handler" returns 0 and the same is passed
>>>> to "handle_nested_irq"
>>>
>>> This would typically happen if the driver using an IRQ
>>> from the PCA953x does not properly request it.
> 
> ...
> 
>> We've just got another similar report, don't know the root cause, but it's not HDMI
> 
> Does
>   e43c26e12dd4 ("gpio: pca953x: Fix uninitialized pending variable")
> help?
> 

Looks like it is. Thanks

-- 
Best regards,
grygorii