| Message ID | 1432565608-26036-11-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 25, 2015 at 04:53:14PM +0200, Tomeu Vizoso wrote: > When looking up a regulator through its DT node, ensure that the > corresponding device has been registered. I'm sorry but I'm going to need a much better changelog to review this patch. I can't entirely tell what the intended effect is supposed to be, the subject line says we're probing things and the body says we're registering things which aren't the same thing at all. > if (node) { > + of_platform_device_ensure(node); This function doesn't appear in git so I'm guessing that some other patch in the series adds it. See my reply to the first patch... The obvious questions here based on the name are why we're doing something specific to platform devices and why this isn't something we're abstracting in the driver core (or at least generic firmware code) - we're going to have the same thing with ACPI.
On 25 May 2015 at 19:32, Mark Brown <broonie@kernel.org> wrote: > On Mon, May 25, 2015 at 04:53:14PM +0200, Tomeu Vizoso wrote: > >> When looking up a regulator through its DT node, ensure that the >> corresponding device has been registered. > > I'm sorry but I'm going to need a much better changelog to review this > patch. I can't entirely tell what the intended effect is supposed to > be, the subject line says we're probing things and the body says we're > registering things which aren't the same thing at all. Agreed, it would be more appropriate to say that we are registering devices, I will make it clearer in the next revision. It ends up probing the device at that point because we have made sure with an earlier patch that all built-in drivers have been registered already. >> if (node) { >> + of_platform_device_ensure(node); > > This function doesn't appear in git so I'm guessing that some other > patch in the series adds it. See my reply to the first patch... Sorry about that, the series touches so many subsystems that I would have reached the maximum number of recipients on the mailing lists I'm sending it to. I will send it next with you and Krzysztof on CC on the whole series. > The obvious questions here based on the name are why we're doing > something specific to platform devices and why this isn't something > we're abstracting in the driver core (or at least generic firmware code) > - we're going to have the same thing with ACPI. I don't know how useful this is going to be in systems with ACPI. My experience is limited to 32bit ARM, where the kernel has to manage every regulator, clock, gpio, etc so the dependency tree is so big. Is deferred probing a problem with ACPI as well? Thanks, Tomeu
On Tue, May 26, 2015 at 08:17:23AM +0200, Tomeu Vizoso wrote: > On 25 May 2015 at 19:32, Mark Brown <broonie@kernel.org> wrote: > > The obvious questions here based on the name are why we're doing > > something specific to platform devices and why this isn't something > > we're abstracting in the driver core (or at least generic firmware code) > > - we're going to have the same thing with ACPI. > I don't know how useful this is going to be in systems with ACPI. My > experience is limited to 32bit ARM, where the kernel has to manage > every regulator, clock, gpio, etc so the dependency tree is so big. Is > deferred probing a problem with ACPI as well? Yes, x86 based embedded systems use ACPI (and we really ought to be trying to help systems using board files too for that matter).
On 26 May 2015 at 11:36, Mark Brown <broonie@kernel.org> wrote: > On Tue, May 26, 2015 at 08:17:23AM +0200, Tomeu Vizoso wrote: >> On 25 May 2015 at 19:32, Mark Brown <broonie@kernel.org> wrote: > >> > The obvious questions here based on the name are why we're doing >> > something specific to platform devices and why this isn't something >> > we're abstracting in the driver core (or at least generic firmware code) >> > - we're going to have the same thing with ACPI. > >> I don't know how useful this is going to be in systems with ACPI. My >> experience is limited to 32bit ARM, where the kernel has to manage >> every regulator, clock, gpio, etc so the dependency tree is so big. Is >> deferred probing a problem with ACPI as well? > > Yes, x86 based embedded systems use ACPI (and we really ought to be > trying to help systems using board files too for that matter). Yes, I see how registering devices on-demand could be implemented for all those, but what I don't see is how they would benefit from it. I can see an hypothetical maintenance benefit in sharing as much code as possible between these different scenarios, but in this case, because this feature is so closely tied to machine description I think complexity would be actually bigger. The problem I'm trying to address only manifests on systems with dozens of devices that are currently registered in an arbitrary order (as they are in the DT). On machines that have ACPI, most of those devices aren't exposed to the kernel and few or no deferred probes happen (though I have only tested on qemu and Minnowboard MAX, both with no deferred probes). On machines with board files, devices are registered in a predetermined order, presumably without any deferred probes. My understanding is that the problem I'm addressing is specific of machines in which the kernel is in charge of pretty much everything and that the information about what devices are present is given in an arbitrary order. Deferred probe gives us reliably a working system at the end for these machines, but in the meantime some devices will have been deferred several times and when they get to probe the user will have noticed the delay. Regards, Tomeu
On Tue, May 26, 2015 at 05:08:38PM +0200, Tomeu Vizoso wrote: > On 26 May 2015 at 11:36, Mark Brown <broonie@kernel.org> wrote: > > Yes, x86 based embedded systems use ACPI (and we really ought to be > > trying to help systems using board files too for that matter). > Yes, I see how registering devices on-demand could be implemented for > all those, but what I don't see is how they would benefit from it. You'd need to be clearer about what problem you're trying to solve there, which is something you left us guessing at! > I can see an hypothetical maintenance benefit in sharing as much code > as possible between these different scenarios, but in this case, > because this feature is so closely tied to machine description I think > complexity would be actually bigger. We've now got abstractions for common firmware operations (look at the fwnode stuff) and this isn't exactly deep introspection here. If you're trying to solve the probe order problem you can probably get a long way by just doing something that boils down to "try to instantiate everything referenced from this node" which could probably even be kicked from the driver core prior to probe and cover most cases. Or put this into the node lookup interface so we try to instantiate everything we reference. > On machines that have ACPI, most of those devices aren't exposed to > the kernel and few or no deferred probes happen (though I have only > tested on qemu and Minnowboard MAX, both with no deferred probes). On the machines that you happen to have looked at; I would rather expect that x86 based phones will be in a similar situation once they move to ACPI which they should be doing this year if they didn't already, and the embedded systems will doubtless run into this once they have any meaningful hardware on them (the base Minnoboard isn't really interesting here, it's once you build a system on top of that). > On machines with board files, devices are registered in a > predetermined order, presumably without any deferred probes. No, not in the least. Quite aside from anything else as soon as you allow things to be built as modules userspace is free to load things in whatever order amuses it. Think about what's going on here - it's not just registration of devices, it's also about the order in which subsystems and drivers register themselves. > My understanding is that the problem I'm addressing is specific of > machines in which the kernel is in charge of pretty much everything > and that the information about what devices are present is given in an > arbitrary order. I don't think you've fully understood the problem space here.
On 26 May 2015 at 18:54, Mark Brown <broonie@kernel.org> wrote: > On Tue, May 26, 2015 at 05:08:38PM +0200, Tomeu Vizoso wrote: >> On 26 May 2015 at 11:36, Mark Brown <broonie@kernel.org> wrote: > >> > Yes, x86 based embedded systems use ACPI (and we really ought to be >> > trying to help systems using board files too for that matter). > >> Yes, I see how registering devices on-demand could be implemented for >> all those, but what I don't see is how they would benefit from it. > > You'd need to be clearer about what problem you're trying to solve > there, which is something you left us guessing at! > >> I can see an hypothetical maintenance benefit in sharing as much code >> as possible between these different scenarios, but in this case, >> because this feature is so closely tied to machine description I think >> complexity would be actually bigger. > > We've now got abstractions for common firmware operations (look at the > fwnode stuff) and this isn't exactly deep introspection here. > > If you're trying to solve the probe order problem you can probably get a > long way by just doing something that boils down to "try to instantiate > everything referenced from this node" which could probably even be > kicked from the driver core prior to probe and cover most cases. Or put > this into the node lookup interface so we try to instantiate everything > we reference. > >> On machines that have ACPI, most of those devices aren't exposed to >> the kernel and few or no deferred probes happen (though I have only >> tested on qemu and Minnowboard MAX, both with no deferred probes). > > On the machines that you happen to have looked at; I would rather expect > that x86 based phones will be in a similar situation once they move to > ACPI which they should be doing this year if they didn't already, and > the embedded systems will doubtless run into this once they have any > meaningful hardware on them (the base Minnoboard isn't really > interesting here, it's once you build a system on top of that). > >> On machines with board files, devices are registered in a >> predetermined order, presumably without any deferred probes. > > No, not in the least. Quite aside from anything else as soon as you > allow things to be built as modules userspace is free to load things in > whatever order amuses it. Think about what's going on here - it's not > just registration of devices, it's also about the order in which > subsystems and drivers register themselves. > >> My understanding is that the problem I'm addressing is specific of >> machines in which the kernel is in charge of pretty much everything >> and that the information about what devices are present is given in an >> arbitrary order. > > I don't think you've fully understood the problem space here. Fair enough, what's your understanding of it? Thanks, Tomeu
On Tue, May 26, 2015 at 07:53:33PM +0200, Tomeu Vizoso wrote: > On 26 May 2015 at 18:54, Mark Brown <broonie@kernel.org> wrote: > >> My understanding is that the problem I'm addressing is specific of > >> machines in which the kernel is in charge of pretty much everything > >> and that the information about what devices are present is given in an > >> arbitrary order. > > I don't think you've fully understood the problem space here. > > Fair enough, what's your understanding of it? Basically what I said in the e-mail you quoted fully in your reply, especially this section: > > No, not in the least. Quite aside from anything else as soon as you > > allow things to be built as modules userspace is free to load things in > > whatever order amuses it. Think about what's going on here - it's not > > just registration of devices, it's also about the order in which > > subsystems and drivers register themselves. Probe ordering is fairly weakly related to the interface used to register devices, you're going to get dependencies more often the more detail is exposed to the kernel but it's not specific to that. It's definitely not something that we have a solution to for board files, they rely on deferred probing just as much as device tree does.
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 50f4404..3ac44fe 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -26,6 +26,7 @@ #include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/of.h> +#include <linux/of_platform.h> #include <linux/regmap.h> #include <linux/regulator/of_regulator.h> #include <linux/regulator/consumer.h> @@ -1286,6 +1287,7 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev, if (dev && dev->of_node) { node = of_get_regulator(dev, supply); if (node) { + of_platform_device_ensure(node); mutex_lock(®ulator_list_mutex); list_for_each_entry(r, ®ulator_list, list) if (r->dev.parent &&
When looking up a regulator through its DT node, ensure that the corresponding device has been registered. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- drivers/regulator/core.c | 2 ++ 1 file changed, 2 insertions(+)