| Message ID | 20181001174450.16625-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RFC] i2c: reject transfers when adapter is suspended | expand |
Hi, On 01-10-18 19:44, Wolfram Sang wrote: > When an I2C adapter is suspended, it should reject incoming transfers > from other layers. Some drivers open code this. However, because the > logical I2C adapter device has its own lock for serialization, we can > let the I2C core do that to save the (error prone) boilerplate code. > Make use of a newly added 'is_suspended' flag which gets set if the > suspend_noirq stage of the driver was successful. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > This needs some text. I twisted my brain around all those different PM > requirements for days. So, I might not see the woods for the trees. Anyway: > > This is only tackling that resumed I2C adapters should not accept transfers any > longer. If this happens, it will be rejected with a WARNing. Basically, any > regression is considered a bug. > > This is a bit different to the shutdown/reboot case we were discussing, too. In > that case (with also no irqs), we decided to introduce a new callback > 'master_xfer_irqless' and if it is not present, then we just try the old > callback just to avoid regressions, e.g. I have a board which needs to be reset > using the PMIC watchdog connected via I2C. > > I don't like the different handling of these two cases. Ideally, we should > whitelist such special transfers? The best I could come up with that idea was > to introduce a flag I2C_CLIENT_I_M_SPECIAL for such I2C clients which will then > tag all the I2C transfers with a similar flag, so we know these need special > treatment. I am not really happy with it, though... I've seen various cases where an i2c access to the PMIC is the last thing done at shutdown and those are relying on IRQs working at that time, so yeah some cases will need some special handling. > I didn't test the below patch as is because I couldn't write a test case to > trigger it. I tested it by moving these new PM handlers away from _noirq and > triggered a transfer in a I2C clients' noirq stage. That worked as expected, > the transfer was rejected. > > Which made me wonder if the _noirq stage is really the best hook for this? We > might not catch drivers doing unwanted transfers in their noirq stage. Actually on Intel devices the LPSS I2C controllers are left running until their no_irq suspend method gets run because PCI devices are also left running until their no_irq suspend and they LPSS I2C controllers are needed to access the PMIC which may happen when turning off the PCI devices. 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. > I am also not super happy about using the bus_type PM callbacks here. With > I2C adapters being logical devices only and having no driver attached, I > didn't see another way, though. I'm a bit surprised that you are using the bus_type PM callbacks here, we've been fixing suspend ordering issues by putting RPM consumer device-links to 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. But this child may be suspended earlier then the hardware device and the device-links we add will only influence the ordering wrt the suspend of the actual "hardware" device, the child will stay in the same place in the sorted list and thus may be suspended earlier leading to false-positive -ESHUTDOWN errors. TL;DR: 1) Some controllers stop being ready for transfers in the normal suspend phase, others in the late or noirq phases 2) Putting the suspend/resume callbacks on a child device, rather then the device which has the actual adapter-drivers suspend method may lead to the device being marked as suspended earlier then it actually is, triggering false positives. Together to me this means that we should not play tricks with the bus pm_ops 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. Regards, Hans > > All these issues aside, I hope it is a good starting point for a discussion. > > Looking forward to comments, > > Wolfram > > > drivers/i2c/i2c-core-base.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/i2c.h | 2 ++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 9ee9a15e7134..ebd1ca38f768 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -448,6 +448,42 @@ static void i2c_device_shutdown(struct device *dev) > driver->shutdown(client); > } > > +static int i2c_adap_suspend_noirq(struct device *dev) > +{ > + struct i2c_adapter *adap = i2c_verify_adapter(dev); > + int ret; > + > + if (!adap) > + return pm_generic_suspend_noirq(dev); > + > + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER); > + ret = pm_generic_suspend_noirq(dev); > + if (ret == 0) > + adap->is_suspended = true; > + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER); > + return ret; > +} > + > +static int i2c_adap_resume_noirq(struct device *dev) > +{ > + struct i2c_adapter *adap = i2c_verify_adapter(dev); > + int ret; > + > + if (!adap) > + return pm_generic_resume_noirq(dev); > + > + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER); > + ret = pm_generic_resume_noirq(dev); > + if (ret == 0) > + adap->is_suspended = false; > + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER); > + return ret; > +} > + > +static const struct dev_pm_ops i2c_bus_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(i2c_adap_suspend_noirq, i2c_adap_resume_noirq) > +}; > + > static void i2c_client_dev_release(struct device *dev) > { > kfree(to_i2c_client(dev)); > @@ -493,6 +529,7 @@ struct bus_type i2c_bus_type = { > .probe = i2c_device_probe, > .remove = i2c_device_remove, > .shutdown = i2c_device_shutdown, > + .pm = &i2c_bus_pm_ops, > }; > EXPORT_SYMBOL_GPL(i2c_bus_type); > > @@ -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; > }; > #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev) > >
> 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
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
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 9ee9a15e7134..ebd1ca38f768 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -448,6 +448,42 @@ static void i2c_device_shutdown(struct device *dev) driver->shutdown(client); } +static int i2c_adap_suspend_noirq(struct device *dev) +{ + struct i2c_adapter *adap = i2c_verify_adapter(dev); + int ret; + + if (!adap) + return pm_generic_suspend_noirq(dev); + + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER); + ret = pm_generic_suspend_noirq(dev); + if (ret == 0) + adap->is_suspended = true; + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER); + return ret; +} + +static int i2c_adap_resume_noirq(struct device *dev) +{ + struct i2c_adapter *adap = i2c_verify_adapter(dev); + int ret; + + if (!adap) + return pm_generic_resume_noirq(dev); + + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER); + ret = pm_generic_resume_noirq(dev); + if (ret == 0) + adap->is_suspended = false; + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER); + return ret; +} + +static const struct dev_pm_ops i2c_bus_pm_ops = { + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(i2c_adap_suspend_noirq, i2c_adap_resume_noirq) +}; + static void i2c_client_dev_release(struct device *dev) { kfree(to_i2c_client(dev)); @@ -493,6 +529,7 @@ struct bus_type i2c_bus_type = { .probe = i2c_device_probe, .remove = i2c_device_remove, .shutdown = i2c_device_shutdown, + .pm = &i2c_bus_pm_ops, }; EXPORT_SYMBOL_GPL(i2c_bus_type); @@ -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; }; #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
When an I2C adapter is suspended, it should reject incoming transfers from other layers. Some drivers open code this. However, because the logical I2C adapter device has its own lock for serialization, we can let the I2C core do that to save the (error prone) boilerplate code. Make use of a newly added 'is_suspended' flag which gets set if the suspend_noirq stage of the driver was successful. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- This needs some text. I twisted my brain around all those different PM requirements for days. So, I might not see the woods for the trees. Anyway: This is only tackling that resumed I2C adapters should not accept transfers any longer. If this happens, it will be rejected with a WARNing. Basically, any regression is considered a bug. This is a bit different to the shutdown/reboot case we were discussing, too. In that case (with also no irqs), we decided to introduce a new callback 'master_xfer_irqless' and if it is not present, then we just try the old callback just to avoid regressions, e.g. I have a board which needs to be reset using the PMIC watchdog connected via I2C. I don't like the different handling of these two cases. Ideally, we should whitelist such special transfers? The best I could come up with that idea was to introduce a flag I2C_CLIENT_I_M_SPECIAL for such I2C clients which will then tag all the I2C transfers with a similar flag, so we know these need special treatment. I am not really happy with it, though... I didn't test the below patch as is because I couldn't write a test case to trigger it. I tested it by moving these new PM handlers away from _noirq and triggered a transfer in a I2C clients' noirq stage. That worked as expected, the transfer was rejected. Which made me wonder if the _noirq stage is really the best hook for this? We might not catch drivers doing unwanted transfers in their noirq stage. I am also not super happy about using the bus_type PM callbacks here. With I2C adapters being logical devices only and having no driver attached, I didn't see another way, though. All these issues aside, I hope it is a good starting point for a discussion. Looking forward to comments, Wolfram drivers/i2c/i2c-core-base.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/i2c.h | 2 ++ 2 files changed, 42 insertions(+)