On Mon, May 25, 2015 at 04:53:05PM +0200, Tomeu Vizoso wrote:
> ...by moving the locking to regulator_dev_lookup.

I appear to have only patches 1 and 10 of a 21 patch series. It is
difficult to tell what the series might be about or how any of the
patches in it might be related to each other, it doesn't look like the
sort of obviously repetitive series which is easily understandable.
Please don't do things like this, if you're sending a series people need
to be able to tell what it is.

What's going on here?

Attachments:

(No filename) (516.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-25 17:33:14

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 10/21] regulator: core: Probe regulators on demand

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.

Attachments:

(No filename) (888.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-25 17:46:05

On Tue, May 26, 2015 at 08:17:23AM +0200, Tomeu Vizoso wrote:
> On 25 May 2015 at 19:32, Mark Brown <[email protected]> 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).

Attachments:

(No filename) (793.00 B)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-26 12:26:27

by Tomeu Vizoso

[permalink] [raw]

Subject: Re: [PATCH 15/21] backlight: Probe backlight devices on demand

On 26 May 2015 at 10:39, Lee Jones <[email protected]> wrote:
> On Tue, 26 May 2015, Sascha Hauer wrote:
>> On Tue, May 26, 2015 at 08:18:50AM +0100, Lee Jones wrote:
>> > On Mon, 25 May 2015, Tomeu Vizoso wrote:
>> >
>> > > When looking up a backlight device through its DT node, ensure that the
>> > > corresponding device has been registered.
>> > >
>> > > Signed-off-by: Tomeu Vizoso <[email protected]>
>> > > ---
>> > > drivers/video/backlight/backlight.c | 3 +++
>> > > 1 file changed, 3 insertions(+)
>> >
>> > Looks reasonable.
>> >
>> > Until anyone screams at me, applied thanks.
>>
>> The compiler will scream at you when it realizes that
>> of_platform_device_ensure() doesn't exist in your kernel...
>
> Yup, indeed it did.
>
> I assumed this was *only* enabling subsystems and that the
> framework/API was already accepted.
>
> So the advice I'd give to Tomeu when sending full enablement
> patch-sets i.e. ones which provide the framework/API *and* enable
> subsystems in the same set, is to send the entire set to everyone, so
> we can see what the aim of the set is and how to deal with it.

Yeah, but get_maintainer.pl outputs 33 maintainer addresses, plus 1
reviewer plus 14 mailing lists, so to avoid rejects because of too
many recipients I went with the advice in [0] and sent each patch to
their maintainers and list(s) and the cover letter to all lists. Also
sent the whole series to lakml to make sure that it's at least indexed
there.

Any advice on what to do with series that span so many subsystems?

Thanks,

Tomeu

[0] http://lwn.net/Articles/585782/

> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2015-05-26 13:34:34

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 15/21] backlight: Probe backlight devices on demand

On Tue, 26 May 2015, Tomeu Vizoso wrote:

> On 26 May 2015 at 10:39, Lee Jones <[email protected]> wrote:
> > On Tue, 26 May 2015, Sascha Hauer wrote:
> >> On Tue, May 26, 2015 at 08:18:50AM +0100, Lee Jones wrote:
> >> > On Mon, 25 May 2015, Tomeu Vizoso wrote:
> >> >
> >> > > When looking up a backlight device through its DT node, ensure that the
> >> > > corresponding device has been registered.
> >> > >
> >> > > Signed-off-by: Tomeu Vizoso <[email protected]>
> >> > > ---
> >> > > drivers/video/backlight/backlight.c | 3 +++
> >> > > 1 file changed, 3 insertions(+)
> >> >
> >> > Looks reasonable.
> >> >
> >> > Until anyone screams at me, applied thanks.
> >>
> >> The compiler will scream at you when it realizes that
> >> of_platform_device_ensure() doesn't exist in your kernel...
> >
> > Yup, indeed it did.
> >
> > I assumed this was *only* enabling subsystems and that the
> > framework/API was already accepted.
> >
> > So the advice I'd give to Tomeu when sending full enablement
> > patch-sets i.e. ones which provide the framework/API *and* enable
> > subsystems in the same set, is to send the entire set to everyone, so
> > we can see what the aim of the set is and how to deal with it.
>
> Yeah, but get_maintainer.pl outputs 33 maintainer addresses, plus 1
> reviewer plus 14 mailing lists, so to avoid rejects because of too
> many recipients I went with the advice in [0] and sent each patch to
> their maintainers and list(s) and the cover letter to all lists. Also
> sent the whole series to lakml to make sure that it's at least indexed
> there.

