On Thursday, December 11, 2014 04:51:37 PM Ulf Hansson wrote: > On 11 December 2014 at 16:31, Kevin Hilman <khilman at kernel.org> wrote: > > [+ Laurent Pinchart] > > > > Tomasz Figa <tfiga at chromium.org> writes: > > > >> On Thu, Dec 11, 2014 at 8:58 PM, Ulf Hansson <ulf.hansson at linaro.org> wrote: > > > > [...] > > > >>>> @@ -988,11 +1107,28 @@ static int rk_iommu_probe(struct platform_device *pdev) > >>>> return -ENXIO; > >>>> } > >>>> > >>>> + pm_runtime_no_callbacks(dev); > >>>> + pm_runtime_enable(dev); > >>>> + > >>>> + /* Synchronize state of the domain with driver data. */ > >>>> + pm_runtime_get_sync(dev); > >>>> + iommu->is_powered = true; > >>> > >>> Doesn't the runtime PM status reflect the value of "is_powered", thus > >>> why do you need to have a copy of it? Could it perpahps be that you > >>> try to cope with the case when CONFIG_PM is unset? > >>> > >> > >> It's worth noting that this driver fully relies on status of other > >> devices in the power domain the IOMMU is in and does not enforce the > >> status on its own. So in general, as far as my understanding of PM > >> runtime subsystem, the status of the IOMMU device will be always > >> suspended, because nobody will call pm_runtime_get() on it (except the > >> get and put pair in probe). So is_powered is here to track status of > >> the domain, not the device. Feel free to suggest a better way, though. > > > > I still don't like these notifiers. I think they add ways to bypass > > having proper runtime PM implemented for devices/subsystems. > > I do agree, but I haven't found another good solution to the problem. For the record, I'm not liking this mostly because it "fixes" a generic problem in a way that's hidden in the genpd code and very indirect. > > From a high-level, the IOMMU is just another device inside the PM > > domain, so ideally it should be doing it's own _get() and _put() calls > > so the PM domain code would just do the right thing without the need for > > notifiers. > > As I understand it, the IOMMU (or for other similar cases) shouldn't > be doing any get() and put() at all because there are no IO API to > serve request from. > > In principle we could consider these kind devices as "parent" devices > to those other devices that needs them. Then runtime PM core would > take care of things for us, right!? > > Now, I am not so sure using the "parent" approach is actually viable, > since it will likely have other complications, but I haven't > thoroughly thought it though yet. That actually need not be a "parent". What's needed in this case is to do a pm_runtime_get_sync() on a device depended on every time a dependent device is runtime-resumed (and analogously for suspending). The core doesn't have a way to do that, but it looks like we'll need to add it anyway for various reasons (ACPI _DEP is one of them as I mentioned some time ago, but people dismissed it basically as not their problem). -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
