Re: [PATCH 1/2] i2c: designware: Lock the adapter while setting the suspended flag

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

Hi Andy,

On 2/23/22 15:52, Andy Shevchenko wrote:
> On Wed, Feb 23, 2022 at 02:48:38PM +0100, Hans de Goede wrote:
>> Lock the adapter while setting the suspended flag, to ensure that other
>> locked code always sees the change immediately, rather then possibly using
>> a stale value.
>>
>> This involves splitting the suspend/resume callbacks into separate runtime
>> and normal suspend/resume calls. This is necessary because i2c_dw_xfer()
>> will get called by the i2c-core with the adapter locked and it in turn
>> calls the runtime-resume callback through pm_runtime_get_sync().
>>
>> So the runtime versions of the suspend/resume callbacks cannot take
>> the adapter-lock. Note this patch simply makes the runtime suspend/resume
>> callbacks not deal with the suspended flag at all. During runtime the
>> pm_runtime_get_sync() from i2c_dw_xfer() will always ensure that the
>> adapter is resumed when necessary.
>>
>> The suspended flag check is only necessary to check proper suspend/resume
>> ordering during normal suspend/resume which makes the pm_runtime_get_sync()
>> call a no-op.
> 
> Both patches look good to me and seems like a nice clean up. Not sure if the
> first one should be backported (and hence have a Fixes tag).
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you for the review. As for adding a Fixes: tag, this was inspired
by an actual bug-report, but it did not help in fixing the bug, so I left
the fixes tag out.

If you or Wolfram still want to add a Fixes tag, then it should be:

Fixes: 275154155538  ("i2c: designware: Do not allow i2c_dw_xfer() calls while suspended")

Regards,

Hans




> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/i2c/busses/i2c-designware-pcidrv.c  | 31 +++++++++++++++++----
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 31 +++++++++++++++++----
>>  2 files changed, 52 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> index ef4250f8852b..9553d7075223 100644
>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> @@ -213,14 +213,30 @@ static struct dw_pci_controller dw_pci_controllers[] = {
>>  	},
>>  };
>>  
>> +static int __maybe_unused i2c_dw_pci_runtime_suspend(struct device *dev)
>> +{
>> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +	i_dev->disable(i_dev);
>> +	return 0;
>> +}
>> +
>>  static int __maybe_unused i2c_dw_pci_suspend(struct device *dev)
>>  {
>>  	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>>  
>> +	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  	i_dev->suspended = true;
>> -	i_dev->disable(i_dev);
>> +	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  
>> -	return 0;
>> +	return i2c_dw_pci_runtime_suspend(dev);
>> +}
>> +
>> +static int __maybe_unused i2c_dw_pci_runtime_resume(struct device *dev)
>> +{
>> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +	return i_dev->init(i_dev);
>>  }
>>  
>>  static int __maybe_unused i2c_dw_pci_resume(struct device *dev)
>> @@ -228,14 +244,19 @@ static int __maybe_unused i2c_dw_pci_resume(struct device *dev)
>>  	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>>  	int ret;
>>  
>> -	ret = i_dev->init(i_dev);
>> +	ret = i2c_dw_pci_runtime_resume(dev);
>> +
>> +	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  	i_dev->suspended = false;
>> +	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  
>>  	return ret;
>>  }
>>  
>> -static UNIVERSAL_DEV_PM_OPS(i2c_dw_pm_ops, i2c_dw_pci_suspend,
>> -			    i2c_dw_pci_resume, NULL);
>> +static const struct dev_pm_ops i2c_dw_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(i2c_dw_pci_suspend, i2c_dw_pci_resume)
>> +	SET_RUNTIME_PM_OPS(i2c_dw_pci_runtime_suspend, i2c_dw_pci_runtime_resume, NULL)
>> +};
>>  
>>  static int i2c_dw_pci_probe(struct pci_dev *pdev,
>>  			    const struct pci_device_id *id)
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 2bd81abc86f6..8e45f65bab73 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -368,12 +368,10 @@ static void dw_i2c_plat_complete(struct device *dev)
>>  #endif
>>  
>>  #ifdef CONFIG_PM
>> -static int dw_i2c_plat_suspend(struct device *dev)
>> +static int dw_i2c_plat_runtime_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;
>>  
>> @@ -383,7 +381,18 @@ static int dw_i2c_plat_suspend(struct device *dev)
>>  	return 0;
>>  }
>>  
>> -static int dw_i2c_plat_resume(struct device *dev)
>> +static int dw_i2c_plat_suspend(struct device *dev)
>> +{
>> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>> +	i_dev->suspended = true;
>> +	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>> +
>> +	return dw_i2c_plat_runtime_suspend(dev);
>> +}
>> +
>> +static int dw_i2c_plat_runtime_resume(struct device *dev)
>>  {
>>  	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>>  
>> @@ -391,7 +400,19 @@ static int dw_i2c_plat_resume(struct device *dev)
>>  		i2c_dw_prepare_clk(i_dev, true);
>>  
>>  	i_dev->init(i_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dw_i2c_plat_resume(struct device *dev)
>> +{
>> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +	dw_i2c_plat_runtime_resume(dev);
>> +
>> +	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  	i_dev->suspended = false;
>> +	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  
>>  	return 0;
>>  }
>> @@ -400,7 +421,7 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>>  	.prepare = dw_i2c_plat_prepare,
>>  	.complete = dw_i2c_plat_complete,
>>  	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
>> -	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
>> +	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, dw_i2c_plat_runtime_resume, NULL)
>>  };
>>  
>>  #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
>> -- 
>> 2.35.1
>>
>