Re: [RFC] i2c: reject transfers when adapter is suspended

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

Hi,

On 01-10-18 23:59, Wolfram Sang wrote:
> 
>> IMHO the is_suspended flag should be set by the adapter driver from its
>> own suspend callback (which may be a normal, late or noirq suspend callback),
>> as this is the point past which the bus should no longer be used because
>> e.g. the suspend callbacks has disabled clocks and/or put the controller
>> in reset, etc.
> 
> That's what I meant with "not seeing the woods for the trees". Thanks
> for clearing my view!
> 
>> the i2c-controller device where necessary. But you're hooking into the
>> child-device of the actual "hardware" device which represents the i2c_adapter
>> bits.
> 
> That was intentional because this is the device the I2C core has access
> to. Because the callbacks deal only with 'is_suspended' and not with HW,
> I thought the logic device could be suitable. I didn't think of
> additional relations only affecting its parent.
> 
>> on the adapter-device. I believe that instead we should add
>> i2c_adapter_mark_suspended and i2c_adapter_mark_resumed helpers and let
>> the adapter-driver suspend/resume callbacks call these. This will ensure
>> that they get called at the right phase and at the exact moment that
>> the adapter is actually suspending.
> 
> Agreed. And these helpers are basically this:
> 
>>> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>> +	adap->is_suspended = true;
>>> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> 
> and this:
> 
>>> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>> +	adap->is_suspended = false;
>>> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);

Yes that is correct.

> together with this still in the I2C core:
> 
>>> @@ -1867,6 +1904,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>>>    	if (WARN_ON(!msgs || num < 1))
>>>    		return -EINVAL;
>>> +	if (WARN_ON(adap->is_suspended))
>>> +		return -ESHUTDOWN;
>>> +
>>>    	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
>>>    		return -EOPNOTSUPP;

Ack.

>>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>>> index 65b4eaed1d96..ee46295a67d4 100644
>>> --- a/include/linux/i2c.h
>>> +++ b/include/linux/i2c.h
>>> @@ -692,6 +692,8 @@ struct i2c_adapter {
>>>    	const struct i2c_adapter_quirks *quirks;
>>>    	struct irq_domain *host_notify_domain;
>>> +
>>> +	int is_suspended:1;
> 
> Are we aligned?

I believe we are.

Regards,

Hans