On 4 November 2014 14:51, Rafael J. Wysocki [off-list ref] wrote:

On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:

quoted

[...]

quoted

Generally, there are two or even three levels of runtime PM handling,
driver, (possibly) bus type and (possibly) PM domain (and multiple levels
of these are possible in principle).  All of them have to be initialized
at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should
be initialized even before registerind the device or during device
registration.  Doing that later may be too late.  When the device has been
registered, runtime PM should work to an extent allowing the driver to access
the device and configure it further after calling pm_runtime_resume().

Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
it must take the fact that the driver's own ->runtime_resume() may be called
as a result of this into account.  That's why I'm asking whether or not the
core should call pm_runtime_resume() before calling really_probe() in a
followup branch of this thread.

I am reading the other thread, let's see.

quoted

The driver's own runtime PM handling must be initialized in the driver and
the only place suitable for that is ->probe().  However, it needs to be done
*before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
is executed.  If that is done properly, it should be possible to cover
both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in
->runtime_resume() (when it is called for the first time), as that would be
error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

quoted

quoted

The AMBA bus and some of its drivers a good example of how this has
been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly
power up a PM domain, is not suitable as the _common_ solution to
solve the race condition. It certainly may work for some scenarios,
but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync()
from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they
are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using
pm_runtime_get_sync(), until the device has been attached to its PM
domain. Right?

Yes, but my point was that those bus types might need to be changed.

We can't make everyone happy at the same time if their ideas about what to do
are different.

Urgh. I fail to understand this comment.

Why do we prefer the pm_runtime_get_sync() solution in favour of this
pathset's approach?

What are the benefit do we get with pm_runtime_get_sync()?

quoted

[...]

quoted

quoted

For PM domains that are initialized in powered off state, we can't
rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
power on these PM domains. We need a different mechanism, which is
suggested in this v3 patchset.

That is quite simple to address, though.  You can register a bus type
notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
(where the target device belongs to the domain), and do that only for
CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

I guess we could use notifiers, but I am not sure I see any benefit.
The code will be more complex and we need error handling as well.

Like "oh, I can't power up this thing, so I should fail ->probe()"?

Then your driver would need to depend on the specific knowledge about the
given PM domain, I'm afraid.

If you want error handling like that, it needs to be handled by the core,
so as to avoid calling the bus type's ->probe() as well in that case.

Yes, I want this error handling - but I fail to understand why the bus
can't handle the errors.

If works perfectly in this patchset's approach.

So to summarize:

- Devices need to be added to power domains before really_probe() is called
  for them.  Otherwise we'll have ordering problems all over.

What ordering problems?

- Runtime PM (if compiled in) needs to be enabled for all devices in power
  domains by default.  Otherwise devices may lose power as a result for
  power management of the other devices in the same domain.

- The core should try to power up domains before calling really_probe() both
  for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
  "device is accessible" assumption.

And how exactly will you then power up the PM domain when
CONFIG_PM_RUNTIME is unset?

- Bus types may need to do more on top of that in their ->probe(), so the
  driver's ->probe() can make that assumption too in all cases.

Does that make sense to you?

I working on fully understanding your idea. :-)

Kind regards
Uffe