Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use

From: Marek Vasut <marex@denx.de>
To: Thomas Gleixner <tglx@linutronix.de>,
	linux-arm-kernel@lists.infradead.org
Cc: Marc Zyngier <marc.zyngier@arm.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Fabien Dessenne <fabien.dessenne@st.com>
Subject: Re: [PATCH] [RFC] genirq: Check irq_data_get_irq_chip() return value before use
Date: Sun, 10 May 2020 16:49:20 +0200	[thread overview]
Message-ID: <d3b76d43-675b-c699-06fa-aab448504f25@denx.de> (raw)
In-Reply-To: <875zd7e7m0.fsf@nanos.tec.linutronix.de>

On 5/7/20 11:51 PM, Thomas Gleixner wrote:
> Marek,
> 
> Marek Vasut <marex@denx.de> writes:
>> On 5/7/20 7:30 PM, Thomas Gleixner wrote:
>>> Marek Vasut <marex@denx.de> writes:
>>>> NOTE: I don't know whether this is a correct fix. Maybe the
>>>>       irq_data_get_irq_chip() should never return NULL, and
>>>>       I have some other issue?
>>>
>>> What's the callchain?
>>
>> Hmm, I'm currently unable to replicate it on linux-next, but on 5.4.39 I
>> get what's at the end of the email.
>>
>> On next I just noticed I get i2c: Transfer while suspended, which is
>> what I suspect would be the real root cause of my problem, and why
>> irq_data_get_irq_chip() returns NULL?
> 
> Looks like.
> 
>>  Unable to handle kernel NULL pointer dereference at virtual address 00000070
>>  PC is at __irq_get_irqchip_state+0x4/0x30
>>  LR is at __synchronize_hardirq+0x7c/0xe8
>>  [<c0166758>] (__irq_get_irqchip_state) from [<c0166800>] (__synchronize_hardirq+0x7c/0xe8)
>>  [<c0166800>] (__synchronize_hardirq) from [<c01668f0>] (synchronize_irq+0x2c/0x9c)
>>  [<c01668f0>] (synchronize_irq) from [<c016cba0>] (suspend_device_irqs+0xd8/0xf4)
>>  [<c016cba0>] (suspend_device_irqs) from [<c04e3388>] (dpm_suspend_noirq+0x18/0x194)
>>  [<c04e3388>] (dpm_suspend_noirq) from [<c015f174>] (suspend_devices_and_enter+0x170/0x514)
>>  [<c015f174>] (suspend_devices_and_enter) from [<c015f6f4>] (pm_suspend+0x1dc/0x278)
>>  [<c015f6f4>] (pm_suspend) from [<c015e100>] (state_store+0x9c/0xcc)
>>  [<c015e100>] (state_store) from [<c029a424>] (kernfs_fop_write+0x124/0x1e0)
>>  [<c029a424>] (kernfs_fop_write) from [<c02301c8>] (__vfs_write+0x2c/0xe8)
>>  [<c02301c8>] (__vfs_write) from [<c0231658>] (vfs_write+0x98/0xbc)
>>  [<c0231658>] (vfs_write) from [<c02317fc>] (ksys_write+0x74/0xc4)
>>  [<c02317fc>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
> 
> I assume that the i2c controller in question tears down the interrupt on
> suspend. The changelog of that i2c driver should give you a few hints.

All right, so I found out the root cause is already fixed in next, and
just needs to be backported to stable. I'll ping the patch author about
that.

It's this patch:
69269446ccbf ("mailbox: stm32-ipcc: Update wakeup management")

I also need to revisit the regulator suspend topic next, that seems to
be a separate issue after all.

Sorry for the noise.

That said, do you want to take this patch to add the missing check
anyway or is there a reason the check is missing ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel