| Message ID | 1433153905-25204-1-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> > * 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)