[PATCH] pinctrl: qcom: Guard irq_eoi()

[PATCH] pinctrl: qcom: Guard irq_eoi()

[Linus Walleij]

In the commit setting up the qcom/msm pin controller to
be hierarchical some callbacks were careful to check that
d->parent_data on irq_data was valid before calling the
parent function, however irq_chip_eoi_parent() was called
unconditionally which doesn't work with elder Qualcomm
platforms such as APQ8060.

When the drivers/mfd/qcom-pm8xxx.c driver calls
chained_irq_exit() that call will in turn call chip->irq_eoi()
which is set to irq_chip_eoi_parent() by default on a
hierachical IRQ chip, and the parent is pinctrl-msm.c
so that will in turn unconditionally call
irq_chip_eoi_parent() again, but its parent is invalid
so we get the following crash:

 Unnable to handle kernel NULL pointer dereference at
 virtual address 00000010
 pgd = (ptrval)
 [00000010] *pgd=00000000
 Internal error: Oops: 5 [#1] PREEMPT SMP ARM
 (...)
 PC is at irq_chip_eoi_parent+0x4/0x10
 LR is at pm8xxx_irq_handler+0x1b4/0x2d8

Implement a local stub just avoiding to call down to
irq_chip_eoi_parent() if d->parent_data is not set.

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: stable@vger.kernel.org
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 9a8daa256a32..511f596cf2c3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -828,6 +828,12 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 	msm_gpio_irq_clear_unmask(d, false);
 }
 
+static void msm_gpio_irq_eoi(struct irq_data *d)
+{
+	if (d->parent_data)
+		irq_chip_eoi_parent(d);
+}
+
 static void msm_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -1104,7 +1110,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
-	pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
+	pctrl->irq_chip.irq_eoi = msm_gpio_irq_eoi;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
-- 
2.24.1

Re: [PATCH] pinctrl: qcom: Guard irq_eoi()

[Marc Zyngier]

Hi Linus,

