| Message ID | 20200717144040.63253-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | edee3bc6589484ec8c32b04c3b83497acfa6102f |
| Headers | show |
| Series | gpio: pca953x: Use irqchip template | expand |
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
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 >
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
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
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;
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?
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
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;
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(-) -- 2.26.2