[PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Ulf Hansson]

Other subsystem buses attach PM domains during probe, but prior calling
the driver's ->probe() method. During the removal phase, detaching the PM
domain will be done after invoking the driver's ->remove() callback.

Convert the SDIO bus to follow this behavior and add error handling.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

A similar patch as $subject patch has been posted and discussed earlier.

According to Aaron Lu, who added the initial support for the ACPI PM domain to
the SDIO bus, this change is safe.
http://www.spinics.net/lists/linux-mmc/msg28946.html

---
 drivers/mmc/core/sdio_bus.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index bee02e6..7e327a6 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -137,6 +137,10 @@ static int sdio_bus_probe(struct device *dev)
 	if (!id)
 		return -ENODEV;
 
+	ret = dev_pm_domain_attach(dev, false);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
 	/* Unbound SDIO functions are always suspended.
 	 * During probe, the function is set active and the usage count
 	 * is incremented.  If the driver supports runtime PM,
@@ -166,6 +170,7 @@ static int sdio_bus_probe(struct device *dev)
 disable_runtimepm:
 	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
 		pm_runtime_put_noidle(dev);
+	dev_pm_domain_detach(dev, false);
 	return ret;
 }
 
@@ -197,6 +202,8 @@ static int sdio_bus_remove(struct device *dev)
 	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
 		pm_runtime_put_sync(dev);
 
+	dev_pm_domain_detach(dev, false);
+
 	return ret;
 }
 
@@ -316,10 +323,8 @@ int sdio_add_func(struct sdio_func *func)
 	sdio_set_of_node(func);
 	sdio_acpi_set_handle(func);
 	ret = device_add(&func->dev);
-	if (ret == 0) {
+	if (ret == 0)
 		sdio_func_set_present(func);
-		dev_pm_domain_attach(&func->dev, false);
-	}
 
 	return ret;
 }
@@ -335,7 +340,6 @@ void sdio_remove_func(struct sdio_func *func)
 	if (!sdio_func_present(func))
 		return;
 
-	dev_pm_domain_detach(&func->dev, false);
 	device_del(&func->dev);
 	of_node_put(func->dev.of_node);
 	put_device(&func->dev);
-- 
1.9.1

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Dmitry Torokhov]

Hi Ulf,

On Mon, Jun 01, 2015 at 12:18:25PM +0200, Ulf Hansson wrote:
> Other subsystem buses attach PM domains during probe, but prior calling
> the driver's ->probe() method. During the removal phase, detaching the PM
> domain will be done after invoking the driver's ->remove() callback.
> 
> Convert the SDIO bus to follow this behavior and add error handling.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> A similar patch as $subject patch has been posted and discussed earlier.
> 
> According to Aaron Lu, who added the initial support for the ACPI PM domain to
> the SDIO bus, this change is safe.
> http://www.spinics.net/lists/linux-mmc/msg28946.html
> 
> ---
>  drivers/mmc/core/sdio_bus.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
> index bee02e6..7e327a6 100644
> --- a/drivers/mmc/core/sdio_bus.c
> +++ b/drivers/mmc/core/sdio_bus.c
> @@ -137,6 +137,10 @@ static int sdio_bus_probe(struct device *dev)
>  	if (!id)
>  		return -ENODEV;
>  
> +	ret = dev_pm_domain_attach(dev, false);
> +	if (ret == -EPROBE_DEFER)
> +		return ret;

Why do we handle only -EPROBE_DEFER? What about other errors? Maybe
dev_pm_domain_attach() is too chatty?

> +
>  	/* Unbound SDIO functions are always suspended.
>  	 * During probe, the function is set active and the usage count
>  	 * is incremented.  If the driver supports runtime PM,
> @@ -166,6 +170,7 @@ static int sdio_bus_probe(struct device *dev)
>  disable_runtimepm:
>  	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
>  		pm_runtime_put_noidle(dev);
> +	dev_pm_domain_detach(dev, false);

If dev_pm_domain_attach() returned -EEXIST (which means that someone
else attached the device to the domain) should we be detaching it?

I realize that the other buses do the same thing, so these questions are
general.

Thanks.

-- 
Dmitry

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Ulf Hansson]

On 1 June 2015 at 21:42, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Ulf,
>
> On Mon, Jun 01, 2015 at 12:18:25PM +0200, Ulf Hansson wrote:
>> Other subsystem buses attach PM domains during probe, but prior calling
>> the driver's ->probe() method. During the removal phase, detaching the PM
>> domain will be done after invoking the driver's ->remove() callback.
>>
>> Convert the SDIO bus to follow this behavior and add error handling.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> A similar patch as $subject patch has been posted and discussed earlier.
>>
>> According to Aaron Lu, who added the initial support for the ACPI PM domain to
>> the SDIO bus, this change is safe.
>> http://www.spinics.net/lists/linux-mmc/msg28946.html
>>
>> ---
>>  drivers/mmc/core/sdio_bus.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
>> index bee02e6..7e327a6 100644
>> --- a/drivers/mmc/core/sdio_bus.c
>> +++ b/drivers/mmc/core/sdio_bus.c
>> @@ -137,6 +137,10 @@ static int sdio_bus_probe(struct device *dev)
>>       if (!id)
>>               return -ENODEV;
>>
>> +     ret = dev_pm_domain_attach(dev, false);
>> +     if (ret == -EPROBE_DEFER)
>> +             return ret;
>
> Why do we handle only -EPROBE_DEFER? What about other errors? Maybe
> dev_pm_domain_attach() is too chatty?

Agree.

>
>> +
>>       /* Unbound SDIO functions are always suspended.
>>        * During probe, the function is set active and the usage count
>>        * is incremented.  If the driver supports runtime PM,
>> @@ -166,6 +170,7 @@ static int sdio_bus_probe(struct device *dev)
>>  disable_runtimepm:
>>       if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
>>               pm_runtime_put_noidle(dev);
>> +     dev_pm_domain_detach(dev, false);
>
> If dev_pm_domain_attach() returned -EEXIST (which means that someone
> else attached the device to the domain) should we be detaching it?
>
> I realize that the other buses do the same thing, so these questions are
> general.

dev_pm_domain_attach() could take a closer look at what errors it gets
from acpi_dev_pm_attach() and genpd_dev_pm_attach(). In principle we
want the caller of dev_pm_domain_attach() to be able to consider all
returned error codes as errors, instead of just -EPROBE_DEFER.

On the other hand, the "long term plan" is to have PM domain pointers
assigned at device registration point instead of at probe. Since
driver core then invokes the ->activate|sync|dismiss() callbacks, it
will make the dev_pm_domain_attach|detach() APIs redundant.

So, I wonder if it's worth to update the buses/APIs in an intermediate
step (even if it would be an improvement), instead of working on
adopting the "long term plan". What do you think?

Kind regards
Uffe

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 09:54:13AM +0200, Ulf Hansson wrote:
> On the other hand, the "long term plan" is to have PM domain pointers
> assigned at device registration point instead of at probe. Since
> driver core then invokes the ->activate|sync|dismiss() callbacks, it
> will make the dev_pm_domain_attach|detach() APIs redundant.

How does that work if the PM domain doesn't exist yet, because the driver
supplying it hasn't had its own device probed ?

You can't return -EPROBE_DEFER to anything that isn't being called via a
driver's probe function.  So, when the OF code is scanning the device
tree, creating the various struct device's, returning -EPROBE_DEFER will
cause it to fail to register that struct device and there is no mechanism
to retry.

Also, your idea above goes against the grain that other resources are
heading - that is, towards probe time resolution of resources.  IRQ
resources have the same problem, and this has caused real problems on
platforms to have the IRQ resources resolved to a number at registration
time.

To me, your idea sounds like the wrong approach and unworkable.

What would resolve that would be to have the device registration succeed,
but also to scan all devices in the system when a PM domain is attached
and attach the PM domain to matching devices.  The problem you then have
is the same race between attaching the PM domain to all devices in the
domain, and devices being probed.  Remember, this may not be happening at
boot time, but during module load, which on some systems is multithreaded.
So, the problem of not having all devices in the PM domain hasn't changed
and nothing is solved by this approach - the only thing is the code has
become more complex.

I don't think that's an improvement.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 09:51:01AM +0100, Russell King - ARM Linux wrote:
> What would resolve that would be to have the device registration succeed,
> but also to scan all devices in the system when a PM domain is attached
> and attach the PM domain to matching devices.  The problem you then have
> is the same race between attaching the PM domain to all devices in the
> domain, and devices being probed.  Remember, this may not be happening at
> boot time, but during module load, which on some systems is multithreaded.
> So, the problem of not having all devices in the PM domain hasn't changed
> and nothing is solved by this approach - the only thing is the code has
> become more complex.
> 
> I don't think that's an improvement.

Another idea, which I came up with while replying to Ivan on a different
but related topic is:

* When we create a device which we know has a PM domain which should be
  attached, try to look up that PM domain.
* If we find the PM domain, attach the domain, otherwise create the
  domain but mark it "incomplete".
* When the device is attempted to be probed, force the probe to defer if
  the PM domain is inccomplete.
* When a PM domain is initialised, search for any incomplete PM domain,
  and if found, connect with the domain and arrange to handle it.

The problem you still run into is whether all devices have been registered
into the domain, but I really think that expecting all devices to be
registered is in itself a problem.

Consider the case where a PM domain spreads between a parent device (eg,
a bridge) and a set of child devices.  We may need to probe the bridge
in order to discover and register the child devices.  If we're expecting
the PM domain to be fully populated with all devices before that happens,
we're in a catch-22 situation: we can't register the child devices until
the bridge has been probed, and we can't probe the bridge because the
PM domain is incomplete.

I don't think there's a good answer to that one, not without going back
to today's model, where a PM domain is able to be brought up and down
without all of its associated devices attached.  That, in itself, should
not be a problem since drivers should _not_ assume that the device has
any context preserved from the last time it saw it (eg, consider the
random state of a device when a crashed kernel kexecs into a new kernel.)
Doing anything else is just fragile.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Ulf Hansson]

On 2 June 2015 at 13:07, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 02, 2015 at 09:51:01AM +0100, Russell King - ARM Linux wrote:
>> What would resolve that would be to have the device registration succeed,
>> but also to scan all devices in the system when a PM domain is attached
>> and attach the PM domain to matching devices.  The problem you then have
>> is the same race between attaching the PM domain to all devices in the
>> domain, and devices being probed.  Remember, this may not be happening at
>> boot time, but during module load, which on some systems is multithreaded.
>> So, the problem of not having all devices in the PM domain hasn't changed
>> and nothing is solved by this approach - the only thing is the code has
>> become more complex.
>>
>> I don't think that's an improvement.
>
> Another idea, which I came up with while replying to Ivan on a different
> but related topic is:
>
> * When we create a device which we know has a PM domain which should be
>   attached, try to look up that PM domain.
> * If we find the PM domain, attach the domain, otherwise create the
>   domain but mark it "incomplete".
> * When the device is attempted to be probed, force the probe to defer if
>   the PM domain is inccomplete.
> * When a PM domain is initialised, search for any incomplete PM domain,
>   and if found, connect with the domain and arrange to handle it.

This is a good description from what I had in mind and for what I have
started working on. :-)

>
> The problem you still run into is whether all devices have been registered
> into the domain, but I really think that expecting all devices to be
> registered is in itself a problem.
>
> Consider the case where a PM domain spreads between a parent device (eg,
> a bridge) and a set of child devices.  We may need to probe the bridge
> in order to discover and register the child devices.  If we're expecting
> the PM domain to be fully populated with all devices before that happens,
> we're in a catch-22 situation: we can't register the child devices until
> the bridge has been probed, and we can't probe the bridge because the
> PM domain is incomplete.

I think I understand what you have in mind and it's an important point!

Although, I have some ideas for how we could allow the PM domain to be
initialized independently of whether all devices has been attached or
not. That would address your concerns, right!?

>
> I don't think there's a good answer to that one, not without going back
> to today's model, where a PM domain is able to be brought up and down
> without all of its associated devices attached.  That, in itself, should
> not be a problem since drivers should _not_ assume that the device has
> any context preserved from the last time it saw it (eg, consider the
> random state of a device when a crashed kernel kexecs into a new kernel.)
> Doing anything else is just fragile.

I agree that today's model has some positives, but there are also
drawbacks. See below.

1. In the case where the PM domain driver hasn't yet been probed and
thus not called pm_genpd_init() + of_genpd_add_provider_*(), we can't
support probe deferral of a device which needs its PM domain powered.
Simply because dev_pm_domain_attach() has no knowledge about the (not
yet registered) PM domain.

2. We can't attach a devices to its PM domain via DT, unless it has a
matching bus/driver which can be probed.

3. Dealing with device enumeration from HW is fragile, since there are
no way to keep the PM domain powered during that sequence. That's
because the PM domain hasn't yet been attached. For example
amba_device_add() has this limitation.

Rafael might want to add some additional reasons...


Finally, regarding $subject patch, I intend to queue it for my next
branch, unless someone has objections.

Kind regards
Uffe

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 03:18:07PM +0200, Ulf Hansson wrote:
> On 2 June 2015 at 13:07, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Jun 02, 2015 at 09:51:01AM +0100, Russell King - ARM Linux wrote:
> >> What would resolve that would be to have the device registration succeed,
> >> but also to scan all devices in the system when a PM domain is attached
> >> and attach the PM domain to matching devices.  The problem you then have
> >> is the same race between attaching the PM domain to all devices in the
> >> domain, and devices being probed.  Remember, this may not be happening at
> >> boot time, but during module load, which on some systems is multithreaded.
> >> So, the problem of not having all devices in the PM domain hasn't changed
> >> and nothing is solved by this approach - the only thing is the code has
> >> become more complex.
> >>
> >> I don't think that's an improvement.
> >
> > Another idea, which I came up with while replying to Ivan on a different
> > but related topic is:
> >
> > * When we create a device which we know has a PM domain which should be
> >   attached, try to look up that PM domain.
> > * If we find the PM domain, attach the domain, otherwise create the
> >   domain but mark it "incomplete".
> > * When the device is attempted to be probed, force the probe to defer if
> >   the PM domain is inccomplete.
> > * When a PM domain is initialised, search for any incomplete PM domain,
> >   and if found, connect with the domain and arrange to handle it.
> 
> This is a good description from what I had in mind and for what I have
> started working on. :-)

It would help if you explained things better, since your original
comment made it sound like you were intending to _only_ attach PM
domains at device probe time.

> > The problem you still run into is whether all devices have been registered
> > into the domain, but I really think that expecting all devices to be
> > registered is in itself a problem.
> >
> > Consider the case where a PM domain spreads between a parent device (eg,
> > a bridge) and a set of child devices.  We may need to probe the bridge
> > in order to discover and register the child devices.  If we're expecting
> > the PM domain to be fully populated with all devices before that happens,
> > we're in a catch-22 situation: we can't register the child devices until
> > the bridge has been probed, and we can't probe the bridge because the
> > PM domain is incomplete.
> 
> I think I understand what you have in mind and it's an important point!
> 
> Although, I have some ideas for how we could allow the PM domain to be
> initialized independently of whether all devices has been attached or
> not. That would address your concerns, right!?

Yes, but then I end up wondering what the point of changing this stuff
is.  It sounds like it's change for the sake of change rather than for
any proper tangible benefit.

> > I don't think there's a good answer to that one, not without going back
> > to today's model, where a PM domain is able to be brought up and down
> > without all of its associated devices attached.  That, in itself, should
> > not be a problem since drivers should _not_ assume that the device has
> > any context preserved from the last time it saw it (eg, consider the
> > random state of a device when a crashed kernel kexecs into a new kernel.)
> > Doing anything else is just fragile.
> 
> I agree that today's model has some positives, but there are also
> drawbacks. See below.
> 
> 1. In the case where the PM domain driver hasn't yet been probed and
> thus not called pm_genpd_init() + of_genpd_add_provider_*(), we can't
> support probe deferral of a device which needs its PM domain powered.
> Simply because dev_pm_domain_attach() has no knowledge about the (not
> yet registered) PM domain.

Why can't we do something like the clk API, regulator API, or of the
many other resource providing subsystems, and detect when there's an
firmware node saying that a resource should be provided to a device,
but the resource is not yet available?

For example, if you call clk_get() for a device and clock consumer name
and firmware says that the clock should exist, but it can't be found,
clk_get() returns an EPROBE_DEFER error-pointer.  Same for the regulator
subsystem.  IRQ domains behave very similarly, but earlier on before the
device driver's probe function is called.

Why can't PM domains also follow this model?

> 2. We can't attach a devices to its PM domain via DT, unless it has a
> matching bus/driver which can be probed.
> 
> 3. Dealing with device enumeration from HW is fragile, since there are
> no way to keep the PM domain powered during that sequence. That's
> because the PM domain hasn't yet been attached. For example
> amba_device_add() has this limitation.

Stop right there.  Device enumeration from hardware is _not_ fragile,
it's what is used on virtually all PCI-using platforms out there.  If
you invent something that takes the kernel away from being able to do
hardware enumeration, you will be breaking lots of stuff outside the
ARM architecture and you will _not_ be popular.

You're working on a core subsystem, and you _have_ to realise that the
world is not made up of DT - you have to cater for other probing styles
besides DT, which includes hardware based enumeration.   You can't say
"we're not going to support that anymore" - that's not your decision to
make.

Yes, amba_device_add() has the limitation that the ID must be readable
when the device is declared, and we can't do much about that because
the ID is exported to userspace - hence it's part of the kernel's
userspace API.  Changing it is going to risk breaking userspace, so
we need to be careful about what we do there - but that's my problem,
not yours.  I know Linaro likes to take chunks of the kernel away from
me without talking to me first, but I do wish they wouldn't.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] mmc: core: Attach PM domain prior probing of SDIO func driver

[Ulf Hansson]

[...]

>> > * When we create a device which we know has a PM domain which should be
>> >   attached, try to look up that PM domain.
>> > * If we find the PM domain, attach the domain, otherwise create the
>> >   domain but mark it "incomplete".
>> > * When the device is attempted to be probed, force the probe to defer if
>> >   the PM domain is inccomplete.
>> > * When a PM domain is initialised, search for any incomplete PM domain,
>> >   and if found, connect with the domain and arrange to handle it.
>>
>> This is a good description from what I had in mind and for what I have
>> started working on. :-)
>
> It would help if you explained things better, since your original
> comment made it sound like you were intending to _only_ attach PM
> domains at device probe time.

I was expecting people to recall from earlier discussions on linux-pm,
sorry. Of course it's impossible to keep up with all things that goes
on.

Anyway, commit e90d5532773e2bcccc538dd346b9fc3482cd700c ("driver core
/ PM: Add PM domain callbacks for device setup/cleanup"), indicates
the direction of change for how we wants to deal with assigning PM
domain pointers.

[...]

>>
>> 1. In the case where the PM domain driver hasn't yet been probed and
>> thus not called pm_genpd_init() + of_genpd_add_provider_*(), we can't
>> support probe deferral of a device which needs its PM domain powered.
>> Simply because dev_pm_domain_attach() has no knowledge about the (not
>> yet registered) PM domain.
>
> Why can't we do something like the clk API, regulator API, or of the
> many other resource providing subsystems, and detect when there's an
> firmware node saying that a resource should be provided to a device,
> but the resource is not yet available?
>
> For example, if you call clk_get() for a device and clock consumer name
> and firmware says that the clock should exist, but it can't be found,
> clk_get() returns an EPROBE_DEFER error-pointer.  Same for the regulator
> subsystem.  IRQ domains behave very similarly, but earlier on before the
> device driver's probe function is called.
>
> Why can't PM domains also follow this model?

That seems like separate and much wider discussion.

I haven't suggested to re-write the entire PM domain/genpd, just the
way how PM domain pointers gets assigned. I don't think it will be
that of a big deal, but let's see.

>
>> 2. We can't attach a devices to its PM domain via DT, unless it has a
>> matching bus/driver which can be probed.
>>
>> 3. Dealing with device enumeration from HW is fragile, since there are
>> no way to keep the PM domain powered during that sequence. That's
>> because the PM domain hasn't yet been attached. For example
>> amba_device_add() has this limitation.
>
> Stop right there.  Device enumeration from hardware is _not_ fragile,
> it's what is used on virtually all PCI-using platforms out there.  If
> you invent something that takes the kernel away from being able to do
> hardware enumeration, you will be breaking lots of stuff outside the
> ARM architecture and you will _not_ be popular.
>
> You're working on a core subsystem, and you _have_ to realise that the
> world is not made up of DT - you have to cater for other probing styles
> besides DT, which includes hardware based enumeration.   You can't say
> "we're not going to support that anymore" - that's not your decision to
> make.

I was too vague in 3). Obviously enumeration from HW isn't fragile in
most cases.

>
> Yes, amba_device_add() has the limitation that the ID must be readable
> when the device is declared, and we can't do much about that because
> the ID is exported to userspace - hence it's part of the kernel's
> userspace API.  Changing it is going to risk breaking userspace, so
> we need to be careful about what we do there - but that's my problem,
> not yours.  I know Linaro likes to take chunks of the kernel away from
> me without talking to me first, but I do wish they wouldn't.
>

Let's discuss the amba_device_add() case in more detail.

When the amba device resides in a PM domain, it's likely (I know for
sure that's case for ux500) that the PM domain must be powered while
amba_device_add() tries to read the ID from the HW.

In the current solution the PM domain pointer is assigned at
->probe(), which makes it impossible for amba_device_add() to control
the power to the PM domain.

If we instead had that PM domain pointer assigned during
amba_device_add(), we can use runtime PM to control the power to the
PM domain. That's the simple idea, I will try to make it work in
practice.

Anyway, I don't have any patches available for posting/sharing yet. I
will keep you on cc while posting them.

Kind regards
Uffe