Re: [PATCH v2] i2c: designware: Do not allow i2c_dw_xfer() calls while suspended

[Hans de Goede <hdegoede@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> > ---
> > 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