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

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]


> 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);

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;
> > 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?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]