Re: [PATCH v1] pinctrl: intel: fix unexpected interrupt

[Łukasz Bartosik <lb@xxxxxxxxxxxx>]

sob., 15 sty 2022 o 09:57 Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
napisał(a):
>
>
>
> On Friday, January 14, 2022, Lukasz Bartosik <lb@xxxxxxxxxxxx> wrote:
>>
>> From: Łukasz Bartosik <lb@xxxxxxxxxxxx>
>>
>> ASUS Chromebook C223 with Celeron N3350 crashes sometimes during
>> cold booot. Inspection of the kernel log showed that it gets into
>> an inifite loop logging the following message:
>>
>> ->handle_irq():  000000009cdb51e8, handle_bad_irq+0x0/0x251
>> ->irq_data.chip(): 000000005ec212a7, 0xffffa043009d8e7
>> ->action(): 00000
>>    IRQ_NOPROBE set
>> unexpected IRQ trap at vector 7c
>>
>> The issue happens during cold boot but only if cold boot happens
>> at most several dozen seconds after Chromebook is powered off. For
>> longer intervals between power off and power on (cold boot) the issue
>> does not reproduce. The unexpected interrupt is sourced from INT3452
>> GPIO pin which is used for SD card detect. Investigation relevealed
>> that when the interval between power off and power on (cold boot)
>> is less than several dozen seconds then values of INT3452 GPIO interrupt
>> enable and interrupt pending registers survive power off and power
>> on sequence and interrupt for SD card detect pin is enabled and pending
>> during probe of SD controller which causes the unexpected IRQ message.
>> "Intel Pentium and Celeron Processor N- and J- Series" volume 3 doc
>> mentions that GPIO interrupt enable and status registers default
>> value is 0x0.
>> The fix clears INT3452 GPIO interrupt enabled and interrupt pending
>> registers in its probe function.
>>
>
>
> Thanks for the report and patch!
>
>
> What I am afraid of is that we may lost an events from important sources, I.e. GPE and touchpad which may be crucial.
>
> In this case how do we know that no event is pending at this stage?
>

After probe is completed when an interrupt is being enabled for a pin
in intel_gpio_irq_unmask() then
an interrupt pending bit is cleared just before enabling the interrupt.
Would you please elaborate how events may be lost if they are being
explicitly cleared before enabling the interrupts ?

Another approach could be to clear only enable interrupts register and
leave pending interrupts register intact ?

>
>>
>> Fixes: 7981c0015af2 ("pinctrl: intel: Add Intel Sunrisepoint pin controller and GPIO support")
>> Signed-off-by: Łukasz Bartosik <lb@xxxxxxxxxxxx>
>> ---
>>  drivers/pinctrl/intel/pinctrl-intel.c | 46 +++++++++++++++------------
>>  1 file changed, 26 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
>> index b6ef1911c1dd..b8282d5f485e 100644
>> --- a/drivers/pinctrl/intel/pinctrl-intel.c
>> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
>> @@ -1428,6 +1428,26 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
>>         return 0;
>>  }
>>
>> +static void intel_gpio_irq_init(struct intel_pinctrl *pctrl)
>> +{
>> +       size_t i;
>> +
>> +       for (i = 0; i < pctrl->ncommunities; i++) {
>> +               const struct intel_community *community;
>> +               void __iomem *base;
>> +               unsigned int gpp;
>> +
>> +               community = &pctrl->communities[i];
>> +               base = community->regs;
>> +
>> +               for (gpp = 0; gpp < community->ngpps; gpp++) {
>> +                       /* Mask and clear all interrupts */
>> +                       writel(0, base + community->ie_offset + gpp * 4);
>> +                       writel(0xffff, base + community->is_offset + gpp * 4);
>> +               }
>> +       }
>> +}
>> +
>>  static int intel_pinctrl_probe(struct platform_device *pdev,
>>                                const struct intel_pinctrl_soc_data *soc_data)
>>  {
>> @@ -1488,6 +1508,12 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
>>                         return ret;
>>         }
>>
>> +       /*
>> +        * Make sure the interrupt lines are in a proper state before
>> +        * further configuration
>> +        */
>> +       intel_gpio_irq_init(pctrl);
>
>
>
> This should go in the corresponding callback in struct gpio_chip. ->irq_hw_init() or so.
>

I will move it into gpio_chip->irq->init_hw().

>>
>> +
>>         irq = platform_get_irq(pdev, 0);
>>         if (irq < 0)
>>                 return irq;
>> @@ -1640,26 +1666,6 @@ int intel_pinctrl_suspend_noirq(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(intel_pinctrl_suspend_noirq);
>>
>> -static void intel_gpio_irq_init(struct intel_pinctrl *pctrl)
>> -{
>> -       size_t i;
>> -
>> -       for (i = 0; i < pctrl->ncommunities; i++) {
>> -               const struct intel_community *community;
>> -               void __iomem *base;
>> -               unsigned int gpp;
>> -
>> -               community = &pctrl->communities[i];
>> -               base = community->regs;
>> -
>> -               for (gpp = 0; gpp < community->ngpps; gpp++) {
>> -                       /* Mask and clear all interrupts */
>> -                       writel(0, base + community->ie_offset + gpp * 4);
>> -                       writel(0xffff, base + community->is_offset + gpp * 4);
>> -               }
>> -       }
>> -}
>> -
>>  static bool intel_gpio_update_reg(void __iomem *reg, u32 mask, u32 value)
>>  {
>>         u32 curr, updated;
>> --
>> 2.34.1.703.g22d0c6ccf7-goog
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Best regards,
Lukasz