Mails aren't usually rejected because they have too many recipients,
rather they require approval. This isn't a blocker, especially for
patch-sets like this.

> Any advice on what to do with series that span so many subsystems?

I would either ensure everyone is informed, split into two choices;
either CC everyone on every patch, or at least everyone on the
cover-letter and the core API changes. The other method is to have
the API changes accepted first, then once accepted send out the
subsystem changes [FWIW: this is the method I (wrongly) assumed you
used].

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2015-05-26 14:47:31

On Tue, May 26, 2015 at 05:08:38PM +0200, Tomeu Vizoso wrote:
> On 26 May 2015 at 11:36, Mark Brown <[email protected]> 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.

Attachments:

(No filename) (2.59 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-05-26 17:53:58

by Tomeu Vizoso

[permalink] [raw]

Subject: Re: [PATCH 10/21] regulator: core: Probe regulators on demand

On 26 May 2015 at 18:54, Mark Brown <[email protected]> wrote:
> On Tue, May 26, 2015 at 05:08:38PM +0200, Tomeu Vizoso wrote:
>> On 26 May 2015 at 11:36, Mark Brown <[email protected]> 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

2015-05-26 18:57:02

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH 06/21] of/platform: Add of_platform_device_ensure()

Hi Tomeu,

On Mon, May 25, 2015 at 04:53:10PM +0200, Tomeu Vizoso wrote:
> This function ensures that the device that encloses the passed device
> node is registered, and thus probed if the corresponding driver has been
> registered already.

Even if the driver has already been registered this code will not
guarantee that the device has been probed if driver enabled asynchronous
probing (async probing should appear in 4.2 and the end goal is to have
async probing enabled by default for most drivers).

Also, why are we concentrating on platform drivers only? What about
other devices, for example a gpio expander behind i2c bus?

I am also concerned about adding OF-specific hooks into every subsystem
out there. Can we make process of instantiating OF devices iterative
instead and add them only if their parent has been already probed and
also devices corresponding to all phandles they reference have also been
probed? We could repeat scanning of the device tree every time we bind
a driver and bulk-add leftovers at late_initcall time. This would
mean that we could keep all logic in OF code (and maybe ACPI will add
their own implementation) and keep other subsystems unaware.

Thanks.

--
Dmitry

2015-05-26 19:42:06

by Stephen Warren

[permalink] [raw]

Subject: Re: [PATCH 02/21] ARM: tegra: Add gpio-ranges property

On 05/25/2015 08:53 AM, Tomeu Vizoso wrote:
> Specify how the GPIOs map to the pins in T124, so the dependency is
> explicit.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> ---
> arch/arm/boot/dts/tegra124.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
> index 13cc7ca..5d1d35f 100644
> --- a/arch/arm/boot/dts/tegra124.dtsi
> +++ b/arch/arm/boot/dts/tegra124.dtsi
> @@ -254,6 +254,7 @@
> gpio-controller;
> #interrupt-cells = <2>;
> interrupt-controller;
> + gpio-ranges = <&pinmux 0 0 250>;

We should be consistent between SoCs. Why not make the same change for
all Tegra SoCs?

I think this change will cause the GPIO subsystem to call into the
pinctrl subsystem and create/add/register a new GPIO<->pinctrl range
structure. The pinctrl driver already does this, so I think we'll end up
with two duplicate entries in the pinctrl device's gpio_ranges list.
This probably won't cause a problem, but I wanted to make sure you'd
thought about it to make sure.

Right now, I think we get lucky and pinctrl ends up probing first (or at
least very early) anyway. Somewhat related to this series, I wonder if
we shouldn't add pinctrl client properties to every node in the Tegra DT
that describes a controller that makes use of external pins that are
affected by the pinmux. Such a change would guarantee this desired
probing order. In order to preserve the "program the entire pinmux at
once" semantics, these new pinctrl client properties would all need to
reference empty states, yet would still need to exist to represent the
dependency.

2015-05-26 19:55:51