On 2020-03-06 12:12, Linus Walleij wrote:
> In the commit setting up the qcom/msm pin controller to
> be hierarchical some callbacks were careful to check that
> d->parent_data on irq_data was valid before calling the
> parent function, however irq_chip_eoi_parent() was called
> unconditionally which doesn't work with elder Qualcomm
> platforms such as APQ8060.
> 
> When the drivers/mfd/qcom-pm8xxx.c driver calls
> chained_irq_exit() that call will in turn call chip->irq_eoi()
> which is set to irq_chip_eoi_parent() by default on a
> hierachical IRQ chip, and the parent is pinctrl-msm.c
> so that will in turn unconditionally call
> irq_chip_eoi_parent() again, but its parent is invalid
> so we get the following crash:
> 
>  Unnable to handle kernel NULL pointer dereference at
>  virtual address 00000010
>  pgd = (ptrval)
>  [00000010] *pgd=00000000
>  Internal error: Oops: 5 [#1] PREEMPT SMP ARM
>  (...)
>  PC is at irq_chip_eoi_parent+0x4/0x10
>  LR is at pm8xxx_irq_handler+0x1b4/0x2d8
> 
> Implement a local stub just avoiding to call down to
> irq_chip_eoi_parent() if d->parent_data is not set.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: stable@vger.kernel.org
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
> b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 9a8daa256a32..511f596cf2c3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -828,6 +828,12 @@ static void msm_gpio_irq_unmask(struct irq_data 
> *d)
>  	msm_gpio_irq_clear_unmask(d, false);
>  }
> 
> +static void msm_gpio_irq_eoi(struct irq_data *d)
> +{
> +	if (d->parent_data)
> +		irq_chip_eoi_parent(d);
> +}
> +
>  static void msm_gpio_irq_ack(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -1104,7 +1110,7 @@ static int msm_gpio_init(struct msm_pinctrl 
> *pctrl)
>  	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
>  	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
>  	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> -	pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
> +	pctrl->irq_chip.irq_eoi = msm_gpio_irq_eoi;
>  	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
>  	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
>  	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;

Long term, it may me better to offer a different set of callbacks
for interrupts that are terminated at the pinctrl level.

In the meantime, and as a fix:

Acked-by: Marc Zyngier <maz@kernel.org>

I assume you'll take this via the pinctrl tree?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] pinctrl: qcom: Guard irq_eoi()

[Linus Walleij]

On Fri, Mar 6, 2020 at 1:18 PM Marc Zyngier <maz@kernel.org> wrote:
> On 2020-03-06 12:12, Linus Walleij wrote:

> > +static void msm_gpio_irq_eoi(struct irq_data *d)
> > +{
> > +     if (d->parent_data)
> > +             irq_chip_eoi_parent(d);
> > +}
> > +
(...)
> > -     pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
> > +     pctrl->irq_chip.irq_eoi = msm_gpio_irq_eoi;
> >       pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
> >       pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
> >       pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>
> Long term, it may me better to offer a different set of callbacks
> for interrupts that are terminated at the pinctrl level.

Yeah, it's gpiolib here actually. Pin control is pretty much innocent,
it's just that the driver doubles as a GPIO driver with an irqchip
inside it.

> In the meantime, and as a fix:
>
> Acked-by: Marc Zyngier <maz@kernel.org>
>
> I assume you'll take this via the pinctrl tree?

Yeps, thanks Marc!

Yours,
Linus Walleij

Re: [PATCH] pinctrl: qcom: Guard irq_eoi()

[Stephen Boyd]

Quoting Linus Walleij (2020-03-06 04:12:21)
> In the commit setting up the qcom/msm pin controller to
> be hierarchical some callbacks were careful to check that
> d->parent_data on irq_data was valid before calling the
> parent function, however irq_chip_eoi_parent() was called
> unconditionally which doesn't work with elder Qualcomm
> platforms such as APQ8060.
> 
> When the drivers/mfd/qcom-pm8xxx.c driver calls
> chained_irq_exit() that call will in turn call chip->irq_eoi()
> which is set to irq_chip_eoi_parent() by default on a
> hierachical IRQ chip, and the parent is pinctrl-msm.c
> so that will in turn unconditionally call
> irq_chip_eoi_parent() again, but its parent is invalid
> so we get the following crash:
> 
>  Unnable to handle kernel NULL pointer dereference at
>  virtual address 00000010
>  pgd = (ptrval)
>  [00000010] *pgd=00000000
>  Internal error: Oops: 5 [#1] PREEMPT SMP ARM
>  (...)
>  PC is at irq_chip_eoi_parent+0x4/0x10
>  LR is at pm8xxx_irq_handler+0x1b4/0x2d8
> 
> Implement a local stub just avoiding to call down to
> irq_chip_eoi_parent() if d->parent_data is not set.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: stable@vger.kernel.org
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH] pinctrl: qcom: Guard irq_eoi()

[Bjorn Andersson]

On Fri 06 Mar 04:12 PST 2020, Linus Walleij wrote:

> In the commit setting up the qcom/msm pin controller to
> be hierarchical some callbacks were careful to check that
> d->parent_data on irq_data was valid before calling the
> parent function, however irq_chip_eoi_parent() was called
> unconditionally which doesn't work with elder Qualcomm
> platforms such as APQ8060.
> 
> When the drivers/mfd/qcom-pm8xxx.c driver calls
> chained_irq_exit() that call will in turn call chip->irq_eoi()
> which is set to irq_chip_eoi_parent() by default on a
> hierachical IRQ chip, and the parent is pinctrl-msm.c
> so that will in turn unconditionally call
> irq_chip_eoi_parent() again, but its parent is invalid
> so we get the following crash:
> 
>  Unnable to handle kernel NULL pointer dereference at
>  virtual address 00000010
>  pgd = (ptrval)
>  [00000010] *pgd=00000000
>  Internal error: Oops: 5 [#1] PREEMPT SMP ARM
>  (...)
>  PC is at irq_chip_eoi_parent+0x4/0x10
>  LR is at pm8xxx_irq_handler+0x1b4/0x2d8
> 
> Implement a local stub just avoiding to call down to
> irq_chip_eoi_parent() if d->parent_data is not set.
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: stable@vger.kernel.org
> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 9a8daa256a32..511f596cf2c3 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -828,6 +828,12 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
>  	msm_gpio_irq_clear_unmask(d, false);
>  }
>  
> +static void msm_gpio_irq_eoi(struct irq_data *d)

I find it odd that the pinctrl-msm driver would be the only place that
needs this. But let's start with this.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> +{
> +	if (d->parent_data)
> +		irq_chip_eoi_parent(d);
> +}
> +
>  static void msm_gpio_irq_ack(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -1104,7 +1110,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
>  	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
>  	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> -	pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
> +	pctrl->irq_chip.irq_eoi = msm_gpio_irq_eoi;
>  	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
>  	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
>  	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
> -- 
> 2.24.1
> 

Re: [PATCH] pinctrl: qcom: Guard irq_eoi()

[Marc Zyngier]

On 2020-03-07 01:06, Bjorn Andersson wrote:
> On Fri 06 Mar 04:12 PST 2020, Linus Walleij wrote:
> 
>> In the commit setting up the qcom/msm pin controller to
>> be hierarchical some callbacks were careful to check that
>> d->parent_data on irq_data was valid before calling the
>> parent function, however irq_chip_eoi_parent() was called
>> unconditionally which doesn't work with elder Qualcomm
>> platforms such as APQ8060.
>> 
>> When the drivers/mfd/qcom-pm8xxx.c driver calls
>> chained_irq_exit() that call will in turn call chip->irq_eoi()
>> which is set to irq_chip_eoi_parent() by default on a
>> hierachical IRQ chip, and the parent is pinctrl-msm.c
>> so that will in turn unconditionally call
>> irq_chip_eoi_parent() again, but its parent is invalid
>> so we get the following crash:
>> 
>>  Unnable to handle kernel NULL pointer dereference at
>>  virtual address 00000010
>>  pgd = (ptrval)
>>  [00000010] *pgd=00000000
>>  Internal error: Oops: 5 [#1] PREEMPT SMP ARM
>>  (...)
>>  PC is at irq_chip_eoi_parent+0x4/0x10
>>  LR is at pm8xxx_irq_handler+0x1b4/0x2d8
>> 
>> Implement a local stub just avoiding to call down to
>> irq_chip_eoi_parent() if d->parent_data is not set.
>> 
>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Cc: Lina Iyer <ilina@codeaurora.org>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Stephen Boyd <swboyd@chromium.org>
>> Cc: stable@vger.kernel.org
>> Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>>  drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c 
>> b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 9a8daa256a32..511f596cf2c3 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -828,6 +828,12 @@ static void msm_gpio_irq_unmask(struct irq_data 
>> *d)
>>  	msm_gpio_irq_clear_unmask(d, false);
>>  }
>> 
>> +static void msm_gpio_irq_eoi(struct irq_data *d)
> 
> I find it odd that the pinctrl-msm driver would be the only place that
> needs this. But let's start with this.

This driver does something very odd: depending on the SoC and/or the
configuration, some interrupts are either forwarded to a parent
interrupt controller, or terminated at GPIO level for some reason
(probably because it is then signaled as a chained interrupt, or
somehow triggers something else locally). In the latter case, there is
obviously no interrupt parent to talk about.

To my knowledge, this is the only example of such an exotic design
in the tree. Please, let's make sure it stays that way... :-/

         M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] pinctrl: qcom: Guard irq_eoi()

