| Message ID | 20190222130840.9373-2-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] i2c: designware: Do not allow i2c_dw_xfer() calls while suspended | expand |
On Fri, Feb 22, 2019 at 02:08:40PM +0100, Hans de Goede wrote: > On most Intel Bay- and Cherry-Trail systems the PMIC is connected over I2C > and the PMIC is accessed through various means by the _PS0 and _PS3 ACPI > methods (power on / off methods) of various devices. > > This leads to suspend/resume ordering problems where a device may be > resumed and get its _PS0 method executed before the I2C controller is > resumed. On Cherry Trail this leads to errors like these: > > i2c_designware 808622C1:06: controller timed out > ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] > ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR > video LNXVIDEO:00: Failed to change power state to D0 > > But on Bay Trail this caused I2C reads to seem to succeed, but they end > up returning wrong data, which ends up getting written back by the typical > read-modify-write cycle done to turn on various power-resources. > > Debugging the problems caused by this silent data corruption is quite > nasty. This commit adds a check which disallows i2c_dw_xfer() calls to > happen until the controller's resume method has completed. > > Which turns the silent data corruption into getting these errors in > dmesg instead: > > i2c_designware 80860F41:04: Error i2c_dw_xfer call while suspended > ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] > ACPI Error: Method parse/execution failed \_SB.PCI0.GFX0._PS0, AE_ERROR > > Which is much better. > > Note the above errors are an example of issues which this patch will > help to debug, the actual fix requires fixing the suspend order and > this has been fixed by a different commit. > > Note the setting / clearing of the suspended flag in the suspend / resume > methods is NOT protected by i2c_lock_bus(). This is intentional as these > methods get called from i2c_dw_xfer() (through pm_runtime_get/put) a nd > i2c_dw_xfer() is called with the i2c_bus_lock held, so otherwise we would > deadlock. This means that there is a theoretical race between a non runtime > suspend and the suspended check in i2c_dw_xfer(), this is not a problem > since normally we should not hit the race and this check is primarily a > debugging tool so hitting the check if there are suspend/resume ordering > problems does not need to be 100% reliable. Patch itself looks good to me, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> though I'm wondering if this suspended flag can be tracked by PM runtime core since we have a lot of drivers doing this and more coming (at least I have to do the same in 8250). > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Change error return to -ESHUTDOWN > -Add a blurb to the commit message to mention that the errors in the commitmsg > are examples of errors which will be easier to debug with this change and > that this change does not actually fixes these errors > -Add a blurb to the commit message about why i2c_lock_bus() is not called > from the suspend/resume methods > --- > drivers/i2c/busses/i2c-designware-core.h | 2 ++ > drivers/i2c/busses/i2c-designware-master.c | 6 ++++++ > drivers/i2c/busses/i2c-designware-pcidrv.c | 7 ++++++- > drivers/i2c/busses/i2c-designware-platdrv.c | 3 +++ > 4 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h > index b4a0b2b99a78..6b4ef1d38fb2 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -215,6 +215,7 @@ > * @disable_int: function to disable all interrupts > * @init: function to initialize the I2C hardware > * @mode: operation mode - DW_IC_MASTER or DW_IC_SLAVE > + * @suspended: set to true if the controller is suspended > * > * HCNT and LCNT parameters can be used if the platform knows more accurate > * values than the one computed based only on the input clock frequency. > @@ -270,6 +271,7 @@ struct dw_i2c_dev { > int (*set_sda_hold_time)(struct dw_i2c_dev *dev); > int mode; > struct i2c_bus_recovery_info rinfo; > + bool suspended; > }; > > #define ACCESS_SWAP 0x00000001 > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c > index 8d1bc44d2530..bb8e3f149979 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -426,6 +426,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > pm_runtime_get_sync(dev->dev); > > + if (dev->suspended) { > + dev_err(dev->dev, "Error %s call while suspended\n", __func__); > + ret = -ESHUTDOWN; > + goto done_nolock; > + } > + > reinit_completion(&dev->cmd_complete); > dev->msgs = msgs; > dev->msgs_num = num; > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c > index d50f80487214..76810deb2de6 100644 > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > @@ -176,6 +176,7 @@ static int i2c_dw_pci_suspend(struct device *dev) > struct pci_dev *pdev = to_pci_dev(dev); > struct dw_i2c_dev *i_dev = pci_get_drvdata(pdev); > > + i_dev->suspended = true; > i_dev->disable(i_dev); > > return 0; > @@ -185,8 +186,12 @@ static int i2c_dw_pci_resume(struct device *dev) > { > struct pci_dev *pdev = to_pci_dev(dev); > struct dw_i2c_dev *i_dev = pci_get_drvdata(pdev); > + int ret; > > - return i_dev->init(i_dev); > + ret = i_dev->init(i_dev); > + i_dev->suspended = false; > + > + return ret; > } > #endif > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 9eaac3be1f63..ead5e7de3e4d 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -454,6 +454,8 @@ static int dw_i2c_plat_suspend(struct device *dev) > { > struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); > > + i_dev->suspended = true; > + > if (i_dev->shared_with_punit) > return 0; > > @@ -471,6 +473,7 @@ static int dw_i2c_plat_resume(struct device *dev) > i2c_dw_prepare_clk(i_dev, true); > > i_dev->init(i_dev); > + i_dev->suspended = false; > > return 0; > } > -- > 2.20.1 >
Hi, On 2/22/19 3:56 PM, Andy Shevchenko wrote: > On Fri, Feb 22, 2019 at 02:08:40PM +0100, Hans de Goede wrote: >> On most Intel Bay- and Cherry-Trail systems the PMIC is connected over I2C >> and the PMIC is accessed through various means by the _PS0 and _PS3 ACPI >> methods (power on / off methods) of various devices. >> >> This leads to suspend/resume ordering problems where a device may be >> resumed and get its _PS0 method executed before the I2C controller is >> resumed. On Cherry Trail this leads to errors like these: >> >> i2c_designware 808622C1:06: controller timed out >> ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] >> ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR >> video LNXVIDEO:00: Failed to change power state to D0 >> >> But on Bay Trail this caused I2C reads to seem to succeed, but they end >> up returning wrong data, which ends up getting written back by the typical >> read-modify-write cycle done to turn on various power-resources. >> >> Debugging the problems caused by this silent data corruption is quite >> nasty. This commit adds a check which disallows i2c_dw_xfer() calls to >> happen until the controller's resume method has completed. >> >> Which turns the silent data corruption into getting these errors in >> dmesg instead: >> >> i2c_designware 80860F41:04: Error i2c_dw_xfer call while suspended >> ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] >> ACPI Error: Method parse/execution failed \_SB.PCI0.GFX0._PS0, AE_ERROR >> >> Which is much better. >> >> Note the above errors are an example of issues which this patch will >> help to debug, the actual fix requires fixing the suspend order and >> this has been fixed by a different commit. >> >> Note the setting / clearing of the suspended flag in the suspend / resume >> methods is NOT protected by i2c_lock_bus(). This is intentional as these >> methods get called from i2c_dw_xfer() (through pm_runtime_get/put) a nd >> i2c_dw_xfer() is called with the i2c_bus_lock held, so otherwise we would >> deadlock. This means that there is a theoretical race between a non runtime >> suspend and the suspended check in i2c_dw_xfer(), this is not a problem >> since normally we should not hit the race and this check is primarily a >> debugging tool so hitting the check if there are suspend/resume ordering >> problems does not need to be 100% reliable. > > Patch itself looks good to me, > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > though I'm wondering if this suspended flag can be tracked by PM runtime core > since we have a lot of drivers doing this and more coming (at least I have to > do the same in 8250). We tried using device->power.is_suspended but there were various issues with that. I do not remember from the top of my head what the problem with that approach was though. I vaguely recollect using it breaking lots of things :) Regards, Hans > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Change error return to -ESHUTDOWN >> -Add a blurb to the commit message to mention that the errors in the commitmsg >> are examples of errors which will be easier to debug with this change and >> that this change does not actually fixes these errors >> -Add a blurb to the commit message about why i2c_lock_bus() is not called >> from the suspend/resume methods >> --- >> drivers/i2c/busses/i2c-designware-core.h | 2 ++ >> drivers/i2c/busses/i2c-designware-master.c | 6 ++++++ >> drivers/i2c/busses/i2c-designware-pcidrv.c | 7 ++++++- >> drivers/i2c/busses/i2c-designware-platdrv.c | 3 +++ >> 4 files changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h >> index b4a0b2b99a78..6b4ef1d38fb2 100644 >> --- a/drivers/i2c/busses/i2c-designware-core.h >> +++ b/drivers/i2c/busses/i2c-designware-core.h >> @@ -215,6 +215,7 @@ >> * @disable_int: function to disable all interrupts >> * @init: function to initialize the I2C hardware >> * @mode: operation mode - DW_IC_MASTER or DW_IC_SLAVE >> + * @suspended: set to true if the controller is suspended >> * >> * HCNT and LCNT parameters can be used if the platform knows more accurate >> * values than the one computed based only on the input clock frequency. >> @@ -270,6 +271,7 @@ struct dw_i2c_dev { >> int (*set_sda_hold_time)(struct dw_i2c_dev *dev); >> int mode; >> struct i2c_bus_recovery_info rinfo; >> + bool suspended; >> }; >> >> #define ACCESS_SWAP 0x00000001 >> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c >> index 8d1bc44d2530..bb8e3f149979 100644 >> --- a/drivers/i2c/busses/i2c-designware-master.c >> +++ b/drivers/i2c/busses/i2c-designware-master.c >> @@ -426,6 +426,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >> >> pm_runtime_get_sync(dev->dev); >> >> + if (dev->suspended) { >> + dev_err(dev->dev, "Error %s call while suspended\n", __func__); >> + ret = -ESHUTDOWN; >> + goto done_nolock; >> + } >> + >> reinit_completion(&dev->cmd_complete); >> dev->msgs = msgs; >> dev->msgs_num = num; >> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c >> index d50f80487214..76810deb2de6 100644 >> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c >> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c >> @@ -176,6 +176,7 @@ static int i2c_dw_pci_suspend(struct device *dev) >> struct pci_dev *pdev = to_pci_dev(dev); >> struct dw_i2c_dev *i_dev = pci_get_drvdata(pdev); >> >> + i_dev->suspended = true; >> i_dev->disable(i_dev); >> >> return 0; >> @@ -185,8 +186,12 @@ static int i2c_dw_pci_resume(struct device *dev) >> { >> struct pci_dev *pdev = to_pci_dev(dev); >> struct dw_i2c_dev *i_dev = pci_get_drvdata(pdev); >> + int ret; >> >> - return i_dev->init(i_dev); >> + ret = i_dev->init(i_dev); >> + i_dev->suspended = false; >> + >> + return ret; >> } >> #endif >> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c >> index 9eaac3be1f63..ead5e7de3e4d 100644 >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >> @@ -454,6 +454,8 @@ static int dw_i2c_plat_suspend(struct device *dev) >> { >> struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); >> >> + i_dev->suspended = true; >> + >> if (i_dev->shared_with_punit) >> return 0; >> >> @@ -471,6 +473,7 @@ static int dw_i2c_plat_resume(struct device *dev) >> i2c_dw_prepare_clk(i_dev, true); >> >> i_dev->init(i_dev); >> + i_dev->suspended = false; >> >> return 0; >> } >> -- >> 2.20.1 >> >
> > though I'm wondering if this suspended flag can be tracked by PM runtime core > > since we have a lot of drivers doing this and more coming (at least I have to > > do the same in 8250). > > We tried using device->power.is_suspended but there were various issues > with that. I do not remember from the top of my head what the problem with > that approach was though. I vaguely recollect using it breaking lots of > things :) Let's benefit again that we do development in public :D The patches are here: http://patchwork.ozlabs.org/project/linux-i2c/list/?series=83383&state=* The mail thread is: [PATCH v2 0/9] i2c: move handling of suspended adapters to the core
On Fri, Feb 22, 2019 at 02:08:40PM +0100, Hans de Goede wrote: > On most Intel Bay- and Cherry-Trail systems the PMIC is connected over I2C > and the PMIC is accessed through various means by the _PS0 and _PS3 ACPI > methods (power on / off methods) of various devices. > > This leads to suspend/resume ordering problems where a device may be > resumed and get its _PS0 method executed before the I2C controller is > resumed. On Cherry Trail this leads to errors like these: > > i2c_designware 808622C1:06: controller timed out > ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] > ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR > video LNXVIDEO:00: Failed to change power state to D0 > > But on Bay Trail this caused I2C reads to seem to succeed, but they end > up returning wrong data, which ends up getting written back by the typical > read-modify-write cycle done to turn on various power-resources. > > Debugging the problems caused by this silent data corruption is quite > nasty. This commit adds a check which disallows i2c_dw_xfer() calls to > happen until the controller's resume method has completed. > > Which turns the silent data corruption into getting these errors in > dmesg instead: > > i2c_designware 80860F41:04: Error i2c_dw_xfer call while suspended > ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] > ACPI Error: Method parse/execution failed \_SB.PCI0.GFX0._PS0, AE_ERROR > > Which is much better. > > Note the above errors are an example of issues which this patch will > help to debug, the actual fix requires fixing the suspend order and > this has been fixed by a different commit. > > Note the setting / clearing of the suspended flag in the suspend / resume > methods is NOT protected by i2c_lock_bus(). This is intentional as these > methods get called from i2c_dw_xfer() (through pm_runtime_get/put) a nd > i2c_dw_xfer() is called with the i2c_bus_lock held, so otherwise we would > deadlock. This means that there is a theoretical race between a non runtime > suspend and the suspended check in i2c_dw_xfer(), this is not a problem > since normally we should not hit the race and this check is primarily a > debugging tool so hitting the check if there are suspend/resume ordering > problems does not need to be 100% reliable. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Applied to for-next, thanks! For the record, a checkpatch check which I think is not much of a problem here, but still wanted to let you know: CHECKPATCH CHECK: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384 #78: FILE: drivers/i2c/busses/i2c-designware-core.h:274: + bool suspended; ^
On Fri, Feb 22, 2019 at 06:06:52PM +0100, Wolfram Sang wrote: > > > > though I'm wondering if this suspended flag can be tracked by PM runtime core > > > since we have a lot of drivers doing this and more coming (at least I have to > > > do the same in 8250). > > > > We tried using device->power.is_suspended but there were various issues > > with that. I do not remember from the top of my head what the problem with > > that approach was though. I vaguely recollect using it breaking lots of > > things :) > > Let's benefit again that we do development in public :D > > The patches are here: > > http://patchwork.ozlabs.org/project/linux-i2c/list/?series=83383&state=* > > The mail thread is: > > [PATCH v2 0/9] i2c: move handling of suspended adapters to the core A-ha, thank you!
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index b4a0b2b99a78..6b4ef1d38fb2 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -215,6 +215,7 @@ * @disable_int: function to disable all interrupts * @init: function to initialize the I2C hardware * @mode: operation mode - DW_IC_MASTER or DW_IC_SLAVE + * @suspended: set to true if the controller is suspended * * HCNT and LCNT parameters can be used if the platform knows more accurate * values than the one computed based only on the input clock frequency. @@ -270,6 +271,7 @@ struct dw_i2c_dev { int (*set_sda_hold_time)(struct dw_i2c_dev *dev); int mode; struct i2c_bus_recovery_info rinfo; + bool suspended; }; #define ACCESS_SWAP 0x00000001 diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 8d1bc44d2530..bb8e3f149979 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -426,6 +426,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) pm_runtime_get_sync(dev->dev); + if (dev->suspended) { + dev_err(dev->dev, "Error %s call while suspended\n", __func__); + ret = -ESHUTDOWN; + goto done_nolock; + } + reinit_completion(&dev->cmd_complete); dev->msgs = msgs; dev->msgs_num = num; diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index d50f80487214..76810deb2de6 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -176,6 +176,7 @@ static int i2c_dw_pci_suspend(struct device *dev) struct pci_dev *pdev = to_pci_dev(dev); struct dw_i2c_dev *i_dev = pci_get_drvdata(pdev); + i_dev->suspended = true; i_dev->disable(i_dev); return 0; @@ -185,8 +186,12 @@ static int i2c_dw_pci_resume(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct dw_i2c_dev *i_dev = pci_get_drvdata(pdev); + int ret; - return i_dev->init(i_dev); + ret = i_dev->init(i_dev); + i_dev->suspended = false; + + return ret; } #endif diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 9eaac3be1f63..ead5e7de3e4d 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -454,6 +454,8 @@ static int dw_i2c_plat_suspend(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); + i_dev->suspended = true; + if (i_dev->shared_with_punit) return 0; @@ -471,6 +473,7 @@ static int dw_i2c_plat_resume(struct device *dev) i2c_dw_prepare_clk(i_dev, true); i_dev->init(i_dev); + i_dev->suspended = false; return 0; }
On most Intel Bay- and Cherry-Trail systems the PMIC is connected over I2C and the PMIC is accessed through various means by the _PS0 and _PS3 ACPI methods (power on / off methods) of various devices. This leads to suspend/resume ordering problems where a device may be resumed and get its _PS0 method executed before the I2C controller is resumed. On Cherry Trail this leads to errors like these: i2c_designware 808622C1:06: controller timed out ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR video LNXVIDEO:00: Failed to change power state to D0 But on Bay Trail this caused I2C reads to seem to succeed, but they end up returning wrong data, which ends up getting written back by the typical read-modify-write cycle done to turn on various power-resources. Debugging the problems caused by this silent data corruption is quite nasty. This commit adds a check which disallows i2c_dw_xfer() calls to happen until the controller's resume method has completed. Which turns the silent data corruption into getting these errors in dmesg instead: i2c_designware 80860F41:04: Error i2c_dw_xfer call while suspended ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion] ACPI Error: Method parse/execution failed \_SB.PCI0.GFX0._PS0, AE_ERROR Which is much better. Note the above errors are an example of issues which this patch will help to debug, the actual fix requires fixing the suspend order and this has been fixed by a different commit. Note the setting / clearing of the suspended flag in the suspend / resume methods is NOT protected by i2c_lock_bus(). This is intentional as these methods get called from i2c_dw_xfer() (through pm_runtime_get/put) a nd i2c_dw_xfer() is called with the i2c_bus_lock held, so otherwise we would deadlock. This means that there is a theoretical race between a non runtime suspend and the suspended check in i2c_dw_xfer(), this is not a problem since normally we should not hit the race and this check is primarily a debugging tool so hitting the check if there are suspend/resume ordering problems does not need to be 100% reliable. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Change error return to -ESHUTDOWN -Add a blurb to the commit message to mention that the errors in the commitmsg are examples of errors which will be easier to debug with this change and that this change does not actually fixes these errors -Add a blurb to the commit message about why i2c_lock_bus() is not called from the suspend/resume methods --- drivers/i2c/busses/i2c-designware-core.h | 2 ++ drivers/i2c/busses/i2c-designware-master.c | 6 ++++++ drivers/i2c/busses/i2c-designware-pcidrv.c | 7 ++++++- drivers/i2c/busses/i2c-designware-platdrv.c | 3 +++ 4 files changed, 17 insertions(+), 1 deletion(-)