| Message ID | 20181210210310.12677-1-wsa+renesas@sang-engineering.com |
|---|---|
| Headers | show |
| Series | i2c: move handling of suspended adapters to the core | expand |
Hi Wolfram, On 10-12-18 22:02, Wolfram Sang wrote: > Finally, here is the implementation Hans and I agreed on. Plus, all potential > users I could spot already converted. Renesas R-Car driver was added on top. > This series was tested on a Renesas Lager board (R-Car H2). I had to hack some > error cases into the code to verify the workings. I couldn't create an error > case otherwise, this is why further testing with more complex setups is very > welcome. > > For my taste, there is still too much boilerplate code for drivers left. Plus, > it is also too easy to put it too early or too late. But I haven't come up with > a better idea yet. And it is time to get this somehow forward. > > Please comment, review, test... a branch can be found here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers So I've been testing this patch-set on some Intel devices with an i2c-designware controller, combined with a patch to make the suspend/resume methods of the controller call i2c_mark_adapter_suspended() The results were ... interesting: Take 1: 4.20-rc6 + your branch + i2c-designw-patch (+ some unrelated patches which are in my personal tree). *i2c transfers no longer work*, because the controller runtime suspends and the pm_runtime_get_sync to undo this is called from the driver's master_xfer function and the is_suspended check happens before this. Take 2: Kernel from 1 with a patch added (attached) to make the core call pm_runtime_get_sync from __i2c_transfer() if a flag is set + i2c-designware changes to set this flag *i2c transfers still do not work*, because __i2c_transfer is called with the bus-lock held and the pm_runtime_get_sync calls the adapters resume callback which calls i2c_mark_adapter_suspended() which tries to take the bus-lock again -> deadlock Take 3: Kernel from 2 with the i2c-designware suspend/callback patches modified to directly set adapter.is_suspended. To avoid the deadlock. Ok, this works. Note I've not tested reverting one of my recent ACPI suspend ordering patches yet, which should actually trigger the WARN_ON, I've ran out of steam just getting things to work with the patches present. I'm attaching 2 patches as I'm using them in Take 3. Note that the i2c-sprd driver changes in your patchset also get close to triggering this problem, but they dodge the bullet because there are separate system-wide and runtime suspend handlers and only the system-wide handlers call the new i2c_mark_adapter_suspended() function. As I explain in the commit message of the first attached patch simply always using split handlers is not really an option because of adapter drivers using DPM_FLAG_SMART_PREPARE or DPM_FLAG_SMART_SUSPEND. So I'm afraid that this is way messier then I would like it to be, we could go with my flag solution combined with not protecting the is_suspended flag under the bus lock. That would make the WARN_ON slightly racy, so we might fail to trigger the warning under some rare circumstances, reverting to the old behavior of continuing with the transfer on a suspended adapter, but I don't think that is all that bad. Regards, Hans > Wolfram Sang (10): > i2c: add 'is_suspended' flag for i2c adapters > i2c: reject new transfers when adapters are suspended > i2c: synquacer: remove unused is_suspended flag > i2c: brcmstb: use core helper to mark adapter suspended > i2c: zx2967: use core helper to mark adapter suspended > i2c: sprd: don't use pdev as variable name for struct device * > i2c: sprd: use core helper to mark adapter suspended > i2c: exynos5: use core helper to mark adapter suspended > i2c: s3c2410: use core helper to mark adapter suspended > i2c: rcar: add suspend/resume support > > drivers/i2c/busses/i2c-brcmstb.c | 13 ++----------- > drivers/i2c/busses/i2c-exynos5.c | 11 ++--------- > drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++ > drivers/i2c/busses/i2c-s3c2410.c | 8 ++------ > drivers/i2c/busses/i2c-sprd.c | 34 ++++++++++++---------------------- > drivers/i2c/busses/i2c-synquacer.c | 5 ----- > drivers/i2c/busses/i2c-zx2967.c | 8 ++------ > drivers/i2c/i2c-core-base.c | 3 +++ > include/linux/i2c.h | 9 +++++++++ > 9 files changed, 57 insertions(+), 59 deletions(-) > From 7781ff0b5033436c1d2740570364b1739017e03d Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Tue, 11 Dec 2018 19:19:17 +0100 Subject: [PATCH 1/2] i2c: add I2C_AQ_RUNTIME_PM adapter quirk flag Add a new I2C_AQ_RUNTIME_PM adapter quirk flag, when this flag is set then the i2c-core will all pm_runtime_get_sync() before calling an adapter's master_xfer function and call pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend() after the master_xfer function. Moving these runtime pm calls into the core for adapters which use them is necessary for the WARN_ON(adap->is_suspended) to not trigger when runtime-suspend is used. Another approach would be to only call i2c_mark_adapter_suspended() from the adapter's regular suspend/resume callbacks and not from the runtime-resume ones, but that would circumvent the check also on system suspend when using DPM_FLAG_SMART_PREPARE or DPM_FLAG_SMART_SUSPEND. Note that of the 20 adapter drivers which call pm_runtime_get_sync() from there master_xfer function, 16 call pm_runtime_put_autosuspend() when done. i2c-nomadik.c and i2c-sh_mobile.c use pm_runtime_put_sync() and i2c-tegra.c and i2c-rcar.c use pm_runtime_put(), so these will need to be modified to use these new flag, or they will need another flag for their special case. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/i2c-core-base.c | 16 ++++++++++++++-- include/linux/i2c.h | 2 ++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 5b2078a902f8..acff1e4c09d9 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1866,12 +1866,18 @@ 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; + if (adap->quirks && (adap->quirks->flags & I2C_AQ_RUNTIME_PM)) + pm_runtime_get_sync(adap->dev.parent); + + if (WARN_ON(adap->is_suspended)) { + ret = -ESHUTDOWN; + goto out; + } + /* * i2c_trace_msg_key gets enabled when tracepoint i2c_transfer gets * enabled. This is an efficient way of keeping the for-loop from @@ -1904,6 +1910,12 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) trace_i2c_result(adap, num, ret); } +out: + if (adap->quirks && (adap->quirks->flags & I2C_AQ_RUNTIME_PM)) { + pm_runtime_mark_last_busy(adap->dev.parent); + pm_runtime_put_autosuspend(adap->dev.parent); + } + return ret; } EXPORT_SYMBOL(__i2c_transfer); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 9852038ee3a7..96b9cac6c01e 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -661,6 +661,8 @@ struct i2c_adapter_quirks { #define I2C_AQ_NO_ZERO_LEN_READ BIT(5) #define I2C_AQ_NO_ZERO_LEN_WRITE BIT(6) #define I2C_AQ_NO_ZERO_LEN (I2C_AQ_NO_ZERO_LEN_READ | I2C_AQ_NO_ZERO_LEN_WRITE) +/* core must call pm_runtime_get_sync / put_autosuspend around master_xfer */ +#define I2C_AQ_RUNTIME_PM BIT(7) /* * i2c_adapter is the structure used to identify a physical i2c bus along
Hi Hans, thanks for testing this series! > Note that the i2c-sprd driver changes in your patchset also get close > to triggering this problem, but they dodge the bullet because there > are separate system-wide and runtime suspend handlers and only the > system-wide handlers call the new i2c_mark_adapter_suspended() function. Yes, this what I assumed to be all around - seperate handlers for RPM and PM. And I also assumed that they could be seperated if they aren't already. Until I read... > As I explain in the commit message of the first attached patch > simply always using split handlers is not really an option because > of adapter drivers using DPM_FLAG_SMART_PREPARE or > DPM_FLAG_SMART_SUSPEND. ... this. I don't know what these flags do (and reading SMART in there gives me more a 'uh-oh' feeling) but if the handlers can't be split, the issues you were seeing are a consequence, yes. For me, this sadly spoils the whole concept. The patches you add make things even more complicated. Not happy about that. Looking at the open coded version you did for the designware driver, I wonder now if it is better to just leave it at driver level? Need to sleep over it, though. Thanks again, Wolfram
Hi, On 12-12-18 00:41, Wolfram Sang wrote: > Hi Hans, > > thanks for testing this series! > >> Note that the i2c-sprd driver changes in your patchset also get close >> to triggering this problem, but they dodge the bullet because there >> are separate system-wide and runtime suspend handlers and only the >> system-wide handlers call the new i2c_mark_adapter_suspended() function. > > Yes, this what I assumed to be all around - seperate handlers for RPM > and PM. And I also assumed that they could be seperated if they aren't > already. Until I read... > >> As I explain in the commit message of the first attached patch >> simply always using split handlers is not really an option because >> of adapter drivers using DPM_FLAG_SMART_PREPARE or >> DPM_FLAG_SMART_SUSPEND. > > ... this. I don't know what these flags do (and reading SMART in there > gives me more a 'uh-oh' feeling) On x86 the lines between runtime suspend and system-suspend are blurring with technologies like "connected standby" and in general devices moving to suspend2idle as system-suspend state, I guess this also applies to modern smartphone platforms but I'm not following those closely. What this means on x86 is that the firmware is not doing any suspend handling anymore and the OS / device-drivers get to do all the suspend. For many devices this means that if they are runtime-suspended, and there is no difference between the runtime and system suspend handling at the driver level, they can be left as is, so during a system suspend they are not touched at all (and left runtime suspended during system resume). The "SMART" bit is really not all that smart, SMART_PREPARE means that the drivers pm prepare callback will return positive non 0 (e.g. 1) to indicate that the device may not be kept in its runtime suspended state when transitioning to system-suspend, otoh when the prepare callback returns 0 and the SMART_PREPARE flag is set then *all* suspend/ resume handling can be skipped during a system suspend. SMART_SUSPEND means that if the device is runtime-suspended it may be left as such, this only gets checked if the driver either does not have the SMART_PREPARE flag, or the prepare callback indicated that skipping the entire suspend/resume handling is not ok. So this is not that scary, but it does require driver authors to know what they are doing... > but if the handlers can't be split, the > issues you were seeing are a consequence, yes. > > For me, this sadly spoils the whole concept. The patches you add make > things even more complicated. Not happy about that. Agreed, they don't fill me with happiness either. > Looking at the open coded version you did for the designware driver, I > wonder now if it is better to just leave it at driver level? Need to > sleep over it, though. I myself was thinking in the same direction (leave the entire suspended check at the driver level). Regards, Hans
Hans, all, > > ... this. I don't know what these flags do (and reading SMART in there > > gives me more a 'uh-oh' feeling) > > On x86 the lines between runtime suspend and system-suspend are blurring > with technologies like "connected standby" and in general devices moving > to suspend2idle as system-suspend state, I guess this also applies to > modern smartphone platforms but I'm not following those closely. I'd guess so, too. I am not aware of any existing mechanism for that at the moment, though. If somebody does, please enlighten us. > The "SMART" bit is really not all that smart, SMART_PREPARE means that > the drivers pm prepare callback will return positive non 0 (e.g. 1) to > indicate that the device may not be kept in its runtime suspended > state when transitioning to system-suspend, otoh when the prepare > callback returns 0 and the SMART_PREPARE flag is set then *all* suspend/ > resume handling can be skipped during a system suspend. Thanks for the detailed explanation! Much appreciated. > > Looking at the open coded version you did for the designware driver, I > > wonder now if it is better to just leave it at driver level? Need to > > sleep over it, though. > > I myself was thinking in the same direction (leave the entire suspended > check at the driver level). So, I was giving it some more thoughts, and my feeling is to still apply this series with the review comments addressed. Plus, clearly mark the new 'is_suspended' flag and the helper function as *optional* to allow for driver specific solutions as well. The then-to-be-added documentation would state that it is mostly useful for seperated system suspend and runtime suspend. For more complex situations, custom solutions are accepted, too. Which means your patch for designware should be added to the series. My take is there are enough drivers out there already which can benefit from this new helper. If it turns out to be useless somewhen in the future, we can still remove it. What do you (and all others, of course) think? Thanks, Wolfram
Hi, On 18-12-18 21:17, Wolfram Sang wrote: > Hans, all, > >>> ... this. I don't know what these flags do (and reading SMART in there >>> gives me more a 'uh-oh' feeling) >> >> On x86 the lines between runtime suspend and system-suspend are blurring >> with technologies like "connected standby" and in general devices moving >> to suspend2idle as system-suspend state, I guess this also applies to >> modern smartphone platforms but I'm not following those closely. > > I'd guess so, too. I am not aware of any existing mechanism for that at > the moment, though. If somebody does, please enlighten us. > >> The "SMART" bit is really not all that smart, SMART_PREPARE means that >> the drivers pm prepare callback will return positive non 0 (e.g. 1) to >> indicate that the device may not be kept in its runtime suspended >> state when transitioning to system-suspend, otoh when the prepare >> callback returns 0 and the SMART_PREPARE flag is set then *all* suspend/ >> resume handling can be skipped during a system suspend. > > Thanks for the detailed explanation! Much appreciated. > >>> Looking at the open coded version you did for the designware driver, I >>> wonder now if it is better to just leave it at driver level? Need to >>> sleep over it, though. >> >> I myself was thinking in the same direction (leave the entire suspended >> check at the driver level). > > So, I was giving it some more thoughts, and my feeling is to still apply > this series with the review comments addressed. Plus, clearly mark the > new 'is_suspended' flag and the helper function as *optional* to allow > for driver specific solutions as well. The then-to-be-added > documentation would state that it is mostly useful for seperated system > suspend and runtime suspend. For more complex situations, custom > solutions are accepted, too. Which means your patch for designware > should be added to the series. > > My take is there are enough drivers out there already which can benefit > from this new helper. If it turns out to be useless somewhen in the > future, we can still remove it. > > What do you (and all others, of course) think? I've been thinking along the same lines: your series for the drivers with separate runtime and system suspend handlers, "custom" driver specific code for troublesome cases like i2c-designware. Do you want me to send out a new version of my patch for the i2c-designware code? Regards, Hans
> I've been thinking along the same lines: your series for the drivers > with separate runtime and system suspend handlers, "custom" driver > specific code for troublesome cases like i2c-designware. Cool, thanks! > Do you want me to send out a new version of my patch for the i2c-designware > code? Yes, please do.