i2c: qup: Add shutdown method

Message ID 1516138534-30842-1-git-send-email-austinwc@codeaurora.org
State Changes Requested
Headers show
Series
  • i2c: qup: Add shutdown method
Related show

Commit Message

Austin Christ Jan. 16, 2018, 9:35 p.m.
This shutdown method disables I2C to avoid corrupting a new kernel
started with kexec.

Signed-off-by: Austin Christ <austinwc@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson Feb. 2, 2018, 11:36 p.m. | #1
On Tue 16 Jan 13:35 PST 2018, Austin Christ wrote:

> This shutdown method disables I2C to avoid corrupting a new kernel
> started with kexec.
> 

Can you elaborate on the issue you're seeing here? In what way is the
i2c-qup driver special, will there be similar patches for all other
drivers in the system?

Regards,
Bjorn
Austin Christ March 14, 2018, 8:05 p.m. | #2
This patch is designed to address a possible issue where an interrupt is 
fired but not yet handled by the i2c-qup driver and the kernel goes down 
as kexec prepares to start a secondary kernel. In this case it is 
possible the interrupt will be left unhandled.

This is not unique to the i2c-qup driver, similar patches have gone into 
other drivers such as the following for the arm-smmu-v3 driver.

commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
Author: Nate Watterson <nwatters@codeaurora.org>
Date:   Thu Jun 29 18:18:15 2017 -0400

     iommu/arm-smmu-v3: Implement shutdown method

     The shutdown method disables the SMMU to avoid corrupting a new kernel
     started with kexec.

     Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
     Signed-off-by: Will Deacon <will.deacon@arm.com>



On 2/2/2018 4:36 PM, Bjorn Andersson wrote:
> On Tue 16 Jan 13:35 PST 2018, Austin Christ wrote:
> 
>> This shutdown method disables I2C to avoid corrupting a new kernel
>> started with kexec.
>>
> 
> Can you elaborate on the issue you're seeing here? In what way is the
> i2c-qup driver special, will there be similar patches for all other
> drivers in the system?
> 
> Regards,
> Bjorn
>
Bjorn Andersson March 14, 2018, 11:40 p.m. | #3
On Wed 14 Mar 13:05 PDT 2018, Christ, Austin wrote:

> This patch is designed to address a possible issue where an interrupt is
> fired but not yet handled by the i2c-qup driver and the kernel goes down as
> kexec prepares to start a secondary kernel. In this case it is possible the
> interrupt will be left unhandled.
> 

What's the problem with not handling this IRQ? Does the hardware end up
in some funky state? Are you worried that the kexec kernel will not
function because there's a pending I2C interrupt?

> This is not unique to the i2c-qup driver, similar patches have gone into
> other drivers such as the following for the arm-smmu-v3 driver.
> 

The only similarity I can see here is that the SMMU is shut down in
order for any ongoing memory transaction to stop, which could be analog
to ongoing BAM operations in the QUP driver. But in that case I think
the shutdown should go in the BAM, to stop any ongoing transactions.

Regards,
Bjorn

> commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
> Author: Nate Watterson <nwatters@codeaurora.org>
> Date:   Thu Jun 29 18:18:15 2017 -0400
> 
>     iommu/arm-smmu-v3: Implement shutdown method
> 
>     The shutdown method disables the SMMU to avoid corrupting a new kernel
>     started with kexec.
> 
>     Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>     Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> 
> 
> On 2/2/2018 4:36 PM, Bjorn Andersson wrote:
> > On Tue 16 Jan 13:35 PST 2018, Austin Christ wrote:
> > 
> > > This shutdown method disables I2C to avoid corrupting a new kernel
> > > started with kexec.
> > > 
> > 
> > Can you elaborate on the issue you're seeing here? In what way is the
> > i2c-qup driver special, will there be similar patches for all other
> > drivers in the system?
> > 
> > Regards,
> > Bjorn
> > 
> 
> -- 
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
> Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
Austin Christ March 19, 2018, 3:18 p.m. | #4
On 3/14/2018 5:40 PM, Bjorn Andersson wrote:
> On Wed 14 Mar 13:05 PDT 2018, Christ, Austin wrote:
> 
>> This patch is designed to address a possible issue where an interrupt is
>> fired but not yet handled by the i2c-qup driver and the kernel goes down as
>> kexec prepares to start a secondary kernel. In this case it is possible the
>> interrupt will be left unhandled.
>>
> 
> What's the problem with not handling this IRQ? Does the hardware end up
> in some funky state? Are you worried that the kexec kernel will not
> function because there's a pending I2C interrupt?
> 
The potential issue is that if an IRQ is left high, the secondary kernel
driver for I2C may enter a bad state trying to handle an interrupt that 
does not apply to its context. In the QUP driver case, it may be less of 
an issue because the probe does a soft reset of the core.