[Linus Walleij]

In the commit setting up the qcom/msm pin controller to
be hierarchical some callbacks were careful to check that
d->parent_data on irq_data was valid before calling the
parent function, however irq_chip_eoi_parent() was called
unconditionally which doesn't work with elder Qualcomm
platforms such as APQ8060.

When the drivers/mfd/qcom-pm8xxx.c driver calls
chained_irq_exit() that call will in turn call chip->irq_eoi()
which is set to irq_chip_eoi_parent() by default on a
hierachical IRQ chip, and the parent is pinctrl-msm.c
so that will in turn unconditionally call
irq_chip_eoi_parent() again, but its parent is invalid
so we get the following crash:

 Unnable to handle kernel NULL pointer dereference at
 virtual address 00000010
 pgd = (ptrval)
 [00000010] *pgd=00000000
 Internal error: Oops: 5 [#1] PREEMPT SMP ARM
 (...)
 PC is at irq_chip_eoi_parent+0x4/0x10
 LR is at pm8xxx_irq_handler+0x1b4/0x2d8

Implement a local stub just avoiding to call down to
irq_chip_eoi_parent() if d->parent_data is not set.

Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: stable@vger.kernel.org
Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 9a8daa256a32..511f596cf2c3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -828,6 +828,12 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 	msm_gpio_irq_clear_unmask(d, false);
 }
 
+static void msm_gpio_irq_eoi(struct irq_data *d)
+{
+	if (d->parent_data)
+		irq_chip_eoi_parent(d);
+}
+
 static void msm_gpio_irq_ack(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -1104,7 +1110,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
-	pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
+	pctrl->irq_chip.irq_eoi = msm_gpio_irq_eoi;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
-- 
2.24.1