Re: pinctrl-cherryview regression in linux-next on preproduction Braswell

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

[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]

Hi,

On 1/3/22 13:34, Jarkko Nikula wrote:
> Hi
> 
> On 1/3/22 12:42, Hans de Goede wrote:
>> Hi Jarkko,
>>
>> On 1/3/22 10:42, Jarkko Nikula wrote:
>>> Hi
>>>
>>> We have a Braswell based preproduction HW. I noticed linux-next tag next-20211224 doesn't boot on it due to following error:
>>>
>>> [   34.674271] cherryview-pinctrl INT33FF:00: interrupt on unused interrupt line 0
>>> [   34.682476] cherryview-pinctrl INT33FF:00: interrupt on unused interrupt line 0
>>> [   34.690681] cherryview-pinctrl INT33FF:00: interrupt on unused interrupt line 0
>>> ...
>>>
>>> Linux v5.16-rc8 is ok. I found the following commit to be reason for the regression:
>>>
>>> bdfbef2d29dc ("pinctrl: cherryview: Don't use selection 0 to mark an interrupt line as unused")
>>
>> Thank you for the timely headsup about this.
>>
>> I assume that you have tried a revert (if necessary including reverting some
>> of the follow ups) and that reverting the patch you point to fixes the
>> issue, right ?
>>
> Yes since linux-next has only these three commits below to pinctrl-cherryview.c that are not in v5.16-rc8 I reverted them one by one. I often try these kind of simple steps before going to more labor git bisect :-)
> 
> db1b2a8caf5b pinctrl: cherryview: Use temporary variable for struct device
> 07199dbf8cae pinctrl: cherryview: Do not allow the same interrupt line to be used by 2 pins
> bdfbef2d29dc pinctrl: cherryview: Don't use selection 0 to mark an interrupt line as unused
> 
> I also double checked by checking out to bdfbef2d29dc and tested that commit and it reverted.
> 
>> Can you try the 2 attached patches *one at a time* ? :
>>
>> 1. Restores the old behavior of just triggering hwirq 0 of
>> the pincontroller-domain when we don't know the mapping
>>
> Patch 1 does work and here's the output from modified error print:
> 
> [   13.550781] cherryview-pinctrl INT33FF:00: interrupt on unmapped interrupt line 0
> 
> If you want to go with patch 1 you may add my
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
>> 2. Restores the old behavior which allows chv_gpio_irq_startup()
>> to overwrite the interrupt-line to pin mapping if the current
>> mapping points to pin 0
>>
> Patch 2 alone doesn't:
> 
> [   24.977116] cherryview-pinctrl INT33FF:00: interrupt on unused interrupt line 0
> [   24.985314] cherryview-pinctrl INT33FF:00: interrupt on unused interrupt line 0
> [   24.993513] cherryview-pinctrl INT33FF:00: interrupt on unused interrupt line 0
> ...

Ok, that is good news actually, I was already hoping that patch 1
would fix things.

>> Both of these restore old behavior caused by a mapping-table
>> entry containing 0 meaning both unset as well as HWIRQ0
>> before the patch in question.
>>
>> If applying them one at a time does not help, please also try with
>> both applied.
>>
>> These 2 patches should apply cleanly on top of linux-next.
>>
>> If patch 2 fixes things it would be interesting if you can grab a
>> dmesg with "pinctrl-cherryview.dyndbg" added to the command line
>> (with the patched kernel).
>>
> With both applied it does work:
> 
> # dmesg |grep cherryview-pinctrl
> [   15.465425] cherryview-pinctrl INT33FF:00: interrupt on unmapped interrupt line 0
> [   16.562282] cherryview-pinctrl INT33FF:03: using interrupt line 0 for pin 81
> [   17.824905] cherryview-pinctrl INT33FF:02: using interrupt line 0 for pin 22
> [   17.835757] cherryview-pinctrl INT33FF:03: using interrupt line 2 for pin 77
> [   17.850170] cherryview-pinctrl INT33FF:00: using interrupt line 0 for pin 76

Oh, that is actually interesting, this is a per gpio controller thing, so if we
filter on the controller then we end up with:

[   15.465425] cherryview-pinctrl INT33FF:00: interrupt on unmapped interrupt line 0
[   17.850170] cherryview-pinctrl INT33FF:00: using interrupt line 0 for pin 76

So we do eventually get an IRQ request for a pin using the GPIO controller
internal interrupt-line 0. But the IRQ triggers at least once before then and
even though we haven't set a handler yet, calling generic_handle_irq for the
GPIO-chips irqdomain, offset 0 IRQ does manage to silence the interrupt.

I've been tracing this through the kernel code and AFAICT we end up in:

arch/x86/kernel/irq.c: ack_bad_irq() in this case:

Which means that this message should show up in dmesg:

        if (printk_ratelimit())
                pr_err("unexpected IRQ trap at vector %02x\n", irq);

Can you confirm this? Also can you share the full dmesg output of this
device (with both patches, with dyndbg option) ?

###

Note what we are seeing here is basically a spurious IRQ. Except on a few
known broken devices the cherryview pinctrl code relies on the BIOS having
configured things so that there are no spurious IRQs. I've attached a
quick hack which activates the workaround for known broken devices
unconditionally. This replace my previous 2 patches. I expect this to
fix things too.

If you can make some time to give this one a test too that would be
great, then we have some options on how to fix this :)

Regards,

Hans


[-- Attachment #2: 0001-pinctrl-cherryview-Hack-to-try-and-workaround-linux-.patch --]
[-- Type: text/x-patch, Size: 1188 bytes --]

From 48c739b102051b71a9d4de2d128f4f2633cd668d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 3 Jan 2022 17:31:36 +0100
Subject: [PATCH] pinctrl: cherryview: Hack to try and workaround linux-next
 regression

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 683b95e9639a..2ee933c6304a 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1558,13 +1558,13 @@ static int chv_gpio_irq_init_hw(struct gpio_chip *chip)
 	 *
 	 * See also https://bugzilla.kernel.org/show_bug.cgi?id=197953.
 	 */
-	if (!pctrl->chip.irq.init_valid_mask) {
+//	if (!pctrl->chip.irq.init_valid_mask) {
 		/*
 		 * Mask all interrupts the community is able to generate
 		 * but leave the ones that can only generate GPEs unmasked.
 		 */
 		chv_pctrl_writel(pctrl, CHV_INTMASK, GENMASK(31, community->nirqs));
-	}
+//	}
 
 	/* Clear all interrupts */
 	chv_pctrl_writel(pctrl, CHV_INTSTAT, 0xffff);
-- 
2.33.1