>> This is not unique to the i2c-qup driver, similar patches have gone into
>> other drivers such as the following for the arm-smmu-v3 driver.
>>
> 
> The only similarity I can see here is that the SMMU is shut down in
> order for any ongoing memory transaction to stop, which could be analog
> to ongoing BAM operations in the QUP driver. But in that case I think
> the shutdown should go in the BAM, to stop any ongoing transactions.
My point wasn't that the shutdown was addressing the same issues between 
these drivers, but that registering a shutdown function is the standard 
code path for exposing clean tear down for kexec. Kexec does not call 
remove on active drivers; it calls shutdown.

Is there any negative impact for adding this function?

> 
> Regards,
> Bjorn
> 
>> commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
>> Author: Nate Watterson <nwatters@codeaurora.org>
>> Date:   Thu Jun 29 18:18:15 2017 -0400
>>
>>      iommu/arm-smmu-v3: Implement shutdown method
>>
>>      The shutdown method disables the SMMU to avoid corrupting a new kernel
>>      started with kexec.
>>
>>      Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
>>      Signed-off-by: Will Deacon <will.deacon@arm.com>
>>
>>
>>
>> On 2/2/2018 4:36 PM, Bjorn Andersson wrote:
>>> On Tue 16 Jan 13:35 PST 2018, Austin Christ wrote:
>>>
>>>> This shutdown method disables I2C to avoid corrupting a new kernel
>>>> started with kexec.
>>>>
>>>
>>> Can you elaborate on the issue you're seeing here? In what way is the
>>> i2c-qup driver special, will there be similar patches for all other
>>> drivers in the system?
>>>
>>> Regards,
>>> Bjorn
>>>
>>
>> -- 
>> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
>> Inc.
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.
Timur Tabi March 28, 2018, 2:45 p.m. | #5
On Mon, Mar 19, 2018 at 10:18 AM, Christ, Austin
<austinwc@codeaurora.org> wrote:
>> What's the problem with not handling this IRQ? Does the hardware end up
>> in some funky state? Are you worried that the kexec kernel will not
>> function because there's a pending I2C interrupt?
>>
> The potential issue is that if an IRQ is left high, the secondary kernel
> driver for I2C may enter a bad state trying to handle an interrupt that does
> not apply to its context. In the QUP driver case, it may be less of an issue
> because the probe does a soft reset of the core.

Bjorn, do you still have any concerns about this patch?  It seems like
the right thing to do, but I want to make sure you're satisified with
it, and whether it will be accepted for 4.17.
Bjorn Andersson March 29, 2018, 5:33 a.m. | #6
On Mon 19 Mar 08:18 PDT 2018, Christ, Austin wrote:
> On 3/14/2018 5:40 PM, Bjorn Andersson wrote:
> > On Wed 14 Mar 13:05 PDT 2018, Christ, Austin wrote:
> > 
> > > This patch is designed to address a possible issue where an interrupt is
> > > fired but not yet handled by the i2c-qup driver and the kernel goes down as
> > > kexec prepares to start a secondary kernel. In this case it is possible the
> > > interrupt will be left unhandled.
> > > 
> > 
> > What's the problem with not handling this IRQ? Does the hardware end up
> > in some funky state? Are you worried that the kexec kernel will not
> > function because there's a pending I2C interrupt?
> > 
> The potential issue is that if an IRQ is left high, the secondary kernel
> driver for I2C may enter a bad state trying to handle an interrupt that does
> not apply to its context. In the QUP driver case, it may be less of an issue
> because the probe does a soft reset of the core.
> 

This is a very common issue with hardware blocks used by the bootloader,
so drivers must be implemented in such a way that they can handle this
scenario.

> > > This is not unique to the i2c-qup driver, similar patches have gone into
> > > other drivers such as the following for the arm-smmu-v3 driver.
> > > 
> > 
> > The only similarity I can see here is that the SMMU is shut down in
> > order for any ongoing memory transaction to stop, which could be analog
> > to ongoing BAM operations in the QUP driver. But in that case I think
> > the shutdown should go in the BAM, to stop any ongoing transactions.
> My point wasn't that the shutdown was addressing the same issues between
> these drivers, but that registering a shutdown function is the standard code
> path for exposing clean tear down for kexec. Kexec does not call remove on
> active drivers; it calls shutdown.
> 

We do know that there are devices out there with e.g. backlight drivers
on i2c that will be operated in order to display a splash screen in the
boot loader. As such the driver must attempt to clear this state and/or
be able to handle any spurious interrupts.