On Mon, Jun 22, 2015 at 10:23 AM, Tomeu Vizoso
<[email protected]> wrote:
> On 28 May 2015 at 06:33, Rob Herring <[email protected]> wrote:
>> On Mon, May 25, 2015 at 9:53 AM, Tomeu Vizoso
>> <[email protected]> wrote:
>>> Hello,
>>>
>>> I have a problem with the panel on my Tegra Chromebook taking longer than
>>> expected to be ready during boot (Stéphane Marchesin reported what is
>>> basically the same issue in [0]), and have looked into ordered probing as a
>>> better way of solving this than moving nodes around in the DT or playing with
>>> initcall levels.
>>>
>>> While reading the thread [1] that Alexander Holler started with his series to
>>> make probing order deterministic, it occurred to me that it should be possible
>>> to achieve the same by registering devices as they are referenced by other
>>> devices.
>>
>> I like the concept and novel approach.
>>
>>> This basically reuses the information that is already implicit in the probe()
>>> implementations, saving us from refactoring existing drivers or adding
>>> information to DTBs.
>>>
>>> Something I'm not completely happy with is that I have had to move the call to
>>> of_platform_populate after all platform drivers have been registered.
>>> Otherwise I don't see how I could register drivers on demand as we don't have
>>> yet each driver's compatible strings.
>>
>> Yeah, this is the opposite of what we'd really like.
>
> Can you elaborate on the reasons why we would like to have devices
> registered before built-in drivers finish registering, even if we
> don't probe them yet?

My main thought was for modules we will almost always have devices
appearing first. More generally, we can have devices and drivers
coming or going at any point in time and should not put restrictions
on ordering.

Also, I think all the probe ordering and dependency tracking should be
done within the driver core (i.e. dependencies are a list of struct
devices). At some level it has to become firmware specific, but we
want to minimize that part. I could be convinced otherwise and you
have put more thought into this problem than I have.

>> Ideally, we would
>> have a solution that works for modules too. However, we're no worse
>> off. We pretty much build-in dependencies to avoid module ordering
>> problems.
>
> Nod, I haven't looked yet at requesting modules on-demand, but I guess
> it should be doable. Modules that have dependencies described in the
> firmware should get them probed automatically already though.
>
>> Perhaps we need to make the probing on-demand rather than simply on
>> device<->driver match occurring.
>
> I'm afraid that too much old code depends on that. For example, Rafael
> pointed out to the PNP subsystem, which registers a driver that will
> probe devices with the EISA ID PNP0c02 to reserve memory regions for
> devices that will be probed later.
>
> http://lxr.free-electrons.com/source/drivers/pnp/system.c
>
> My understanding is that probing of PNP0c02 devices must happen before
> the actual devices that depend on those regions are probed, so if we
> decoupled the probing from the driver/device registration, we would be
> breaking that assumption.

That shouldn't matter as PNP matching is PNP specific. We already have
different ways of matching with device/driver name or of_match_table
for example. Changing how and when OF matching occurs would not affect
PNP matching. We do matching on device and driver add currently. For
the "when" part, we would need to add what I'll call async matching or
deferred matching which in addition to matching on the of_match_table
also matches on the dependency list having probed. Your last series
essentially does this, but the difference is yours is not OF specific
and I think it needs to be. I mean it is OF specific only in the
aspect that matching already is. From a driver and subsystem
standpoint, it should not be OF specific much like deferred probe is
not OF specific, but in reality only occurs (currently) on OF probed
drivers.

>>> For machs that don't move of_platform_populate() to a later point, these
>>> patches shouldn't cause any problems but it's not guaranteed that we'll avoid
>>> all the deferred probes as some drivers may not be registered yet.
>>
>> Ideally, of_platform_populate is not explicitly called by each
>> platform. So I think we need to make this work for the default case.
>
> The problem is that some platforms will need fixing because some
> initcalls assume that some devices will have been registered already,
> or even probed. I think removing those assumptions shouldn't be
> problematic because I haven't had much trouble with this on the four
> platforms I have tested with, but I cannot test every board that is
> supported upstream.
>
> I can ask though the KernelCI folks to boot my branch in all their
> boards and make sure that those work when of_platform_populate is
> called in late_initcall.

I'd imagine Kevin would be happy to. That is still a subset of h/w, so
we'd need a way to disable any solution.

Rob