> Is there any negative impact for adding this function?
> 

This must be handled already and as such this extra code should be
totally unnecessary, and we shouldn't merge unnecessary code.

And as you say above, the i2c-qup driver does reset the core so this
should be "less of an issue", not merging this patch means that if there
is any issues left we expose these to more testing.

Regards,
Bjorn

> > 
> > Regards,
> > Bjorn
> > 
> > > commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
> > > Author: Nate Watterson <nwatters@codeaurora.org>
> > > Date:   Thu Jun 29 18:18:15 2017 -0400
> > > 
> > >      iommu/arm-smmu-v3: Implement shutdown method
> > > 
> > >      The shutdown method disables the SMMU to avoid corrupting a new kernel
> > >      started with kexec.
> > > 
> > >      Signed-off-by: Nate Watterson <nwatters@codeaurora.org>
> > >      Signed-off-by: Will Deacon <will.deacon@arm.com>
> > > 
> > > 
> > > 
> > > On 2/2/2018 4:36 PM, Bjorn Andersson wrote:
> > > > On Tue 16 Jan 13:35 PST 2018, Austin Christ wrote:
> > > > 
> > > > > This shutdown method disables I2C to avoid corrupting a new kernel
> > > > > started with kexec.
> > > > > 
> > > > 
> > > > Can you elaborate on the issue you're seeing here? In what way is the
> > > > i2c-qup driver special, will there be similar patches for all other
> > > > drivers in the system?
> > > > 
> > > > Regards,
> > > > Bjorn
> > > > 
> > > 
> > > -- 
> > > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
> > > Inc.
> > > Qualcomm Technologies, Inc. is a member of the
> > > Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
> -- 
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
> Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
Timur Tabi April 2, 2018, 9:11 p.m. | #7
On Thu, Mar 29, 2018 at 12:33 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:

> And as you say above, the i2c-qup driver does reset the core so this
> should be "less of an issue", not merging this patch means that if there
> is any issues left we expose these to more testing.

I understand that leaving an interrupt pending doesn't really have any
side-effect since the driver reset the device in the probe, although I
still think it's proper for a driver to clear this interrupt when it
shuts down.  I don't want to belabor that point.

However, I don't understand what you mean by "any issues left"?  What
issues?  What value could there be to leaving resources hanging when
the driver shuts down?
Austin Christ April 24, 2018, 4:34 p.m. | #8
Hey all,

On 4/2/2018 3:11 PM, Timur Tabi wrote:
> On Thu, Mar 29, 2018 at 12:33 AM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> 
>> And as you say above, the i2c-qup driver does reset the core so this
>> should be "less of an issue", not merging this patch means that if there
>> is any issues left we expose these to more testing.
> 
> I understand that leaving an interrupt pending doesn't really have any
> side-effect since the driver reset the device in the probe, although I
> still think it's proper for a driver to clear this interrupt when it
> shuts down.  I don't want to belabor that point.
> 
> However, I don't understand what you mean by "any issues left"?  What
> issues?  What value could there be to leaving resources hanging when
> the driver shuts down?
> 

After reviewing the patch and dependencies within the kernel, I'm 
withdrawing this change.

There is a loose coupling between PCIe and the I2C driver via the PCIe 
hotplug feature. During shutdown or kexec the PCIe driver shuts down the 
hotplug driver which issues I2C commands to turn off slot power. There 
is no explicit dependency between hotplug and I2C, so the I2C driver is 
already gone when this takes place. This can result in the kernel being 
stuck in a spinning loop of unhandled ACPI function.

Based on this and Bjorn's comments, I think the change should be dropped.

Austin

Patch

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 08f8e01..b96f01a 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -1651,12 +1651,16 @@  static int qup_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void qup_i2c_shutdown(struct platform_device *pdev)
+{
+	qup_i2c_remove(pdev);
+}
+
 #ifdef CONFIG_PM
 static int qup_i2c_pm_suspend_runtime(struct device *device)
 {
 	struct qup_i2c_dev *qup = dev_get_drvdata(device);
 
-	dev_dbg(device, "pm_runtime: suspending...\n");
 	qup_i2c_disable_clocks(qup);
 	return 0;
 }
@@ -1717,6 +1721,7 @@  static int qup_i2c_resume(struct device *device)
 static struct platform_driver qup_i2c_driver = {
 	.probe  = qup_i2c_probe,
 	.remove = qup_i2c_remove,
+	.shutdown = qup_i2c_shutdown,
 	.driver = {
 		.name = "i2c_qup",
 		.pm = &qup_i2c_qup_pm_ops,