[RESEND,v1,1/5] of/platform: Speed up of_find_device_by_node()
diff mbox series

Message ID 20190604003218.241354-2-saravanak@google.com
State Superseded
Headers show
Series
  • Solve postboot supplier cleanup and optimize probe ordering
Related show

Commit Message

Saravana Kannan June 4, 2019, 12:32 a.m. UTC
Add a pointer from device tree node to the device created from it.
This allows us to find the device corresponding to a device tree node
without having to loop through all the platform devices.

However, fallback to looping through the platform devices to handle
any devices that might set their own of_node.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 20 +++++++++++++++++++-
 include/linux/of.h    |  3 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

Comments

Rob Herring June 10, 2019, 5:36 p.m. UTC | #1
Why are you resending this rather than replying to Frank's last
comments on the original?

On Mon, Jun 3, 2019 at 6:32 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Add a pointer from device tree node to the device created from it.
> This allows us to find the device corresponding to a device tree node
> without having to loop through all the platform devices.
>
> However, fallback to looping through the platform devices to handle
> any devices that might set their own of_node.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/platform.c | 20 +++++++++++++++++++-
>  include/linux/of.h    |  3 +++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 04ad312fd85b..1115a8d80a33 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -42,6 +42,8 @@ static int of_dev_node_match(struct device *dev, void *data)
>         return dev->of_node == data;
>  }
>
> +static DEFINE_SPINLOCK(of_dev_lock);
> +
>  /**
>   * of_find_device_by_node - Find the platform_device associated with a node
>   * @np: Pointer to device tree node
> @@ -55,7 +57,18 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
>  {
>         struct device *dev;
>
> -       dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
> +       /*
> +        * Spinlock needed to make sure np->dev doesn't get freed between NULL
> +        * check inside and kref count increment inside get_device(). This is
> +        * achieved by grabbing the spinlock before setting np->dev = NULL in
> +        * of_platform_device_destroy().
> +        */
> +       spin_lock(&of_dev_lock);
> +       dev = get_device(np->dev);
> +       spin_unlock(&of_dev_lock);
> +       if (!dev)
> +               dev = bus_find_device(&platform_bus_type, NULL, np,
> +                                     of_dev_node_match);
>         return dev ? to_platform_device(dev) : NULL;
>  }
>  EXPORT_SYMBOL(of_find_device_by_node);
> @@ -196,6 +209,7 @@ static struct platform_device *of_platform_device_create_pdata(
>                 platform_device_put(dev);
>                 goto err_clear_flag;
>         }
> +       np->dev = &dev->dev;
>
>         return dev;
>
> @@ -556,6 +570,10 @@ int of_platform_device_destroy(struct device *dev, void *data)
>         if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
>                 device_for_each_child(dev, NULL, of_platform_device_destroy);
>
> +       /* Spinlock is needed for of_find_device_by_node() to work */
> +       spin_lock(&of_dev_lock);
> +       dev->of_node->dev = NULL;
> +       spin_unlock(&of_dev_lock);
>         of_node_clear_flag(dev->of_node, OF_POPULATED);
>         of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
>
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 0cf857012f11..f2b4912cbca1 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -48,6 +48,8 @@ struct property {
>  struct of_irq_controller;
>  #endif
>
> +struct device;
> +
>  struct device_node {
>         const char *name;
>         phandle phandle;
> @@ -68,6 +70,7 @@ struct device_node {
>         unsigned int unique_id;
>         struct of_irq_controller *irq_trans;
>  #endif
> +       struct device *dev;             /* Device created from this node */
>  };
>
>  #define MAX_PHANDLE_ARGS 16
> --
> 2.22.0.rc1.257.g3120a18244-goog
>
Rob Herring June 10, 2019, 9:07 p.m. UTC | #2
On Mon, Jun 10, 2019 at 1:15 PM Saravana Kannan <saravanak@google.com> wrote:
>
> I did. And I didn't get a response. Folks suggested this might be one of the preferred ways instead of sending "bump" emails.

It's summer time and people go on vacation.

Rob
Frank Rowand June 11, 2019, 3:18 p.m. UTC | #3
Hi Saravana,

On 6/10/19 10:36 AM, Rob Herring wrote:
> Why are you resending this rather than replying to Frank's last
> comments on the original?

Adding on a different aspect...  The independent replies from three different
maintainers (Rob, Mark, myself) pointed out architectural issues with the
patch series.  There were also some implementation issues brought out.
(Although I refrained from bringing up most of my implementation issues
as they are not relevant until architecture issues are resolved.)

When three maintainers say the architecture has issues, you should step
back and think hard.  (Not to say maintainers are always correct...)

My suggestion at this point is that you need to go back to the drawing board
and re-think how to address the use case.

-Frank

> 
> On Mon, Jun 3, 2019 at 6:32 PM Saravana Kannan <saravanak@google.com> wrote:
>>
>> Add a pointer from device tree node to the device created from it.
>> This allows us to find the device corresponding to a device tree node
>> without having to loop through all the platform devices.
>>
>> However, fallback to looping through the platform devices to handle
>> any devices that might set their own of_node.
>>
>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> ---
>>  drivers/of/platform.c | 20 +++++++++++++++++++-
>>  include/linux/of.h    |  3 +++
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 04ad312fd85b..1115a8d80a33 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -42,6 +42,8 @@ static int of_dev_node_match(struct device *dev, void *data)
>>         return dev->of_node == data;
>>  }
>>
>> +static DEFINE_SPINLOCK(of_dev_lock);
>> +
>>  /**
>>   * of_find_device_by_node - Find the platform_device associated with a node
>>   * @np: Pointer to device tree node
>> @@ -55,7 +57,18 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
>>  {
>>         struct device *dev;
>>
>> -       dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
>> +       /*
>> +        * Spinlock needed to make sure np->dev doesn't get freed between NULL
>> +        * check inside and kref count increment inside get_device(). This is
>> +        * achieved by grabbing the spinlock before setting np->dev = NULL in
>> +        * of_platform_device_destroy().
>> +        */
>> +       spin_lock(&of_dev_lock);
>> +       dev = get_device(np->dev);
>> +       spin_unlock(&of_dev_lock);
>> +       if (!dev)
>> +               dev = bus_find_device(&platform_bus_type, NULL, np,
>> +                                     of_dev_node_match);
>>         return dev ? to_platform_device(dev) : NULL;
>>  }
>>  EXPORT_SYMBOL(of_find_device_by_node);
>> @@ -196,6 +209,7 @@ static struct platform_device *of_platform_device_create_pdata(
>>                 platform_device_put(dev);
>>                 goto err_clear_flag;
>>         }
>> +       np->dev = &dev->dev;
>>
>>         return dev;
>>
>> @@ -556,6 +570,10 @@ int of_platform_device_destroy(struct device *dev, void *data)
>>         if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
>>                 device_for_each_child(dev, NULL, of_platform_device_destroy);
>>
>> +       /* Spinlock is needed for of_find_device_by_node() to work */
>> +       spin_lock(&of_dev_lock);
>> +       dev->of_node->dev = NULL;
>> +       spin_unlock(&of_dev_lock);
>>         of_node_clear_flag(dev->of_node, OF_POPULATED);
>>         of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
>>
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index 0cf857012f11..f2b4912cbca1 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -48,6 +48,8 @@ struct property {
>>  struct of_irq_controller;
>>  #endif
>>
>> +struct device;
>> +
>>  struct device_node {
>>         const char *name;
>>         phandle phandle;
>> @@ -68,6 +70,7 @@ struct device_node {
>>         unsigned int unique_id;
>>         struct of_irq_controller *irq_trans;
>>  #endif
>> +       struct device *dev;             /* Device created from this node */
>>  };
>>
>>  #define MAX_PHANDLE_ARGS 16
>> --
>> 2.22.0.rc1.257.g3120a18244-goog
>>
> .
>
Saravana Kannan June 11, 2019, 8:56 p.m. UTC | #4
On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> Hi Saravana,
>
> On 6/10/19 10:36 AM, Rob Herring wrote:
> > Why are you resending this rather than replying to Frank's last
> > comments on the original?
>
> Adding on a different aspect...  The independent replies from three different
> maintainers (Rob, Mark, myself) pointed out architectural issues with the
> patch series.  There were also some implementation issues brought out.
> (Although I refrained from bringing up most of my implementation issues
> as they are not relevant until architecture issues are resolved.)

Right, I'm not too worried about the implementation issues before we
settle on the architectural issues. Those are easy to fix.

Honestly, the main points that the maintainers raised are:
1) This is a configuration property and not describing the device.
Just use the implicit dependencies coming from existing bindings.

I gave a bunch of reasons for why I think it isn't an OS configuration
property. But even if that's not something the maintainers can agree
to, I gave a concrete example (cyclic dependencies between clock
provider hardware) where the implicit dependencies would prevent one
of the devices from probing till the end of time. So even if the
maintainers don't agree we should always look at "depends-on" to
decide the dependencies, we still need some means to override the
implicit dependencies where they don't match the real dependency. Can
we use depends-on as an override when the implicit dependencies aren't
correct?

2) This doesn't need to be solved because this is just optimizing
probing or saving power ("we should get rid of this auto disabling"):

I explained why this patch series is not just about optimizing probe
ordering or saving power. And why we can't ignore auto disabling
(because it's more than just auto disabling). The kernel is currently
broken when trying to use modules in ARM SoCs (probably in other
systems/archs too, but I can't speak for those).

3) Concerns about backwards compatibility

I pointed out why the current scheme (depends-on being the only source
of dependency) doesn't break compatibility. And if we go with
"depends-on" as an override what we could do to keep backwards
compatibility. Happy to hear more thoughts or discuss options.

4) How the "sync_state" would work for a device that supplies multiple
functionalities but a limited driver.

I explained how it would work.

> When three maintainers say the architecture has issues, you should step
> back and think hard.  (Not to say maintainers are always correct...)

Yes, the 3 maintainers brought up their concerns and I replied to
explain my viewpoint on their concerns. So I'm waiting for a response
from them before I proceed further. Even if the response is going to
be "we still don't agree with this architecture", I'd prefer at least
some direction on what the maintainers think is the right
architecture. There are a million different ways to solve this. It's
not reasonable to expect people submitting patches to shoot in the
dark and hope it hits what the maintainers have in their mind as
"acceptable solutions". I even proposed some alternatives. So, it'd be
nice to hear the maintainers' thoughts on my responses, alternative
designs or some alternate proposals.

There's a real problem that needs to be solved. Just saying "I don't
like this solution" without suggesting alternatives isn't a
constructive way to improve the kernel.

> My suggestion at this point is that you need to go back to the drawing board
> and re-think how to address the use case.

I'd be happy to and I've been thinking about other ways, but I'd like
some direction from the maintainers before writing a lot of code
that's just going to be completely rejected.

To summarize the options:
1) Use depends-on as the only source of dependency (this patch series)
- Easy to keep backward compatible
- Handles all cases
- Arguable that this is OS configuration

2) Use existing bindings for dependencies, but use "depends-on" for
overriding cases that are incorrect.
- New kernel not backward compatible with old DT. So devices will stop
booting or won't have full functionality.
- Needs some scheme to disabling all dependency tracking to be
backward compatible.
  a) Can have the dependency linking code under CONFIG_XXXXX
  b) Can have a kernel commandline
  c) NEW alternative to CONFIG_XXXX or commandline. Can have an
additional "implicit-dependencies-are-explicit-too" DT property that
can be added to the "choosen" DT node to tell the OS that the implicit
dependencies are valid real dependencies too. Then if they need
overrides, depends-on can be used. So old DT blobs would effectively
have this feature disabled.
- Handles all cases
- Only (c) above could be argued as OS configuration. But I'd argue
that it's clarifying the DT dependencies to be more explicit.

Thanks,
Saravana

> -Frank
>
> >
> > On Mon, Jun 3, 2019 at 6:32 PM Saravana Kannan <saravanak@google.com> wrote:
> >>
> >> Add a pointer from device tree node to the device created from it.
> >> This allows us to find the device corresponding to a device tree node
> >> without having to loop through all the platform devices.
> >>
> >> However, fallback to looping through the platform devices to handle
> >> any devices that might set their own of_node.
> >>
> >> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> ---
> >>  drivers/of/platform.c | 20 +++++++++++++++++++-
> >>  include/linux/of.h    |  3 +++
> >>  2 files changed, 22 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index 04ad312fd85b..1115a8d80a33 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -42,6 +42,8 @@ static int of_dev_node_match(struct device *dev, void *data)
> >>         return dev->of_node == data;
> >>  }
> >>
> >> +static DEFINE_SPINLOCK(of_dev_lock);
> >> +
> >>  /**
> >>   * of_find_device_by_node - Find the platform_device associated with a node
> >>   * @np: Pointer to device tree node
> >> @@ -55,7 +57,18 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
> >>  {
> >>         struct device *dev;
> >>
> >> -       dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
> >> +       /*
> >> +        * Spinlock needed to make sure np->dev doesn't get freed between NULL
> >> +        * check inside and kref count increment inside get_device(). This is
> >> +        * achieved by grabbing the spinlock before setting np->dev = NULL in
> >> +        * of_platform_device_destroy().
> >> +        */
> >> +       spin_lock(&of_dev_lock);
> >> +       dev = get_device(np->dev);
> >> +       spin_unlock(&of_dev_lock);
> >> +       if (!dev)
> >> +               dev = bus_find_device(&platform_bus_type, NULL, np,
> >> +                                     of_dev_node_match);
> >>         return dev ? to_platform_device(dev) : NULL;
> >>  }
> >>  EXPORT_SYMBOL(of_find_device_by_node);
> >> @@ -196,6 +209,7 @@ static struct platform_device *of_platform_device_create_pdata(
> >>                 platform_device_put(dev);
> >>                 goto err_clear_flag;
> >>         }
> >> +       np->dev = &dev->dev;
> >>
> >>         return dev;
> >>
> >> @@ -556,6 +570,10 @@ int of_platform_device_destroy(struct device *dev, void *data)
> >>         if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
> >>                 device_for_each_child(dev, NULL, of_platform_device_destroy);
> >>
> >> +       /* Spinlock is needed for of_find_device_by_node() to work */
> >> +       spin_lock(&of_dev_lock);
> >> +       dev->of_node->dev = NULL;
> >> +       spin_unlock(&of_dev_lock);
> >>         of_node_clear_flag(dev->of_node, OF_POPULATED);
> >>         of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
> >>
> >> diff --git a/include/linux/of.h b/include/linux/of.h
> >> index 0cf857012f11..f2b4912cbca1 100644
> >> --- a/include/linux/of.h
> >> +++ b/include/linux/of.h
> >> @@ -48,6 +48,8 @@ struct property {
> >>  struct of_irq_controller;
> >>  #endif
> >>
> >> +struct device;
> >> +
> >>  struct device_node {
> >>         const char *name;
> >>         phandle phandle;
> >> @@ -68,6 +70,7 @@ struct device_node {
> >>         unsigned int unique_id;
> >>         struct of_irq_controller *irq_trans;
> >>  #endif
> >> +       struct device *dev;             /* Device created from this node */
> >>  };
> >>
> >>  #define MAX_PHANDLE_ARGS 16
> >> --
> >> 2.22.0.rc1.257.g3120a18244-goog
> >>
> > .
> >
>
Sandeep Patil June 11, 2019, 9:52 p.m. UTC | #5
On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> >
> > Hi Saravana,
> >
> > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > Why are you resending this rather than replying to Frank's last
> > > comments on the original?
> >
> > Adding on a different aspect...  The independent replies from three different
> > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > patch series.  There were also some implementation issues brought out.
> > (Although I refrained from bringing up most of my implementation issues
> > as they are not relevant until architecture issues are resolved.)
> 
> Right, I'm not too worried about the implementation issues before we
> settle on the architectural issues. Those are easy to fix.
> 
> Honestly, the main points that the maintainers raised are:
> 1) This is a configuration property and not describing the device.
> Just use the implicit dependencies coming from existing bindings.
> 
> I gave a bunch of reasons for why I think it isn't an OS configuration
> property. But even if that's not something the maintainers can agree
> to, I gave a concrete example (cyclic dependencies between clock
> provider hardware) where the implicit dependencies would prevent one
> of the devices from probing till the end of time. So even if the
> maintainers don't agree we should always look at "depends-on" to
> decide the dependencies, we still need some means to override the
> implicit dependencies where they don't match the real dependency. Can
> we use depends-on as an override when the implicit dependencies aren't
> correct?
> 
> 2) This doesn't need to be solved because this is just optimizing
> probing or saving power ("we should get rid of this auto disabling"):
> 
> I explained why this patch series is not just about optimizing probe
> ordering or saving power. And why we can't ignore auto disabling
> (because it's more than just auto disabling). The kernel is currently
> broken when trying to use modules in ARM SoCs (probably in other
> systems/archs too, but I can't speak for those).
> 
> 3) Concerns about backwards compatibility
> 
> I pointed out why the current scheme (depends-on being the only source
> of dependency) doesn't break compatibility. And if we go with
> "depends-on" as an override what we could do to keep backwards
> compatibility. Happy to hear more thoughts or discuss options.
> 
> 4) How the "sync_state" would work for a device that supplies multiple
> functionalities but a limited driver.

<snip>
To be clear, all of above are _real_ problems that stops us from efficiently
load device drivers as modules for Android.

So, if 'depends-on' doesn't seem like the right approach and "going back to
the drawing board" is the ask, could you please point us in the right
direction?

- ssp
Rob Herring June 12, 2019, 1:53 p.m. UTC | #6
On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
>
> On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > >
> > > Hi Saravana,
> > >
> > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > Why are you resending this rather than replying to Frank's last
> > > > comments on the original?
> > >
> > > Adding on a different aspect...  The independent replies from three different
> > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > patch series.  There were also some implementation issues brought out.
> > > (Although I refrained from bringing up most of my implementation issues
> > > as they are not relevant until architecture issues are resolved.)
> >
> > Right, I'm not too worried about the implementation issues before we
> > settle on the architectural issues. Those are easy to fix.
> >
> > Honestly, the main points that the maintainers raised are:
> > 1) This is a configuration property and not describing the device.
> > Just use the implicit dependencies coming from existing bindings.
> >
> > I gave a bunch of reasons for why I think it isn't an OS configuration
> > property. But even if that's not something the maintainers can agree
> > to, I gave a concrete example (cyclic dependencies between clock
> > provider hardware) where the implicit dependencies would prevent one
> > of the devices from probing till the end of time. So even if the
> > maintainers don't agree we should always look at "depends-on" to
> > decide the dependencies, we still need some means to override the
> > implicit dependencies where they don't match the real dependency. Can
> > we use depends-on as an override when the implicit dependencies aren't
> > correct?
> >
> > 2) This doesn't need to be solved because this is just optimizing
> > probing or saving power ("we should get rid of this auto disabling"):
> >
> > I explained why this patch series is not just about optimizing probe
> > ordering or saving power. And why we can't ignore auto disabling
> > (because it's more than just auto disabling). The kernel is currently
> > broken when trying to use modules in ARM SoCs (probably in other
> > systems/archs too, but I can't speak for those).
> >
> > 3) Concerns about backwards compatibility
> >
> > I pointed out why the current scheme (depends-on being the only source
> > of dependency) doesn't break compatibility. And if we go with
> > "depends-on" as an override what we could do to keep backwards
> > compatibility. Happy to hear more thoughts or discuss options.
> >
> > 4) How the "sync_state" would work for a device that supplies multiple
> > functionalities but a limited driver.
>
> <snip>
> To be clear, all of above are _real_ problems that stops us from efficiently
> load device drivers as modules for Android.
>
> So, if 'depends-on' doesn't seem like the right approach and "going back to
> the drawing board" is the ask, could you please point us in the right
> direction?

Use the dependencies which are already there in DT. That's clocks,
pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
going to accept duplicating all those dependencies in DT. The downside
for the kernel is you have to address these one by one and can't have
a generic property the driver core code can parse. After that's in
place, then maybe we can consider handling any additional dependencies
not already captured in DT. Once all that is in place, we can probably
sort device and/or driver lists to optimize the probe order (maybe the
driver core already does that now?).

Get rid of the auto disabling of clocks and regulators in
late_initcall. It's simply not a valid marker that boot is done when
modules are involved. We probably can't get rid of it as lot's of
platforms rely on that, so it will have to be opt out. Make it the
platform's responsibility for ensuring a consistent state.

Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
userspace in order to make progress if dependencies are missing. Or
maybe just some timeout would be sufficient. I think this is probably
more useful for development than in a shipping product. Even if you
could fallback to polling mode instead of interrupts for example, I
doubt you would want to in a product.

You should also keep in mind that everything needed for a console has
to be built in. Maybe Android can say the console isn't needed, but in
general we can't.

Rob
Greg Kroah-Hartman June 12, 2019, 2:21 p.m. UTC | #7
On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> >
> > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > >
> > > > Hi Saravana,
> > > >
> > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > Why are you resending this rather than replying to Frank's last
> > > > > comments on the original?
> > > >
> > > > Adding on a different aspect...  The independent replies from three different
> > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > patch series.  There were also some implementation issues brought out.
> > > > (Although I refrained from bringing up most of my implementation issues
> > > > as they are not relevant until architecture issues are resolved.)
> > >
> > > Right, I'm not too worried about the implementation issues before we
> > > settle on the architectural issues. Those are easy to fix.
> > >
> > > Honestly, the main points that the maintainers raised are:
> > > 1) This is a configuration property and not describing the device.
> > > Just use the implicit dependencies coming from existing bindings.
> > >
> > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > property. But even if that's not something the maintainers can agree
> > > to, I gave a concrete example (cyclic dependencies between clock
> > > provider hardware) where the implicit dependencies would prevent one
> > > of the devices from probing till the end of time. So even if the
> > > maintainers don't agree we should always look at "depends-on" to
> > > decide the dependencies, we still need some means to override the
> > > implicit dependencies where they don't match the real dependency. Can
> > > we use depends-on as an override when the implicit dependencies aren't
> > > correct?
> > >
> > > 2) This doesn't need to be solved because this is just optimizing
> > > probing or saving power ("we should get rid of this auto disabling"):
> > >
> > > I explained why this patch series is not just about optimizing probe
> > > ordering or saving power. And why we can't ignore auto disabling
> > > (because it's more than just auto disabling). The kernel is currently
> > > broken when trying to use modules in ARM SoCs (probably in other
> > > systems/archs too, but I can't speak for those).
> > >
> > > 3) Concerns about backwards compatibility
> > >
> > > I pointed out why the current scheme (depends-on being the only source
> > > of dependency) doesn't break compatibility. And if we go with
> > > "depends-on" as an override what we could do to keep backwards
> > > compatibility. Happy to hear more thoughts or discuss options.
> > >
> > > 4) How the "sync_state" would work for a device that supplies multiple
> > > functionalities but a limited driver.
> >
> > <snip>
> > To be clear, all of above are _real_ problems that stops us from efficiently
> > load device drivers as modules for Android.
> >
> > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > the drawing board" is the ask, could you please point us in the right
> > direction?
> 
> Use the dependencies which are already there in DT. That's clocks,
> pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> going to accept duplicating all those dependencies in DT. The downside
> for the kernel is you have to address these one by one and can't have
> a generic property the driver core code can parse. After that's in
> place, then maybe we can consider handling any additional dependencies
> not already captured in DT. Once all that is in place, we can probably
> sort device and/or driver lists to optimize the probe order (maybe the
> driver core already does that now?).
> 
> Get rid of the auto disabling of clocks and regulators in
> late_initcall. It's simply not a valid marker that boot is done when
> modules are involved. We probably can't get rid of it as lot's of
> platforms rely on that, so it will have to be opt out. Make it the
> platform's responsibility for ensuring a consistent state.
> 
> Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> userspace in order to make progress if dependencies are missing.

People have tried to do this multiple times, and you never really know
when "boot is done" due to busses that have discoverable devices and
async probing of other busses.

You do know "something" when you pivot to a new boot disk, and when you
try to load init, but given initramfs and the fact that modules are
usually included on them, that's not really a good indication that
anything is "finished".

I don't want userspace to be responsible for telling the kernel, "hey
you should be finished now!", as that's an async notification that is
going to be ripe for problems.

I really like the "depends-on" information, as it shows a topology that
DT doesn't seem to be able to show today, yet we rely on it in the
kernel with the whole deferred probing mess.  To me, there doesn't seem
to be any other way to properly "know" this.

> Or
> maybe just some timeout would be sufficient. I think this is probably
> more useful for development than in a shipping product. Even if you
> could fallback to polling mode instead of interrupts for example, I
> doubt you would want to in a product.

timeouts suck.  And do not work for shipping products.  I want a device
with 100 modules that relys on DT to be able to boot just as fast as a
laptop with 100 modules that has all of the needed dependancies
described today in their bus topologies, because they used sane hardware
(i.e. PCI and ACPI).  Why hurt embedded people just because their
hardware relies on a system where you have to loop for long periods of
time because DT can not show the topology correctly?

> You should also keep in mind that everything needed for a console has
> to be built in. Maybe Android can say the console isn't needed, but in
> general we can't.

What does a console have to do with any of this?

confused,

greg k-h
Frank Rowand June 12, 2019, 4:07 p.m. UTC | #8
On 6/12/19 6:53 AM, Rob Herring wrote:
> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
>>
>> On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
>>> On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> Hi Saravana,
>>>>
>>>> On 6/10/19 10:36 AM, Rob Herring wrote:
>>>>> Why are you resending this rather than replying to Frank's last
>>>>> comments on the original?
>>>>
>>>> Adding on a different aspect...  The independent replies from three different
>>>> maintainers (Rob, Mark, myself) pointed out architectural issues with the
>>>> patch series.  There were also some implementation issues brought out.
>>>> (Although I refrained from bringing up most of my implementation issues
>>>> as they are not relevant until architecture issues are resolved.)
>>>
>>> Right, I'm not too worried about the implementation issues before we
>>> settle on the architectural issues. Those are easy to fix.
>>>
>>> Honestly, the main points that the maintainers raised are:
>>> 1) This is a configuration property and not describing the device.
>>> Just use the implicit dependencies coming from existing bindings.
>>>
>>> I gave a bunch of reasons for why I think it isn't an OS configuration
>>> property. But even if that's not something the maintainers can agree
>>> to, I gave a concrete example (cyclic dependencies between clock
>>> provider hardware) where the implicit dependencies would prevent one
>>> of the devices from probing till the end of time. So even if the
>>> maintainers don't agree we should always look at "depends-on" to
>>> decide the dependencies, we still need some means to override the
>>> implicit dependencies where they don't match the real dependency. Can
>>> we use depends-on as an override when the implicit dependencies aren't
>>> correct?
>>>
>>> 2) This doesn't need to be solved because this is just optimizing
>>> probing or saving power ("we should get rid of this auto disabling"):
>>>
>>> I explained why this patch series is not just about optimizing probe
>>> ordering or saving power. And why we can't ignore auto disabling
>>> (because it's more than just auto disabling). The kernel is currently
>>> broken when trying to use modules in ARM SoCs (probably in other
>>> systems/archs too, but I can't speak for those).
>>>
>>> 3) Concerns about backwards compatibility
>>>
>>> I pointed out why the current scheme (depends-on being the only source
>>> of dependency) doesn't break compatibility. And if we go with
>>> "depends-on" as an override what we could do to keep backwards
>>> compatibility. Happy to hear more thoughts or discuss options.
>>>
>>> 4) How the "sync_state" would work for a device that supplies multiple
>>> functionalities but a limited driver.
>>
>> <snip>
>> To be clear, all of above are _real_ problems that stops us from efficiently
>> load device drivers as modules for Android.
>>
>> So, if 'depends-on' doesn't seem like the right approach and "going back to
>> the drawing board" is the ask, could you please point us in the right
>> direction?
> 
> Use the dependencies which are already there in DT. That's clocks,
> pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> going to accept duplicating all those dependencies in DT. The downside
> for the kernel is you have to address these one by one and can't have
> a generic property the driver core code can parse. After that's in
> place, then maybe we can consider handling any additional dependencies
> not already captured in DT. Once all that is in place, we can probably
> sort device and/or driver lists to optimize the probe order (maybe the
> driver core already does that now?).
> 
> Get rid of the auto disabling of clocks and regulators in
> late_initcall. It's simply not a valid marker that boot is done when
> modules are involved. We probably can't get rid of it as lot's of
> platforms rely on that, so it will have to be opt out. Make it the
> platform's responsibility for ensuring a consistent state.

Setting aside modules for one moment, why is there any auto disabling
of clocks and regulators in late initcall????  Deferred probe processing
does not begin until deferred_probe_initcall() sets
driver_deferred_probe_enable to true.  No late_initcall function
should ever depend on ordering with respect to any other late_initcall.
(And yes, I know that among various initcall levels, there have been
games played to get a certain amount of ordering, but that is at
best fragile.)

In addition to modules, devicetree overlays need to be considered.

Just as modules can result in a driver appearing after boot finishes,
overlays can result in new devicetree nodes (and thus dependencies)
appearing after boot finishes.

-Frank

> 
> Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> userspace in order to make progress if dependencies are missing. Or
> maybe just some timeout would be sufficient. I think this is probably
> more useful for development than in a shipping product. Even if you
> could fallback to polling mode instead of interrupts for example, I
> doubt you would want to in a product.
> 
> You should also keep in mind that everything needed for a console has
> to be built in. Maybe Android can say the console isn't needed, but in
> general we can't.
> 
> Rob
> .
>
Frank Rowand June 12, 2019, 4:47 p.m. UTC | #9
On 6/12/19 9:07 AM, Frank Rowand wrote:
> On 6/12/19 6:53 AM, Rob Herring wrote:
>> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
>>>
>>> On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
>>>> On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>
>>>>> Hi Saravana,
>>>>>
>>>>> On 6/10/19 10:36 AM, Rob Herring wrote:
>>>>>> Why are you resending this rather than replying to Frank's last
>>>>>> comments on the original?
>>>>>
>>>>> Adding on a different aspect...  The independent replies from three different
>>>>> maintainers (Rob, Mark, myself) pointed out architectural issues with the
>>>>> patch series.  There were also some implementation issues brought out.
>>>>> (Although I refrained from bringing up most of my implementation issues
>>>>> as they are not relevant until architecture issues are resolved.)
>>>>
>>>> Right, I'm not too worried about the implementation issues before we
>>>> settle on the architectural issues. Those are easy to fix.
>>>>
>>>> Honestly, the main points that the maintainers raised are:
>>>> 1) This is a configuration property and not describing the device.
>>>> Just use the implicit dependencies coming from existing bindings.
>>>>
>>>> I gave a bunch of reasons for why I think it isn't an OS configuration
>>>> property. But even if that's not something the maintainers can agree
>>>> to, I gave a concrete example (cyclic dependencies between clock
>>>> provider hardware) where the implicit dependencies would prevent one
>>>> of the devices from probing till the end of time. So even if the
>>>> maintainers don't agree we should always look at "depends-on" to
>>>> decide the dependencies, we still need some means to override the
>>>> implicit dependencies where they don't match the real dependency. Can
>>>> we use depends-on as an override when the implicit dependencies aren't
>>>> correct?
>>>>
>>>> 2) This doesn't need to be solved because this is just optimizing
>>>> probing or saving power ("we should get rid of this auto disabling"):
>>>>
>>>> I explained why this patch series is not just about optimizing probe
>>>> ordering or saving power. And why we can't ignore auto disabling
>>>> (because it's more than just auto disabling). The kernel is currently
>>>> broken when trying to use modules in ARM SoCs (probably in other
>>>> systems/archs too, but I can't speak for those).
>>>>
>>>> 3) Concerns about backwards compatibility
>>>>
>>>> I pointed out why the current scheme (depends-on being the only source
>>>> of dependency) doesn't break compatibility. And if we go with
>>>> "depends-on" as an override what we could do to keep backwards
>>>> compatibility. Happy to hear more thoughts or discuss options.
>>>>
>>>> 4) How the "sync_state" would work for a device that supplies multiple
>>>> functionalities but a limited driver.
>>>
>>> <snip>
>>> To be clear, all of above are _real_ problems that stops us from efficiently
>>> load device drivers as modules for Android.
>>>
>>> So, if 'depends-on' doesn't seem like the right approach and "going back to
>>> the drawing board" is the ask, could you please point us in the right
>>> direction?
>>
>> Use the dependencies which are already there in DT. That's clocks,
>> pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
>> going to accept duplicating all those dependencies in DT. The downside
>> for the kernel is you have to address these one by one and can't have
>> a generic property the driver core code can parse. After that's in
>> place, then maybe we can consider handling any additional dependencies
>> not already captured in DT. Once all that is in place, we can probably
>> sort device and/or driver lists to optimize the probe order (maybe the
>> driver core already does that now?).
>>
>> Get rid of the auto disabling of clocks and regulators in
>> late_initcall. It's simply not a valid marker that boot is done when
>> modules are involved. We probably can't get rid of it as lot's of
>> platforms rely on that, so it will have to be opt out. Make it the
>> platform's responsibility for ensuring a consistent state.
> 
> Setting aside modules for one moment, why is there any auto disabling
> of clocks and regulators in late initcall????  Deferred probe processing
> does not begin until deferred_probe_initcall() sets

I failed to mention that deferred_probe_initcall() is a late_initcall.

-Frank

> driver_deferred_probe_enable to true.  No late_initcall function
> should ever depend on ordering with respect to any other late_initcall.
> (And yes, I know that among various initcall levels, there have been
> games played to get a certain amount of ordering, but that is at
> best fragile.)
> 
> In addition to modules, devicetree overlays need to be considered.
> 
> Just as modules can result in a driver appearing after boot finishes,
> overlays can result in new devicetree nodes (and thus dependencies)
> appearing after boot finishes.
> 
> -Frank
> 
>>
>> Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
>> userspace in order to make progress if dependencies are missing. Or
>> maybe just some timeout would be sufficient. I think this is probably
>> more useful for development than in a shipping product. Even if you
>> could fallback to polling mode instead of interrupts for example, I
>> doubt you would want to in a product.
>>
>> You should also keep in mind that everything needed for a console has
>> to be built in. Maybe Android can say the console isn't needed, but in
>> general we can't.
>>
>> Rob
>> .
>>
> 
>
Rob Herring June 12, 2019, 4:53 p.m. UTC | #10
On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > >
> > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > >
> > > > > Hi Saravana,
> > > > >
> > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > comments on the original?
> > > > >
> > > > > Adding on a different aspect...  The independent replies from three different
> > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > patch series.  There were also some implementation issues brought out.
> > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > as they are not relevant until architecture issues are resolved.)
> > > >
> > > > Right, I'm not too worried about the implementation issues before we
> > > > settle on the architectural issues. Those are easy to fix.
> > > >
> > > > Honestly, the main points that the maintainers raised are:
> > > > 1) This is a configuration property and not describing the device.
> > > > Just use the implicit dependencies coming from existing bindings.
> > > >
> > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > property. But even if that's not something the maintainers can agree
> > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > provider hardware) where the implicit dependencies would prevent one
> > > > of the devices from probing till the end of time. So even if the
> > > > maintainers don't agree we should always look at "depends-on" to
> > > > decide the dependencies, we still need some means to override the
> > > > implicit dependencies where they don't match the real dependency. Can
> > > > we use depends-on as an override when the implicit dependencies aren't
> > > > correct?
> > > >
> > > > 2) This doesn't need to be solved because this is just optimizing
> > > > probing or saving power ("we should get rid of this auto disabling"):
> > > >
> > > > I explained why this patch series is not just about optimizing probe
> > > > ordering or saving power. And why we can't ignore auto disabling
> > > > (because it's more than just auto disabling). The kernel is currently
> > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > systems/archs too, but I can't speak for those).
> > > >
> > > > 3) Concerns about backwards compatibility
> > > >
> > > > I pointed out why the current scheme (depends-on being the only source
> > > > of dependency) doesn't break compatibility. And if we go with
> > > > "depends-on" as an override what we could do to keep backwards
> > > > compatibility. Happy to hear more thoughts or discuss options.
> > > >
> > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > functionalities but a limited driver.
> > >
> > > <snip>
> > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > load device drivers as modules for Android.
> > >
> > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > the drawing board" is the ask, could you please point us in the right
> > > direction?
> >
> > Use the dependencies which are already there in DT. That's clocks,
> > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > going to accept duplicating all those dependencies in DT. The downside
> > for the kernel is you have to address these one by one and can't have
> > a generic property the driver core code can parse. After that's in
> > place, then maybe we can consider handling any additional dependencies
> > not already captured in DT. Once all that is in place, we can probably
> > sort device and/or driver lists to optimize the probe order (maybe the
> > driver core already does that now?).
> >
> > Get rid of the auto disabling of clocks and regulators in
> > late_initcall. It's simply not a valid marker that boot is done when
> > modules are involved. We probably can't get rid of it as lot's of
> > platforms rely on that, so it will have to be opt out. Make it the
> > platform's responsibility for ensuring a consistent state.
> >
> > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > userspace in order to make progress if dependencies are missing.
>
> People have tried to do this multiple times, and you never really know
> when "boot is done" due to busses that have discoverable devices and
> async probing of other busses.

Yes, I know which is why I proposed the second name with more limited
meaning/function.

> You do know "something" when you pivot to a new boot disk, and when you
> try to load init, but given initramfs and the fact that modules are
> usually included on them, that's not really a good indication that
> anything is "finished".
>
> I don't want userspace to be responsible for telling the kernel, "hey
> you should be finished now!", as that's an async notification that is
> going to be ripe for problems.

The usecase I care about here is when the DT has the dependency
information, but the kernel doesn't have the driver and the dependency
is never resolved. The same problem has to be solved with a
'depends-on' property. This easily happens with a new DT with added
dependencies like pinctrl and an old kernel that doesn't have the
"new" driver. Another example is IOMMUs. We need some way to say stop
waiting for dependencies. It is really just a debug option (of course,
how to prevent a debug option from being used in production?). This
works now for built-in cases with the same late_initcall abuse.

Using late_initcall_sync as an indicator has all the same problems
with userspace indicating boot finished. We should get rid of the
late_initcall_sync abuses and stop trying to work around them.

> I really like the "depends-on" information, as it shows a topology that
> DT doesn't seem to be able to show today, yet we rely on it in the
> kernel with the whole deferred probing mess.  To me, there doesn't seem
> to be any other way to properly "know" this.

As I said, DT *does* have this dependency information already. The
problem is the kernel probing doesn't use it. Fix that and then we can
discuss dependencies the DT doesn't provide that the kernel needs.

> > Or
> > maybe just some timeout would be sufficient. I think this is probably
> > more useful for development than in a shipping product. Even if you
> > could fallback to polling mode instead of interrupts for example, I
> > doubt you would want to in a product.
>
> timeouts suck.  And do not work for shipping products.  I want a device
> with 100 modules that relys on DT to be able to boot just as fast as a
> laptop with 100 modules that has all of the needed dependancies
> described today in their bus topologies, because they used sane hardware
> (i.e. PCI and ACPI).  Why hurt embedded people just because their
> hardware relies on a system where you have to loop for long periods of
> time because DT can not show the topology correctly?

I failed to list buses, but those too are already described in DT. Bus
dependencies are handled already.

> > You should also keep in mind that everything needed for a console has
> > to be built in. Maybe Android can say the console isn't needed, but in
> > general we can't.
>
> What does a console have to do with any of this?

Do you want to move all the SoC support to modules and have a console?
That doesn't work. Dependencies for the console have to be resolved
before initcalls are done. I don't recall the exact details, but I did
hit that issue when working on handling the missing dependency issues
(25b4e70dcce9 driver core: allow stopping deferred probe after init).

The point is that moving things to modules does introduce issues
beyond just dependency tracking.

Rob
Frank Rowand June 12, 2019, 5:03 p.m. UTC | #11
On 6/12/19 7:21 AM, Greg Kroah-Hartman wrote:
> On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
>> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
>>>
>>> On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
>>>> On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>
>>>>> Hi Saravana,
>>>>>
>>>>> On 6/10/19 10:36 AM, Rob Herring wrote:
>>>>>> Why are you resending this rather than replying to Frank's last
>>>>>> comments on the original?
>>>>>
>>>>> Adding on a different aspect...  The independent replies from three different
>>>>> maintainers (Rob, Mark, myself) pointed out architectural issues with the
>>>>> patch series.  There were also some implementation issues brought out.
>>>>> (Although I refrained from bringing up most of my implementation issues
>>>>> as they are not relevant until architecture issues are resolved.)
>>>>
>>>> Right, I'm not too worried about the implementation issues before we
>>>> settle on the architectural issues. Those are easy to fix.
>>>>
>>>> Honestly, the main points that the maintainers raised are:
>>>> 1) This is a configuration property and not describing the device.
>>>> Just use the implicit dependencies coming from existing bindings.
>>>>
>>>> I gave a bunch of reasons for why I think it isn't an OS configuration
>>>> property. But even if that's not something the maintainers can agree
>>>> to, I gave a concrete example (cyclic dependencies between clock
>>>> provider hardware) where the implicit dependencies would prevent one
>>>> of the devices from probing till the end of time. So even if the
>>>> maintainers don't agree we should always look at "depends-on" to
>>>> decide the dependencies, we still need some means to override the
>>>> implicit dependencies where they don't match the real dependency. Can
>>>> we use depends-on as an override when the implicit dependencies aren't
>>>> correct?
>>>>
>>>> 2) This doesn't need to be solved because this is just optimizing
>>>> probing or saving power ("we should get rid of this auto disabling"):
>>>>
>>>> I explained why this patch series is not just about optimizing probe
>>>> ordering or saving power. And why we can't ignore auto disabling
>>>> (because it's more than just auto disabling). The kernel is currently
>>>> broken when trying to use modules in ARM SoCs (probably in other
>>>> systems/archs too, but I can't speak for those).
>>>>
>>>> 3) Concerns about backwards compatibility
>>>>
>>>> I pointed out why the current scheme (depends-on being the only source
>>>> of dependency) doesn't break compatibility. And if we go with
>>>> "depends-on" as an override what we could do to keep backwards
>>>> compatibility. Happy to hear more thoughts or discuss options.
>>>>
>>>> 4) How the "sync_state" would work for a device that supplies multiple
>>>> functionalities but a limited driver.
>>>
>>> <snip>
>>> To be clear, all of above are _real_ problems that stops us from efficiently
>>> load device drivers as modules for Android.
>>>
>>> So, if 'depends-on' doesn't seem like the right approach and "going back to
>>> the drawing board" is the ask, could you please point us in the right
>>> direction?
>>
>> Use the dependencies which are already there in DT. That's clocks,
>> pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
>> going to accept duplicating all those dependencies in DT. The downside
>> for the kernel is you have to address these one by one and can't have
>> a generic property the driver core code can parse. After that's in
>> place, then maybe we can consider handling any additional dependencies
>> not already captured in DT. Once all that is in place, we can probably
>> sort device and/or driver lists to optimize the probe order (maybe the
>> driver core already does that now?).
>>
>> Get rid of the auto disabling of clocks and regulators in
>> late_initcall. It's simply not a valid marker that boot is done when
>> modules are involved. We probably can't get rid of it as lot's of
>> platforms rely on that, so it will have to be opt out. Make it the
>> platform's responsibility for ensuring a consistent state.
>>
>> Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
>> userspace in order to make progress if dependencies are missing.
> 
> People have tried to do this multiple times, and you never really know
> when "boot is done" due to busses that have discoverable devices and
> async probing of other busses.
> 
> You do know "something" when you pivot to a new boot disk, and when you
> try to load init, but given initramfs and the fact that modules are
> usually included on them, that's not really a good indication that
> anything is "finished".
> 
> I don't want userspace to be responsible for telling the kernel, "hey
> you should be finished now!", as that's an async notification that is
> going to be ripe for problems.
> 
> I really like the "depends-on" information, as it shows a topology that
> DT doesn't seem to be able to show today, yet we rely on it in the
> kernel with the whole deferred probing mess.  To me, there doesn't seem
> to be any other way to properly "know" this.

Rob pointed out (above) the dependencies that are already contained in
devicetree.  Those are real hardware descriptions that have to be
correct or the devices will not use the hardware that they "depend on"
(thus an incorrect hardware description _also_ results in the resource
that is not described not actually being a dependency).

If you add a second set of properties purely to list dependencies then
the second set of dependencies is likely to have latent errors that are
not initially detected (when the property is first introduced) because
the proper resources just happen to be available by sheer chance.  Then
a later change in boot order resulting from a code change or configuration
change can expose that latent error.

A second set of dependency properties is making boot more fragile.

-Frank

> 
>> Or
>> maybe just some timeout would be sufficient. I think this is probably
>> more useful for development than in a shipping product. Even if you
>> could fallback to polling mode instead of interrupts for example, I
>> doubt you would want to in a product.
> 
> timeouts suck.  And do not work for shipping products.  I want a device
> with 100 modules that relys on DT to be able to boot just as fast as a
> laptop with 100 modules that has all of the needed dependancies
> described today in their bus topologies, because they used sane hardware
> (i.e. PCI and ACPI).  Why hurt embedded people just because their
> hardware relies on a system where you have to loop for long periods of
> time because DT can not show the topology correctly?
> 
>> You should also keep in mind that everything needed for a console has
>> to be built in. Maybe Android can say the console isn't needed, but in
>> general we can't.
> 
> What does a console have to do with any of this?
> 
> confused,
> 
> greg k-h
>
Greg Kroah-Hartman June 12, 2019, 5:08 p.m. UTC | #12
On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > >
> > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > >
> > > > > > Hi Saravana,
> > > > > >
> > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > comments on the original?
> > > > > >
> > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > as they are not relevant until architecture issues are resolved.)
> > > > >
> > > > > Right, I'm not too worried about the implementation issues before we
> > > > > settle on the architectural issues. Those are easy to fix.
> > > > >
> > > > > Honestly, the main points that the maintainers raised are:
> > > > > 1) This is a configuration property and not describing the device.
> > > > > Just use the implicit dependencies coming from existing bindings.
> > > > >
> > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > property. But even if that's not something the maintainers can agree
> > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > of the devices from probing till the end of time. So even if the
> > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > decide the dependencies, we still need some means to override the
> > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > correct?
> > > > >
> > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > >
> > > > > I explained why this patch series is not just about optimizing probe
> > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > systems/archs too, but I can't speak for those).
> > > > >
> > > > > 3) Concerns about backwards compatibility
> > > > >
> > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > "depends-on" as an override what we could do to keep backwards
> > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > >
> > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > functionalities but a limited driver.
> > > >
> > > > <snip>
> > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > load device drivers as modules for Android.
> > > >
> > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > the drawing board" is the ask, could you please point us in the right
> > > > direction?
> > >
> > > Use the dependencies which are already there in DT. That's clocks,
> > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > going to accept duplicating all those dependencies in DT. The downside
> > > for the kernel is you have to address these one by one and can't have
> > > a generic property the driver core code can parse. After that's in
> > > place, then maybe we can consider handling any additional dependencies
> > > not already captured in DT. Once all that is in place, we can probably
> > > sort device and/or driver lists to optimize the probe order (maybe the
> > > driver core already does that now?).
> > >
> > > Get rid of the auto disabling of clocks and regulators in
> > > late_initcall. It's simply not a valid marker that boot is done when
> > > modules are involved. We probably can't get rid of it as lot's of
> > > platforms rely on that, so it will have to be opt out. Make it the
> > > platform's responsibility for ensuring a consistent state.
> > >
> > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > userspace in order to make progress if dependencies are missing.
> >
> > People have tried to do this multiple times, and you never really know
> > when "boot is done" due to busses that have discoverable devices and
> > async probing of other busses.
> 
> Yes, I know which is why I proposed the second name with more limited
> meaning/function.

I still don't want to have the kernel have to rely on this.

> > You do know "something" when you pivot to a new boot disk, and when you
> > try to load init, but given initramfs and the fact that modules are
> > usually included on them, that's not really a good indication that
> > anything is "finished".
> >
> > I don't want userspace to be responsible for telling the kernel, "hey
> > you should be finished now!", as that's an async notification that is
> > going to be ripe for problems.
> 
> The usecase I care about here is when the DT has the dependency
> information, but the kernel doesn't have the driver and the dependency
> is never resolved.

Then we have the same situation as today and nothing different happens,
right?

> The same problem has to be solved with a
> 'depends-on' property. This easily happens with a new DT with added
> dependencies like pinctrl and an old kernel that doesn't have the
> "new" driver. Another example is IOMMUs. We need some way to say stop
> waiting for dependencies. It is really just a debug option (of course,
> how to prevent a debug option from being used in production?). This
> works now for built-in cases with the same late_initcall abuse.

What is a debug option?  We need something "for real".

> Using late_initcall_sync as an indicator has all the same problems
> with userspace indicating boot finished. We should get rid of the
> late_initcall_sync abuses and stop trying to work around them.

I agree, but that's not the issue here.

> > I really like the "depends-on" information, as it shows a topology that
> > DT doesn't seem to be able to show today, yet we rely on it in the
> > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > to be any other way to properly "know" this.
> 
> As I said, DT *does* have this dependency information already. The
> problem is the kernel probing doesn't use it. Fix that and then we can
> discuss dependencies the DT doesn't provide that the kernel needs.

Where can the kernel probing be fixed to use it?  What am I missing that
can be done instead of what this patchset does?

> > > Or
> > > maybe just some timeout would be sufficient. I think this is probably
> > > more useful for development than in a shipping product. Even if you
> > > could fallback to polling mode instead of interrupts for example, I
> > > doubt you would want to in a product.
> >
> > timeouts suck.  And do not work for shipping products.  I want a device
> > with 100 modules that relys on DT to be able to boot just as fast as a
> > laptop with 100 modules that has all of the needed dependancies
> > described today in their bus topologies, because they used sane hardware
> > (i.e. PCI and ACPI).  Why hurt embedded people just because their
> > hardware relies on a system where you have to loop for long periods of
> > time because DT can not show the topology correctly?
> 
> I failed to list buses, but those too are already described in DT. Bus
> dependencies are handled already.

That's not my point, I know sane busses are described in DT :)

> > > You should also keep in mind that everything needed for a console has
> > > to be built in. Maybe Android can say the console isn't needed, but in
> > > general we can't.
> >
> > What does a console have to do with any of this?
> 
> Do you want to move all the SoC support to modules and have a console?

That's not the issue/point here at all.  I really could care less about
a console, the issue is that we need a way to take a "generic" arm64
kernel, with a load of modules (i.e. allmodconfig) and have it boot in a
sane amount of time.

Other arches do this today, why can't arm64?  That is the issue here,
not console or late init calls.

> That doesn't work. Dependencies for the console have to be resolved
> before initcalls are done. I don't recall the exact details, but I did
> hit that issue when working on handling the missing dependency issues
> (25b4e70dcce9 driver core: allow stopping deferred probe after init).
> 
> The point is that moving things to modules does introduce issues
> beyond just dependency tracking.

I totally agree, and yet here we both are typing on Linux machines where
this all "just works" today.  Now to get that to work on a DT-based
system :)

thanks,

greg k-h
Rob Herring June 12, 2019, 6:19 p.m. UTC | #13
On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> > On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > > >
> > > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Saravana,
> > > > > > >
> > > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > > comments on the original?
> > > > > > >
> > > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > > as they are not relevant until architecture issues are resolved.)
> > > > > >
> > > > > > Right, I'm not too worried about the implementation issues before we
> > > > > > settle on the architectural issues. Those are easy to fix.
> > > > > >
> > > > > > Honestly, the main points that the maintainers raised are:
> > > > > > 1) This is a configuration property and not describing the device.
> > > > > > Just use the implicit dependencies coming from existing bindings.
> > > > > >
> > > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > > property. But even if that's not something the maintainers can agree
> > > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > > of the devices from probing till the end of time. So even if the
> > > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > > decide the dependencies, we still need some means to override the
> > > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > > correct?
> > > > > >
> > > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > > >
> > > > > > I explained why this patch series is not just about optimizing probe
> > > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > > systems/archs too, but I can't speak for those).
> > > > > >
> > > > > > 3) Concerns about backwards compatibility
> > > > > >
> > > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > > "depends-on" as an override what we could do to keep backwards
> > > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > > >
> > > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > > functionalities but a limited driver.
> > > > >
> > > > > <snip>
> > > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > > load device drivers as modules for Android.
> > > > >
> > > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > > the drawing board" is the ask, could you please point us in the right
> > > > > direction?
> > > >
> > > > Use the dependencies which are already there in DT. That's clocks,
> > > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > > going to accept duplicating all those dependencies in DT. The downside
> > > > for the kernel is you have to address these one by one and can't have
> > > > a generic property the driver core code can parse. After that's in
> > > > place, then maybe we can consider handling any additional dependencies
> > > > not already captured in DT. Once all that is in place, we can probably
> > > > sort device and/or driver lists to optimize the probe order (maybe the
> > > > driver core already does that now?).
> > > >
> > > > Get rid of the auto disabling of clocks and regulators in
> > > > late_initcall. It's simply not a valid marker that boot is done when
> > > > modules are involved. We probably can't get rid of it as lot's of
> > > > platforms rely on that, so it will have to be opt out. Make it the
> > > > platform's responsibility for ensuring a consistent state.
> > > >
> > > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > > userspace in order to make progress if dependencies are missing.
> > >
> > > People have tried to do this multiple times, and you never really know
> > > when "boot is done" due to busses that have discoverable devices and
> > > async probing of other busses.
> >
> > Yes, I know which is why I proposed the second name with more limited
> > meaning/function.
>
> I still don't want to have the kernel have to rely on this.
>
> > > You do know "something" when you pivot to a new boot disk, and when you
> > > try to load init, but given initramfs and the fact that modules are
> > > usually included on them, that's not really a good indication that
> > > anything is "finished".
> > >
> > > I don't want userspace to be responsible for telling the kernel, "hey
> > > you should be finished now!", as that's an async notification that is
> > > going to be ripe for problems.
> >
> > The usecase I care about here is when the DT has the dependency
> > information, but the kernel doesn't have the driver and the dependency
> > is never resolved.
>
> Then we have the same situation as today and nothing different happens,
> right?

Huh?

This works today, but not for modules.

>
> > The same problem has to be solved with a
> > 'depends-on' property. This easily happens with a new DT with added
> > dependencies like pinctrl and an old kernel that doesn't have the
> > "new" driver. Another example is IOMMUs. We need some way to say stop
> > waiting for dependencies. It is really just a debug option (of course,
> > how to prevent a debug option from being used in production?). This
> > works now for built-in cases with the same late_initcall abuse.
>
> What is a debug option?  We need something "for real".
>
> > Using late_initcall_sync as an indicator has all the same problems
> > with userspace indicating boot finished. We should get rid of the
> > late_initcall_sync abuses and stop trying to work around them.
>
> I agree, but that's not the issue here.

It is because the cover letter mentions it and downstream work around it.

> > > I really like the "depends-on" information, as it shows a topology that
> > > DT doesn't seem to be able to show today, yet we rely on it in the
> > > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > > to be any other way to properly "know" this.
> >
> > As I said, DT *does* have this dependency information already. The
> > problem is the kernel probing doesn't use it. Fix that and then we can
> > discuss dependencies the DT doesn't provide that the kernel needs.
>
> Where can the kernel probing be fixed to use it?  What am I missing that
> can be done instead of what this patchset does?

Somewhere, either in each subsystem or in the DT or core code creating
struct devices, you need to iterate thru the dependencies. Take clocks
as an example:

for each node:
  for each 'clocks' phandle
    Lookup struct device from clock phandle
    Add the clock provider struct device to node's struct device links

Now, repeat this for regulators, interrupts, etc.

This series is pretty much doing the same thing, you just have to
parse each provider rather than only 'depends-on'.

One issue is the struct device for the dependency may not be created
yet. I think this series would have the same issue, but haven't dug
into how it avoids that or whether it just ignores it and falls back
to deferring probe.

I'm also not clear on how you create struct devices and add
dependencies before probing gets attempted. If a driver is already
registered, probe is going to be attempted before any dependencies are
added. I guess the issue is avoided with drivers being modules, but
any solution should work for built-in too.

Rob
Frank Rowand June 12, 2019, 7:03 p.m. UTC | #14
On 6/12/19 6:53 AM, Rob Herring wrote:
> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
>>
>> On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
>>> On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> Hi Saravana,
>>>>
>>>> On 6/10/19 10:36 AM, Rob Herring wrote:
>>>>> Why are you resending this rather than replying to Frank's last
>>>>> comments on the original?
>>>>
>>>> Adding on a different aspect...  The independent replies from three different
>>>> maintainers (Rob, Mark, myself) pointed out architectural issues with the
>>>> patch series.  There were also some implementation issues brought out.
>>>> (Although I refrained from bringing up most of my implementation issues
>>>> as they are not relevant until architecture issues are resolved.)
>>>
>>> Right, I'm not too worried about the implementation issues before we
>>> settle on the architectural issues. Those are easy to fix.
>>>
>>> Honestly, the main points that the maintainers raised are:
>>> 1) This is a configuration property and not describing the device.
>>> Just use the implicit dependencies coming from existing bindings.
>>>
>>> I gave a bunch of reasons for why I think it isn't an OS configuration
>>> property. But even if that's not something the maintainers can agree
>>> to, I gave a concrete example (cyclic dependencies between clock
>>> provider hardware) where the implicit dependencies would prevent one
>>> of the devices from probing till the end of time. So even if the
>>> maintainers don't agree we should always look at "depends-on" to
>>> decide the dependencies, we still need some means to override the
>>> implicit dependencies where they don't match the real dependency. Can
>>> we use depends-on as an override when the implicit dependencies aren't
>>> correct?
>>>
>>> 2) This doesn't need to be solved because this is just optimizing
>>> probing or saving power ("we should get rid of this auto disabling"):
>>>
>>> I explained why this patch series is not just about optimizing probe
>>> ordering or saving power. And why we can't ignore auto disabling
>>> (because it's more than just auto disabling). The kernel is currently
>>> broken when trying to use modules in ARM SoCs (probably in other
>>> systems/archs too, but I can't speak for those).
>>>
>>> 3) Concerns about backwards compatibility
>>>
>>> I pointed out why the current scheme (depends-on being the only source
>>> of dependency) doesn't break compatibility. And if we go with
>>> "depends-on" as an override what we could do to keep backwards
>>> compatibility. Happy to hear more thoughts or discuss options.
>>>
>>> 4) How the "sync_state" would work for a device that supplies multiple
>>> functionalities but a limited driver.
>>
>> <snip>
>> To be clear, all of above are _real_ problems that stops us from efficiently
>> load device drivers as modules for Android.
>>
>> So, if 'depends-on' doesn't seem like the right approach and "going back to
>> the drawing board" is the ask, could you please point us in the right
>> direction?
> 


> Use the dependencies which are already there in DT. That's clocks,
> pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> going to accept duplicating all those dependencies in DT. The downside

Just in case it isn't obvious from my other comments, I'm fully in
agreement with Rob on this.

-Frank

> for the kernel is you have to address these one by one and can't have
> a generic property the driver core code can parse. After that's in
> place, then maybe we can consider handling any additional dependencies
> not already captured in DT. Once all that is in place, we can probably
> sort device and/or driver lists to optimize the probe order (maybe the
> driver core already does that now?).
> 
> Get rid of the auto disabling of clocks and regulators in
> late_initcall. It's simply not a valid marker that boot is done when
> modules are involved. We probably can't get rid of it as lot's of
> platforms rely on that, so it will have to be opt out. Make it the
> platform's responsibility for ensuring a consistent state.
> 
> Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> userspace in order to make progress if dependencies are missing. Or
> maybe just some timeout would be sufficient. I think this is probably
> more useful for development than in a shipping product. Even if you
> could fallback to polling mode instead of interrupts for example, I
> doubt you would want to in a product.
> 
> You should also keep in mind that everything needed for a console has
> to be built in. Maybe Android can say the console isn't needed, but in
> general we can't.
> 
> Rob
> .
>
Saravana Kannan June 12, 2019, 7:29 p.m. UTC | #15
On Wed, Jun 12, 2019 at 11:19 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> > > On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Saravana,
> > > > > > > >
> > > > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > > > comments on the original?
> > > > > > > >
> > > > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > > > as they are not relevant until architecture issues are resolved.)
> > > > > > >
> > > > > > > Right, I'm not too worried about the implementation issues before we
> > > > > > > settle on the architectural issues. Those are easy to fix.
> > > > > > >
> > > > > > > Honestly, the main points that the maintainers raised are:
> > > > > > > 1) This is a configuration property and not describing the device.
> > > > > > > Just use the implicit dependencies coming from existing bindings.
> > > > > > >
> > > > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > > > property. But even if that's not something the maintainers can agree
> > > > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > > > of the devices from probing till the end of time. So even if the
> > > > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > > > decide the dependencies, we still need some means to override the
> > > > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > > > correct?
> > > > > > >
> > > > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > > > >
> > > > > > > I explained why this patch series is not just about optimizing probe
> > > > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > > > systems/archs too, but I can't speak for those).
> > > > > > >
> > > > > > > 3) Concerns about backwards compatibility
> > > > > > >
> > > > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > > > "depends-on" as an override what we could do to keep backwards
> > > > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > > > >
> > > > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > > > functionalities but a limited driver.
> > > > > >
> > > > > > <snip>
> > > > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > > > load device drivers as modules for Android.
> > > > > >
> > > > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > > > the drawing board" is the ask, could you please point us in the right
> > > > > > direction?
> > > > >
> > > > > Use the dependencies which are already there in DT. That's clocks,
> > > > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > > > going to accept duplicating all those dependencies in DT. The downside
> > > > > for the kernel is you have to address these one by one and can't have
> > > > > a generic property the driver core code can parse. After that's in
> > > > > place, then maybe we can consider handling any additional dependencies
> > > > > not already captured in DT. Once all that is in place, we can probably
> > > > > sort device and/or driver lists to optimize the probe order (maybe the
> > > > > driver core already does that now?).
> > > > >
> > > > > Get rid of the auto disabling of clocks and regulators in
> > > > > late_initcall. It's simply not a valid marker that boot is done when
> > > > > modules are involved. We probably can't get rid of it as lot's of
> > > > > platforms rely on that, so it will have to be opt out. Make it the
> > > > > platform's responsibility for ensuring a consistent state.
> > > > >
> > > > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > > > userspace in order to make progress if dependencies are missing.
> > > >
> > > > People have tried to do this multiple times, and you never really know
> > > > when "boot is done" due to busses that have discoverable devices and
> > > > async probing of other busses.
> > >
> > > Yes, I know which is why I proposed the second name with more limited
> > > meaning/function.
> >
> > I still don't want to have the kernel have to rely on this.
> >
> > > > You do know "something" when you pivot to a new boot disk, and when you
> > > > try to load init, but given initramfs and the fact that modules are
> > > > usually included on them, that's not really a good indication that
> > > > anything is "finished".
> > > >
> > > > I don't want userspace to be responsible for telling the kernel, "hey
> > > > you should be finished now!", as that's an async notification that is
> > > > going to be ripe for problems.
> > >
> > > The usecase I care about here is when the DT has the dependency
> > > information, but the kernel doesn't have the driver and the dependency
> > > is never resolved.
> >
> > Then we have the same situation as today and nothing different happens,
> > right?
>
> Huh?
>
> This works today, but not for modules.

Replying to this a bit further down.

>
> >
> > > The same problem has to be solved with a
> > > 'depends-on' property. This easily happens with a new DT with added
> > > dependencies like pinctrl and an old kernel that doesn't have the
> > > "new" driver.

Isn't this the perfect example of an "implicit dependency" in a DT
node not being a mandatory dependency? The old kernel worked fine with
older DT without the added pinctrl dependency, so treating it as a
mandatory dependency seems wrong for that particular device?
depends-on avoids all this because the older kernel won't parse
depends-on. And for newer kernels, it'll parse only what depends-on
says are dependencies and not make wrong assumptions.

> > > Another example is IOMMUs. We need some way to say stop
> > > waiting for dependencies. It is really just a debug option (of course,
> > > how to prevent a debug option from being used in production?). This
> > > works now for built-in cases with the same late_initcall abuse.
> >
> > What is a debug option?  We need something "for real".
> >
> > > Using late_initcall_sync as an indicator has all the same problems
> > > with userspace indicating boot finished. We should get rid of the
> > > late_initcall_sync abuses and stop trying to work around them.
> >
> > I agree, but that's not the issue here.
>
> It is because the cover letter mentions it and downstream work around it.

This patch series is trying to remove the use of late_initcall_sync
and instead relying on dependency information coming from DT. So, you
are agreeing with the patchset.

> > > > I really like the "depends-on" information, as it shows a topology that
> > > > DT doesn't seem to be able to show today, yet we rely on it in the
> > > > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > > > to be any other way to properly "know" this.
> > >
> > > As I said, DT *does* have this dependency information already. The
> > > problem is the kernel probing doesn't use it. Fix that and then we can
> > > discuss dependencies the DT doesn't provide that the kernel needs.
> >
> > Where can the kernel probing be fixed to use it?  What am I missing that
> > can be done instead of what this patchset does?
>
> Somewhere, either in each subsystem or in the DT or core code creating
> struct devices, you need to iterate thru the dependencies. Take clocks
> as an example:
>
> for each node:
>   for each 'clocks' phandle
>     Lookup struct device from clock phandle
>     Add the clock provider struct device to node's struct device links
>
> Now, repeat this for regulators, interrupts, etc.

I'm more than happy to do this if the maintainers can accept this as a
solution, but then we need to agree that we need an override property
if the implicit dependency isn't a mandatory dependency. We also need
to agree on how we do this without breaking backwards compatibility.
Either as a config option for this feature or have a property in the
"chosen" node to say it's okay to assume existing bindings imply
mandatory dependencies (it's just describing the DT more explicitly
and the kernel will use it to enable this feature).

Although regulator binding are a "problem" because the kernel will
have to parse every property in a node to check if it ends with
-supply and then treat it as if it's a regulator binding (even though
it might not be). So regulators will need some kind of "opt out" in
the kernel (not DT).

> This series is pretty much doing the same thing, you just have to
> parse each provider rather than only 'depends-on'.
>
> One issue is the struct device for the dependency may not be created
> yet. I think this series would have the same issue, but haven't dug
> into how it avoids that or whether it just ignores it and falls back
> to deferring probe.

The patch series handles this properly and doesn't fall back to
deferred probing.

> I'm also not clear on how you create struct devices and add
> dependencies before probing gets attempted. If a driver is already
> registered, probe is going to be attempted before any dependencies are
> added. I guess the issue is avoided with drivers being modules, but
> any solution should work for built-in too.

This is also handled properly in the series.

I've actually boot tested both these scenarios you call out and the
patch series handles them properly.

But you are missing the main point here. The goal isn't to just
eliminate deferred probing (it's a great side effect even it it just
stops 99% of them), but also remove the bad assumption that
late_initcall_sync() means all the devices are probed. The suppliers
need a better signal (which the patch series provides) to tell when
they can "unfreeze" the resources left on at boot.

It's true that device tree overlays can be added after userspace comes
up, but in those cases whoever is adding the device tree nodes can make
sure that the resources needed by the "to be added overlay devices" are
kept at the right level. It's also unlikely that the bootloader is
leaving resources
on for these overlay devices because they might never be added.

And even if it doesn't work perfectly for instances with overlays
(neither does the
current kernel), it's still better to fix it for the next
million/billion devices that'll use
ARM without post boot overlays.

Thanks,
Saravana
Frank Rowand June 12, 2019, 8:23 p.m. UTC | #16
On 6/12/19 12:29 PM, Saravana Kannan wrote:
> On Wed, Jun 12, 2019 at 11:19 AM Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
>>>> On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>
>>>>> On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
>>>>>> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
>>>>>>>
>>>>>>> On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
>>>>>>>> On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Saravana,
>>>>>>>>>
>>>>>>>>> On 6/10/19 10:36 AM, Rob Herring wrote:
>>>>>>>>>> Why are you resending this rather than replying to Frank's last
>>>>>>>>>> comments on the original?
>>>>>>>>>
>>>>>>>>> Adding on a different aspect...  The independent replies from three different
>>>>>>>>> maintainers (Rob, Mark, myself) pointed out architectural issues with the
>>>>>>>>> patch series.  There were also some implementation issues brought out.
>>>>>>>>> (Although I refrained from bringing up most of my implementation issues
>>>>>>>>> as they are not relevant until architecture issues are resolved.)
>>>>>>>>
>>>>>>>> Right, I'm not too worried about the implementation issues before we
>>>>>>>> settle on the architectural issues. Those are easy to fix.
>>>>>>>>
>>>>>>>> Honestly, the main points that the maintainers raised are:
>>>>>>>> 1) This is a configuration property and not describing the device.
>>>>>>>> Just use the implicit dependencies coming from existing bindings.
>>>>>>>>
>>>>>>>> I gave a bunch of reasons for why I think it isn't an OS configuration
>>>>>>>> property. But even if that's not something the maintainers can agree
>>>>>>>> to, I gave a concrete example (cyclic dependencies between clock
>>>>>>>> provider hardware) where the implicit dependencies would prevent one
>>>>>>>> of the devices from probing till the end of time. So even if the
>>>>>>>> maintainers don't agree we should always look at "depends-on" to
>>>>>>>> decide the dependencies, we still need some means to override the
>>>>>>>> implicit dependencies where they don't match the real dependency. Can
>>>>>>>> we use depends-on as an override when the implicit dependencies aren't
>>>>>>>> correct?
>>>>>>>>
>>>>>>>> 2) This doesn't need to be solved because this is just optimizing
>>>>>>>> probing or saving power ("we should get rid of this auto disabling"):
>>>>>>>>
>>>>>>>> I explained why this patch series is not just about optimizing probe
>>>>>>>> ordering or saving power. And why we can't ignore auto disabling
>>>>>>>> (because it's more than just auto disabling). The kernel is currently
>>>>>>>> broken when trying to use modules in ARM SoCs (probably in other
>>>>>>>> systems/archs too, but I can't speak for those).
>>>>>>>>
>>>>>>>> 3) Concerns about backwards compatibility
>>>>>>>>
>>>>>>>> I pointed out why the current scheme (depends-on being the only source
>>>>>>>> of dependency) doesn't break compatibility. And if we go with
>>>>>>>> "depends-on" as an override what we could do to keep backwards
>>>>>>>> compatibility. Happy to hear more thoughts or discuss options.
>>>>>>>>
>>>>>>>> 4) How the "sync_state" would work for a device that supplies multiple
>>>>>>>> functionalities but a limited driver.
>>>>>>>
>>>>>>> <snip>
>>>>>>> To be clear, all of above are _real_ problems that stops us from efficiently
>>>>>>> load device drivers as modules for Android.
>>>>>>>
>>>>>>> So, if 'depends-on' doesn't seem like the right approach and "going back to
>>>>>>> the drawing board" is the ask, could you please point us in the right
>>>>>>> direction?
>>>>>>
>>>>>> Use the dependencies which are already there in DT. That's clocks,
>>>>>> pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
>>>>>> going to accept duplicating all those dependencies in DT. The downside
>>>>>> for the kernel is you have to address these one by one and can't have
>>>>>> a generic property the driver core code can parse. After that's in
>>>>>> place, then maybe we can consider handling any additional dependencies
>>>>>> not already captured in DT. Once all that is in place, we can probably
>>>>>> sort device and/or driver lists to optimize the probe order (maybe the
>>>>>> driver core already does that now?).
>>>>>>
>>>>>> Get rid of the auto disabling of clocks and regulators in
>>>>>> late_initcall. It's simply not a valid marker that boot is done when
>>>>>> modules are involved. We probably can't get rid of it as lot's of
>>>>>> platforms rely on that, so it will have to be opt out. Make it the
>>>>>> platform's responsibility for ensuring a consistent state.
>>>>>>
>>>>>> Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
>>>>>> userspace in order to make progress if dependencies are missing.
>>>>>
>>>>> People have tried to do this multiple times, and you never really know
>>>>> when "boot is done" due to busses that have discoverable devices and
>>>>> async probing of other busses.
>>>>
>>>> Yes, I know which is why I proposed the second name with more limited
>>>> meaning/function.
>>>
>>> I still don't want to have the kernel have to rely on this.
>>>
>>>>> You do know "something" when you pivot to a new boot disk, and when you
>>>>> try to load init, but given initramfs and the fact that modules are
>>>>> usually included on them, that's not really a good indication that
>>>>> anything is "finished".
>>>>>
>>>>> I don't want userspace to be responsible for telling the kernel, "hey
>>>>> you should be finished now!", as that's an async notification that is
>>>>> going to be ripe for problems.
>>>>
>>>> The usecase I care about here is when the DT has the dependency
>>>> information, but the kernel doesn't have the driver and the dependency
>>>> is never resolved.
>>>
>>> Then we have the same situation as today and nothing different happens,
>>> right?
>>
>> Huh?
>>
>> This works today, but not for modules.
> 
> Replying to this a bit further down.
> 
>>
>>>
>>>> The same problem has to be solved with a
>>>> 'depends-on' property. This easily happens with a new DT with added
>>>> dependencies like pinctrl and an old kernel that doesn't have the
>>>> "new" driver.
> 
> Isn't this the perfect example of an "implicit dependency" in a DT
> node not being a mandatory dependency? The old kernel worked fine with
> older DT without the added pinctrl dependency, so treating it as a
> mandatory dependency seems wrong for that particular device?
> depends-on avoids all this because the older kernel won't parse
> depends-on. And for newer kernels, it'll parse only what depends-on
> says are dependencies and not make wrong assumptions.
> 
>>>> Another example is IOMMUs. We need some way to say stop
>>>> waiting for dependencies. It is really just a debug option (of course,
>>>> how to prevent a debug option from being used in production?). This
>>>> works now for built-in cases with the same late_initcall abuse.
>>>
>>> What is a debug option?  We need something "for real".
>>>
>>>> Using late_initcall_sync as an indicator has all the same problems
>>>> with userspace indicating boot finished. We should get rid of the
>>>> late_initcall_sync abuses and stop trying to work around them.
>>>
>>> I agree, but that's not the issue here.
>>
>> It is because the cover letter mentions it and downstream work around it.
> 
> This patch series is trying to remove the use of late_initcall_sync
> and instead relying on dependency information coming from DT. So, you
> are agreeing with the patchset.
> 
>>>>> I really like the "depends-on" information, as it shows a topology that
>>>>> DT doesn't seem to be able to show today, yet we rely on it in the
>>>>> kernel with the whole deferred probing mess.  To me, there doesn't seem
>>>>> to be any other way to properly "know" this.
>>>>
>>>> As I said, DT *does* have this dependency information already. The
>>>> problem is the kernel probing doesn't use it. Fix that and then we can
>>>> discuss dependencies the DT doesn't provide that the kernel needs.
>>>
>>> Where can the kernel probing be fixed to use it?  What am I missing that
>>> can be done instead of what this patchset does?
>>
>> Somewhere, either in each subsystem or in the DT or core code creating
>> struct devices, you need to iterate thru the dependencies. Take clocks
>> as an example:
>>
>> for each node:
>>   for each 'clocks' phandle
>>     Lookup struct device from clock phandle
>>     Add the clock provider struct device to node's struct device links
>>
>> Now, repeat this for regulators, interrupts, etc.
> 
> I'm more than happy to do this if the maintainers can accept this as a
> solution, but then we need to agree that we need an override property
> if the implicit dependency isn't a mandatory dependency. We also need

Why is a dependency not mandatory?


> to agree on how we do this without breaking backwards compatibility.
> Either as a config option for this feature or have a property in the
> "chosen" node to say it's okay to assume existing bindings imply
> mandatory dependencies (it's just describing the DT more explicitly
> and the kernel will use it to enable this feature).

You lost me here.


> Although regulator binding are a "problem" because the kernel will
> have to parse every property in a node to check if it ends with
> -supply and then treat it as if it's a regulator binding (even though
> it might not be). So regulators will need some kind of "opt out" in
> the kernel (not DT).
> 
>> This series is pretty much doing the same thing, you just have to
>> parse each provider rather than only 'depends-on'.
>>
>> One issue is the struct device for the dependency may not be created
>> yet. I think this series would have the same issue, but haven't dug
>> into how it avoids that or whether it just ignores it and falls back
>> to deferring probe.
> 
> The patch series handles this properly and doesn't fall back to
> deferred probing.
> 
>> I'm also not clear on how you create struct devices and add
>> dependencies before probing gets attempted. If a driver is already
>> registered, probe is going to be attempted before any dependencies are
>> added. I guess the issue is avoided with drivers being modules, but
>> any solution should work for built-in too.
> 
> This is also handled properly in the series.
> 
> I've actually boot tested both these scenarios you call out and the
> patch series handles them properly.
> 
> But you are missing the main point here. The goal isn't to just
> eliminate deferred probing (it's a great side effect even it it just
> stops 99% of them), but also remove the bad assumption that
> late_initcall_sync() means all the devices are probed. The suppliers
> need a better signal (which the patch series provides) to tell when
> they can "unfreeze" the resources left on at boot.
> 
> It's true that device tree overlays can be added after userspace comes
> up, but in those cases whoever is adding the device tree nodes can make
> sure that the resources needed by the "to be added overlay devices" are
> kept at the right level. It's also unlikely that the bootloader is
> leaving resources
> on for these overlay devices because they might never be added.

Just like modules might never be added.

> 
> And even if it doesn't work perfectly for instances with overlays
> (neither does the
> current kernel), it's still better to fix it for the next
> million/billion devices that'll use
> ARM without post boot overlays.

Sorry, you do not get to ignore overlays.

> 
> Thanks,
> Saravana
>
Rob Herring June 12, 2019, 9:12 p.m. UTC | #17
On Wed, Jun 12, 2019 at 1:29 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jun 12, 2019 at 11:19 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> > > > On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > > > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Saravana,
> > > > > > > > >
> > > > > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > > > > comments on the original?
> > > > > > > > >
> > > > > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > > > > as they are not relevant until architecture issues are resolved.)
> > > > > > > >
> > > > > > > > Right, I'm not too worried about the implementation issues before we
> > > > > > > > settle on the architectural issues. Those are easy to fix.
> > > > > > > >
> > > > > > > > Honestly, the main points that the maintainers raised are:
> > > > > > > > 1) This is a configuration property and not describing the device.
> > > > > > > > Just use the implicit dependencies coming from existing bindings.
> > > > > > > >
> > > > > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > > > > property. But even if that's not something the maintainers can agree
> > > > > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > > > > of the devices from probing till the end of time. So even if the
> > > > > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > > > > decide the dependencies, we still need some means to override the
> > > > > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > > > > correct?
> > > > > > > >
> > > > > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > > > > >
> > > > > > > > I explained why this patch series is not just about optimizing probe
> > > > > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > > > > systems/archs too, but I can't speak for those).
> > > > > > > >
> > > > > > > > 3) Concerns about backwards compatibility
> > > > > > > >
> > > > > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > > > > "depends-on" as an override what we could do to keep backwards
> > > > > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > > > > >
> > > > > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > > > > functionalities but a limited driver.
> > > > > > >
> > > > > > > <snip>
> > > > > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > > > > load device drivers as modules for Android.
> > > > > > >
> > > > > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > > > > the drawing board" is the ask, could you please point us in the right
> > > > > > > direction?
> > > > > >
> > > > > > Use the dependencies which are already there in DT. That's clocks,
> > > > > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > > > > going to accept duplicating all those dependencies in DT. The downside
> > > > > > for the kernel is you have to address these one by one and can't have
> > > > > > a generic property the driver core code can parse. After that's in
> > > > > > place, then maybe we can consider handling any additional dependencies
> > > > > > not already captured in DT. Once all that is in place, we can probably
> > > > > > sort device and/or driver lists to optimize the probe order (maybe the
> > > > > > driver core already does that now?).
> > > > > >
> > > > > > Get rid of the auto disabling of clocks and regulators in
> > > > > > late_initcall. It's simply not a valid marker that boot is done when
> > > > > > modules are involved. We probably can't get rid of it as lot's of
> > > > > > platforms rely on that, so it will have to be opt out. Make it the
> > > > > > platform's responsibility for ensuring a consistent state.
> > > > > >
> > > > > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > > > > userspace in order to make progress if dependencies are missing.
> > > > >
> > > > > People have tried to do this multiple times, and you never really know
> > > > > when "boot is done" due to busses that have discoverable devices and
> > > > > async probing of other busses.
> > > >
> > > > Yes, I know which is why I proposed the second name with more limited
> > > > meaning/function.
> > >
> > > I still don't want to have the kernel have to rely on this.
> > >
> > > > > You do know "something" when you pivot to a new boot disk, and when you
> > > > > try to load init, but given initramfs and the fact that modules are
> > > > > usually included on them, that's not really a good indication that
> > > > > anything is "finished".
> > > > >
> > > > > I don't want userspace to be responsible for telling the kernel, "hey
> > > > > you should be finished now!", as that's an async notification that is
> > > > > going to be ripe for problems.
> > > >
> > > > The usecase I care about here is when the DT has the dependency
> > > > information, but the kernel doesn't have the driver and the dependency
> > > > is never resolved.
> > >
> > > Then we have the same situation as today and nothing different happens,
> > > right?
> >
> > Huh?
> >
> > This works today, but not for modules.
>
> Replying to this a bit further down.
>
> >
> > >
> > > > The same problem has to be solved with a
> > > > 'depends-on' property. This easily happens with a new DT with added
> > > > dependencies like pinctrl and an old kernel that doesn't have the
> > > > "new" driver.
>
> Isn't this the perfect example of an "implicit dependency" in a DT
> node not being a mandatory dependency? The old kernel worked fine with
> older DT without the added pinctrl dependency, so treating it as a
> mandatory dependency seems wrong for that particular device?
> depends-on avoids all this because the older kernel won't parse
> depends-on. And for newer kernels, it'll parse only what depends-on
> says are dependencies and not make wrong assumptions.

What happens when the older kernel is a kernel that parses
'depends-on'? What's 'older' is a moving target. We just need a
'depends-on-v5.2', 'depends-on-v5.3', etc.

It's not just 'old' kernels. Current kernels which didn't enable some
driver also have the same problem. All of this is mostly just a
development problem on incomplete platforms. But it is a real problem
that SUSE folks have raised.

> > > > Another example is IOMMUs. We need some way to say stop
> > > > waiting for dependencies. It is really just a debug option (of course,
> > > > how to prevent a debug option from being used in production?). This
> > > > works now for built-in cases with the same late_initcall abuse.
> > >
> > > What is a debug option?  We need something "for real".
> > >
> > > > Using late_initcall_sync as an indicator has all the same problems
> > > > with userspace indicating boot finished. We should get rid of the
> > > > late_initcall_sync abuses and stop trying to work around them.
> > >
> > > I agree, but that's not the issue here.
> >
> > It is because the cover letter mentions it and downstream work around it.
>
> This patch series is trying to remove the use of late_initcall_sync
> and instead relying on dependency information coming from DT. So, you
> are agreeing with the patchset.

That aspect, yes. It was Greg that said the late_initcall_sync abuses
were not the issue. 'depends-on' is the only thing I'm objecting to
ATM and I understand the problem (you're not the first to try). I've
not studied the the details of the patchset though beyond that.

> > > > > I really like the "depends-on" information, as it shows a topology that
> > > > > DT doesn't seem to be able to show today, yet we rely on it in the
> > > > > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > > > > to be any other way to properly "know" this.
> > > >
> > > > As I said, DT *does* have this dependency information already. The
> > > > problem is the kernel probing doesn't use it. Fix that and then we can
> > > > discuss dependencies the DT doesn't provide that the kernel needs.
> > >
> > > Where can the kernel probing be fixed to use it?  What am I missing that
> > > can be done instead of what this patchset does?
> >
> > Somewhere, either in each subsystem or in the DT or core code creating
> > struct devices, you need to iterate thru the dependencies. Take clocks
> > as an example:
> >
> > for each node:
> >   for each 'clocks' phandle
> >     Lookup struct device from clock phandle
> >     Add the clock provider struct device to node's struct device links
> >
> > Now, repeat this for regulators, interrupts, etc.
>
> I'm more than happy to do this if the maintainers can accept this as a
> solution, but then we need to agree that we need an override property
> if the implicit dependency isn't a mandatory dependency. We also need
> to agree on how we do this without breaking backwards compatibility.
> Either as a config option for this feature or have a property in the
> "chosen" node to say it's okay to assume existing bindings imply
> mandatory dependencies (it's just describing the DT more explicitly
> and the kernel will use it to enable this feature).

Thinking of the above example I gave, the problem is mandatory or not
can't be decided in the DT. It's a function of what the kernel
supports or not, so the kernel has to decide. With a 'depends-on'
property, the value you'd want depends on which kernel do you boot. If
I'm booting a kernel with the pinctrl driver, then pinctrl is a
mandatory dependency. If I'm booting a kernel without the pinctrl
driver, then pinctrl is not a 'depends-on' dependency. Ignoring
dependencies like this case only works when things are built-in and
only for specific subsystems where it makes sense. It would be nice to
solve it for modules too, but that can be done later.

There's also the case of dependencies where the driver decides how to
handle them. For example, UARTs will use DMA if the dmaengine driver
is available and that decision is (at least for some drivers I've
looked at) made on every open(). I suppose in that case, the driver
could remove the dependency link since it knows it can work with or
without DMA, though we'd need some sort of mechanism to do that.

> Although regulator binding are a "problem" because the kernel will
> have to parse every property in a node to check if it ends with
> -supply and then treat it as if it's a regulator binding (even though
> it might not be). So regulators will need some kind of "opt out" in
> the kernel (not DT).

Yes, that's unfortunate. GPIO is the same. You can safely assume that
'-supply' is a regulator. This is at least somewhat enforced by the
binding schema now.

We probably need a for_each_of_property_with_suffix() iterator for
these cases and with some pointer math on the property names or a
custom str compare function, it shouldn't be much slower to match.

> > This series is pretty much doing the same thing, you just have to
> > parse each provider rather than only 'depends-on'.
> >
> > One issue is the struct device for the dependency may not be created
> > yet. I think this series would have the same issue, but haven't dug
> > into how it avoids that or whether it just ignores it and falls back
> > to deferring probe.
>
> The patch series handles this properly and doesn't fall back to
> deferred probing.
>
> > I'm also not clear on how you create struct devices and add
> > dependencies before probing gets attempted. If a driver is already
> > registered, probe is going to be attempted before any dependencies are
> > added. I guess the issue is avoided with drivers being modules, but
> > any solution should work for built-in too.
>
> This is also handled properly in the series.
>
> I've actually boot tested both these scenarios you call out and the
> patch series handles them properly.

Okay, that's good.

> But you are missing the main point here. The goal isn't to just
> eliminate deferred probing (it's a great side effect even it it just
> stops 99% of them), but also remove the bad assumption that
> late_initcall_sync() means all the devices are probed. The suppliers
> need a better signal (which the patch series provides) to tell when
> they can "unfreeze" the resources left on at boot.

I understand this. I think I was clear on my opinion about using
late_initcall_sync.

Rob
Saravana Kannan June 12, 2019, 10:10 p.m. UTC | #18
On Wed, Jun 12, 2019 at 2:12 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Jun 12, 2019 at 1:29 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, Jun 12, 2019 at 11:19 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> > > > > On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > > > > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Saravana,
> > > > > > > > > >
> > > > > > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > > > > > comments on the original?
> > > > > > > > > >
> > > > > > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > > > > > as they are not relevant until architecture issues are resolved.)
> > > > > > > > >
> > > > > > > > > Right, I'm not too worried about the implementation issues before we
> > > > > > > > > settle on the architectural issues. Those are easy to fix.
> > > > > > > > >
> > > > > > > > > Honestly, the main points that the maintainers raised are:
> > > > > > > > > 1) This is a configuration property and not describing the device.
> > > > > > > > > Just use the implicit dependencies coming from existing bindings.
> > > > > > > > >
> > > > > > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > > > > > property. But even if that's not something the maintainers can agree
> > > > > > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > > > > > of the devices from probing till the end of time. So even if the
> > > > > > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > > > > > decide the dependencies, we still need some means to override the
> > > > > > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > > > > > correct?
> > > > > > > > >
> > > > > > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > > > > > >
> > > > > > > > > I explained why this patch series is not just about optimizing probe
> > > > > > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > > > > > systems/archs too, but I can't speak for those).
> > > > > > > > >
> > > > > > > > > 3) Concerns about backwards compatibility
> > > > > > > > >
> > > > > > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > > > > > "depends-on" as an override what we could do to keep backwards
> > > > > > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > > > > > >
> > > > > > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > > > > > functionalities but a limited driver.
> > > > > > > >
> > > > > > > > <snip>
> > > > > > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > > > > > load device drivers as modules for Android.
> > > > > > > >
> > > > > > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > > > > > the drawing board" is the ask, could you please point us in the right
> > > > > > > > direction?
> > > > > > >
> > > > > > > Use the dependencies which are already there in DT. That's clocks,
> > > > > > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > > > > > going to accept duplicating all those dependencies in DT. The downside
> > > > > > > for the kernel is you have to address these one by one and can't have
> > > > > > > a generic property the driver core code can parse. After that's in
> > > > > > > place, then maybe we can consider handling any additional dependencies
> > > > > > > not already captured in DT. Once all that is in place, we can probably
> > > > > > > sort device and/or driver lists to optimize the probe order (maybe the
> > > > > > > driver core already does that now?).
> > > > > > >
> > > > > > > Get rid of the auto disabling of clocks and regulators in
> > > > > > > late_initcall. It's simply not a valid marker that boot is done when
> > > > > > > modules are involved. We probably can't get rid of it as lot's of
> > > > > > > platforms rely on that, so it will have to be opt out. Make it the
> > > > > > > platform's responsibility for ensuring a consistent state.
> > > > > > >
> > > > > > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > > > > > userspace in order to make progress if dependencies are missing.
> > > > > >
> > > > > > People have tried to do this multiple times, and you never really know
> > > > > > when "boot is done" due to busses that have discoverable devices and
> > > > > > async probing of other busses.
> > > > >
> > > > > Yes, I know which is why I proposed the second name with more limited
> > > > > meaning/function.
> > > >
> > > > I still don't want to have the kernel have to rely on this.
> > > >
> > > > > > You do know "something" when you pivot to a new boot disk, and when you
> > > > > > try to load init, but given initramfs and the fact that modules are
> > > > > > usually included on them, that's not really a good indication that
> > > > > > anything is "finished".
> > > > > >
> > > > > > I don't want userspace to be responsible for telling the kernel, "hey
> > > > > > you should be finished now!", as that's an async notification that is
> > > > > > going to be ripe for problems.
> > > > >
> > > > > The usecase I care about here is when the DT has the dependency
> > > > > information, but the kernel doesn't have the driver and the dependency
> > > > > is never resolved.
> > > >
> > > > Then we have the same situation as today and nothing different happens,
> > > > right?
> > >
> > > Huh?
> > >
> > > This works today, but not for modules.
> >
> > Replying to this a bit further down.
> >
> > >
> > > >
> > > > > The same problem has to be solved with a
> > > > > 'depends-on' property. This easily happens with a new DT with added
> > > > > dependencies like pinctrl and an old kernel that doesn't have the
> > > > > "new" driver.
> >
> > Isn't this the perfect example of an "implicit dependency" in a DT
> > node not being a mandatory dependency? The old kernel worked fine with
> > older DT without the added pinctrl dependency, so treating it as a
> > mandatory dependency seems wrong for that particular device?
> > depends-on avoids all this because the older kernel won't parse
> > depends-on. And for newer kernels, it'll parse only what depends-on
> > says are dependencies and not make wrong assumptions.
>
> What happens when the older kernel is a kernel that parses
> 'depends-on'? What's 'older' is a moving target. We just need a
> 'depends-on-v5.2', 'depends-on-v5.3', etc.
> It's not just 'old' kernels. Current kernels which didn't enable some
> driver also have the same problem. All of this is mostly just a
> development problem on incomplete platforms. But it is a real problem
> that SUSE folks have raised.
> > > > > Another example is IOMMUs. We need some way to say stop
> > > > > waiting for dependencies. It is really just a debug option (of course,
> > > > > how to prevent a debug option from being used in production?). This
> > > > > works now for built-in cases with the same late_initcall abuse.
> > > >
> > > > What is a debug option?  We need something "for real".
> > > >
> > > > > Using late_initcall_sync as an indicator has all the same problems
> > > > > with userspace indicating boot finished. We should get rid of the
> > > > > late_initcall_sync abuses and stop trying to work around them.
> > > >
> > > > I agree, but that's not the issue here.
> > >
> > > It is because the cover letter mentions it and downstream work around it.
> >
> > This patch series is trying to remove the use of late_initcall_sync
> > and instead relying on dependency information coming from DT. So, you
> > are agreeing with the patchset.
>
> That aspect, yes. It was Greg that said the late_initcall_sync abuses
> were not the issue. 'depends-on' is the only thing I'm objecting to
> ATM and I understand the problem (you're not the first to try). I've
> not studied the the details of the patchset though beyond that.
>
> > > > > > I really like the "depends-on" information, as it shows a topology that
> > > > > > DT doesn't seem to be able to show today, yet we rely on it in the
> > > > > > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > > > > > to be any other way to properly "know" this.
> > > > >
> > > > > As I said, DT *does* have this dependency information already. The
> > > > > problem is the kernel probing doesn't use it. Fix that and then we can
> > > > > discuss dependencies the DT doesn't provide that the kernel needs.
> > > >
> > > > Where can the kernel probing be fixed to use it?  What am I missing that
> > > > can be done instead of what this patchset does?
> > >
> > > Somewhere, either in each subsystem or in the DT or core code creating
> > > struct devices, you need to iterate thru the dependencies. Take clocks
> > > as an example:
> > >
> > > for each node:
> > >   for each 'clocks' phandle
> > >     Lookup struct device from clock phandle
> > >     Add the clock provider struct device to node's struct device links
> > >
> > > Now, repeat this for regulators, interrupts, etc.
> >
> > I'm more than happy to do this if the maintainers can accept this as a
> > solution, but then we need to agree that we need an override property

Can you please respond to this? For the clock controller example, we
need to explicitly override the implied dependency because... see
subsequent comments below.
Can we at least agree to using "depends-on" as an override when the
existing bindings don't properly define the mandatory dependencies? If
it'll make you happy, we can even make it "does-not-depend-on".

> > if the implicit dependency isn't a mandatory dependency. We also need
> > to agree on how we do this without breaking backwards compatibility.
> > Either as a config option for this feature or have a property in the
> > "chosen" node to say it's okay to assume existing bindings imply
> > mandatory dependencies (it's just describing the DT more explicitly
> > and the kernel will use it to enable this feature).
>
> Thinking of the above example I gave, the problem is mandatory or not
> can't be decided in the DT. It's a function of what the kernel
> supports or not, so the kernel has to decide.

That might be debatable for the example you gave, but it's certainly
not debatable for some clock controllers (CC) that have cyclic
dependencies. Only one of them can function without the other.

> With a 'depends-on'
> property, the value you'd want depends on which kernel do you boot.

In the cases I'm thinking of, this has nothing to do with the kernel.
CC1 supplies 50 clocks, 5 of which need input from CC2. CC2 supplies
10 clocks, all of them are dependent on 4 inputs from CC1. CC2 can
never function without CC1. But in DT, they'll have a cyclic
dependency if you go by clock bindings they list.

> If
> I'm booting a kernel with the pinctrl driver, then pinctrl is a
> mandatory dependency. If I'm booting a kernel without the pinctrl
> driver, then pinctrl is not a 'depends-on' dependency.

The way I define depends-on, pinctrl will never be a dependency for
this specific device (because you are saying it can function without
it). The driver decides if it wants to -EPROBE_DEFER or not depending
on the kernel. But I've beaten this horse to death. So, moving
along...

> Ignoring
> dependencies like this case only works when things are built-in and
> only for specific subsystems where it makes sense. It would be nice to
> solve it for modules too, but that can be done later.
>
> There's also the case of dependencies where the driver decides how to
> handle them. For example, UARTs will use DMA if the dmaengine driver
> is available and that decision is (at least for some drivers I've
> looked at) made on every open(). I suppose in that case, the driver
> could remove the dependency link since it knows it can work with or
> without DMA, though we'd need some sort of mechanism to do that.

Now you are just going back to listing the examples I gave for why we
need "depends-on" and why existing bindings aren't mandatory
dependencies. Don't know if I should laugh or cry :)

-Saravana

> > Although regulator binding are a "problem" because the kernel will
> > have to parse every property in a node to check if it ends with
> > -supply and then treat it as if it's a regulator binding (even though
> > it might not be). So regulators will need some kind of "opt out" in
> > the kernel (not DT).
>
> Yes, that's unfortunate. GPIO is the same. You can safely assume that
> '-supply' is a regulator. This is at least somewhat enforced by the
> binding schema now.
>
> We probably need a for_each_of_property_with_suffix() iterator for
> these cases and with some pointer math on the property names or a
> custom str compare function, it shouldn't be much slower to match.
>
> > > This series is pretty much doing the same thing, you just have to
> > > parse each provider rather than only 'depends-on'.
> > >
> > > One issue is the struct device for the dependency may not be created
> > > yet. I think this series would have the same issue, but haven't dug
> > > into how it avoids that or whether it just ignores it and falls back
> > > to deferring probe.
> >
> > The patch series handles this properly and doesn't fall back to
> > deferred probing.
> >
> > > I'm also not clear on how you create struct devices and add
> > > dependencies before probing gets attempted. If a driver is already
> > > registered, probe is going to be attempted before any dependencies are
> > > added. I guess the issue is avoided with drivers being modules, but
> > > any solution should work for built-in too.
> >
> > This is also handled properly in the series.
> >
> > I've actually boot tested both these scenarios you call out and the
> > patch series handles them properly.
>
> Okay, that's good.
>
> > But you are missing the main point here. The goal isn't to just
> > eliminate deferred probing (it's a great side effect even it it just
> > stops 99% of them), but also remove the bad assumption that
> > late_initcall_sync() means all the devices are probed. The suppliers
> > need a better signal (which the patch series provides) to tell when
> > they can "unfreeze" the resources left on at boot.
>
> I understand this. I think I was clear on my opinion about using
> late_initcall_sync.
>
> Rob
Sandeep Patil June 18, 2019, 8:47 p.m. UTC | #19
On Wed, Jun 12, 2019 at 12:29:18PM -0700, 'Saravana Kannan' via kernel-team wrote:
> On Wed, Jun 12, 2019 at 11:19 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> > > > On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > > > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Saravana,
> > > > > > > > >
> > > > > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > > > > comments on the original?
> > > > > > > > >
> > > > > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > > > > as they are not relevant until architecture issues are resolved.)
> > > > > > > >
> > > > > > > > Right, I'm not too worried about the implementation issues before we
> > > > > > > > settle on the architectural issues. Those are easy to fix.
> > > > > > > >
> > > > > > > > Honestly, the main points that the maintainers raised are:
> > > > > > > > 1) This is a configuration property and not describing the device.
> > > > > > > > Just use the implicit dependencies coming from existing bindings.
> > > > > > > >
> > > > > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > > > > property. But even if that's not something the maintainers can agree
> > > > > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > > > > of the devices from probing till the end of time. So even if the
> > > > > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > > > > decide the dependencies, we still need some means to override the
> > > > > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > > > > correct?
> > > > > > > >
> > > > > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > > > > >
> > > > > > > > I explained why this patch series is not just about optimizing probe
> > > > > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > > > > systems/archs too, but I can't speak for those).
> > > > > > > >
> > > > > > > > 3) Concerns about backwards compatibility
> > > > > > > >
> > > > > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > > > > "depends-on" as an override what we could do to keep backwards
> > > > > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > > > > >
> > > > > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > > > > functionalities but a limited driver.
> > > > > > >
> > > > > > > <snip>
> > > > > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > > > > load device drivers as modules for Android.
> > > > > > >
> > > > > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > > > > the drawing board" is the ask, could you please point us in the right
> > > > > > > direction?
> > > > > >
> > > > > > Use the dependencies which are already there in DT. That's clocks,
> > > > > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > > > > going to accept duplicating all those dependencies in DT. The downside
> > > > > > for the kernel is you have to address these one by one and can't have
> > > > > > a generic property the driver core code can parse. After that's in
> > > > > > place, then maybe we can consider handling any additional dependencies
> > > > > > not already captured in DT. Once all that is in place, we can probably
> > > > > > sort device and/or driver lists to optimize the probe order (maybe the
> > > > > > driver core already does that now?).
> > > > > >
> > > > > > Get rid of the auto disabling of clocks and regulators in
> > > > > > late_initcall. It's simply not a valid marker that boot is done when
> > > > > > modules are involved. We probably can't get rid of it as lot's of
> > > > > > platforms rely on that, so it will have to be opt out. Make it the
> > > > > > platform's responsibility for ensuring a consistent state.
> > > > > >
> > > > > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > > > > userspace in order to make progress if dependencies are missing.
> > > > >
> > > > > People have tried to do this multiple times, and you never really know
> > > > > when "boot is done" due to busses that have discoverable devices and
> > > > > async probing of other busses.
> > > >
> > > > Yes, I know which is why I proposed the second name with more limited
> > > > meaning/function.
> > >
> > > I still don't want to have the kernel have to rely on this.
> > >
> > > > > You do know "something" when you pivot to a new boot disk, and when you
> > > > > try to load init, but given initramfs and the fact that modules are
> > > > > usually included on them, that's not really a good indication that
> > > > > anything is "finished".
> > > > >
> > > > > I don't want userspace to be responsible for telling the kernel, "hey
> > > > > you should be finished now!", as that's an async notification that is
> > > > > going to be ripe for problems.
> > > >
> > > > The usecase I care about here is when the DT has the dependency
> > > > information, but the kernel doesn't have the driver and the dependency
> > > > is never resolved.
> > >
> > > Then we have the same situation as today and nothing different happens,
> > > right?
> >
> > Huh?
> >
> > This works today, but not for modules.
> 
> Replying to this a bit further down.
> 
> >
> > >
> > > > The same problem has to be solved with a
> > > > 'depends-on' property. This easily happens with a new DT with added
> > > > dependencies like pinctrl and an old kernel that doesn't have the
> > > > "new" driver.
> 
> Isn't this the perfect example of an "implicit dependency" in a DT
> node not being a mandatory dependency? The old kernel worked fine with
> older DT without the added pinctrl dependency, so treating it as a
> mandatory dependency seems wrong for that particular device?
> depends-on avoids all this because the older kernel won't parse
> depends-on. And for newer kernels, it'll parse only what depends-on
> says are dependencies and not make wrong assumptions.
> 
> > > > Another example is IOMMUs. We need some way to say stop
> > > > waiting for dependencies. It is really just a debug option (of course,
> > > > how to prevent a debug option from being used in production?). This
> > > > works now for built-in cases with the same late_initcall abuse.
> > >
> > > What is a debug option?  We need something "for real".
> > >
> > > > Using late_initcall_sync as an indicator has all the same problems
> > > > with userspace indicating boot finished. We should get rid of the
> > > > late_initcall_sync abuses and stop trying to work around them.
> > >
> > > I agree, but that's not the issue here.
> >
> > It is because the cover letter mentions it and downstream work around it.
> 
> This patch series is trying to remove the use of late_initcall_sync
> and instead relying on dependency information coming from DT. So, you
> are agreeing with the patchset.
> 
> > > > > I really like the "depends-on" information, as it shows a topology that
> > > > > DT doesn't seem to be able to show today, yet we rely on it in the
> > > > > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > > > > to be any other way to properly "know" this.
> > > >
> > > > As I said, DT *does* have this dependency information already. The
> > > > problem is the kernel probing doesn't use it. Fix that and then we can
> > > > discuss dependencies the DT doesn't provide that the kernel needs.
> > >
> > > Where can the kernel probing be fixed to use it?  What am I missing that
> > > can be done instead of what this patchset does?
> >
> > Somewhere, either in each subsystem or in the DT or core code creating
> > struct devices, you need to iterate thru the dependencies. Take clocks
> > as an example:
> >
> > for each node:
> >   for each 'clocks' phandle
> >     Lookup struct device from clock phandle
> >     Add the clock provider struct device to node's struct device links
> >
> > Now, repeat this for regulators, interrupts, etc.
> 
> I'm more than happy to do this if the maintainers can accept this as a
> solution, but then we need to agree that we need an override property
> if the implicit dependency isn't a mandatory dependency.

I don't quite understand what you mean by "isn't a mandatory dependency"
here. I think IIUC, what Rob said will solve the probe order problem,
correct?

Is there a problem if we split this in two and handle the
late_initcall_sync() + regulators separately and solve the probe ordering
here as suggested above?

I know the original intention of the series is to resolve the
late_initcall_sync() assumption and probe order was a "side-effect". However,
I think probing in the dependency order is still extremely valuable and can
resolve boot time issues ahead of time.

> We also need
> to agree on how we do this without breaking backwards compatibility.
> Either as a config option for this feature or have a property in the
> "chosen" node to say it's okay to assume existing bindings imply
> mandatory dependencies (it's just describing the DT more explicitly
> and the kernel will use it to enable this feature).
> 
> Although regulator binding are a "problem" because the kernel will
> have to parse every property in a node to check if it ends with
> -supply and then treat it as if it's a regulator binding (even though
> it might not be). So regulators will need some kind of "opt out" in
> the kernel (not DT).

Agree and it is going to immediately conflict with 'power-supply' for
example. If we are going this route, then we need to fix and agree on
standard regulator bindings too and make the changes everywhere in the
kernel.

> 
> > This series is pretty much doing the same thing, you just have to
> > parse each provider rather than only 'depends-on'.
> >
> > One issue is the struct device for the dependency may not be created
> > yet. I think this series would have the same issue, but haven't dug
> > into how it avoids that or whether it just ignores it and falls back
> > to deferring probe.
> 
> The patch series handles this properly and doesn't fall back to
> deferred probing.
> 
> > I'm also not clear on how you create struct devices and add
> > dependencies before probing gets attempted. If a driver is already
> > registered, probe is going to be attempted before any dependencies are
> > added. I guess the issue is avoided with drivers being modules, but
> > any solution should work for built-in too.
> 
> This is also handled properly in the series.
> 
> I've actually boot tested both these scenarios you call out and the
> patch series handles them properly.
> 
> But you are missing the main point here. The goal isn't to just
> eliminate deferred probing (it's a great side effect even it it just
> stops 99% of them), but also remove the bad assumption that
> late_initcall_sync() means all the devices are probed. The suppliers
> need a better signal (which the patch series provides) to tell when
> they can "unfreeze" the resources left on at boot.
> 

Is the summary here that we need to figure out a different approach / fix
regulator framework, or something else ? It wasn't clear from all other
emails from this thread, sorry for noise if I missed it.

- ssp
Saravana Kannan June 18, 2019, 9:22 p.m. UTC | #20
On Tue, Jun 18, 2019 at 1:47 PM Sandeep Patil <sspatil@android.com> wrote:
>
> On Wed, Jun 12, 2019 at 12:29:18PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > On Wed, Jun 12, 2019 at 11:19 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Jun 12, 2019 at 10:53:09AM -0600, Rob Herring wrote:
> > > > > On Wed, Jun 12, 2019 at 8:22 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> > > > > > > On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil <sspatil@android.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jun 11, 2019 at 01:56:25PM -0700, 'Saravana Kannan' via kernel-team wrote:
> > > > > > > > > On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand <frowand.list@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Saravana,
> > > > > > > > > >
> > > > > > > > > > On 6/10/19 10:36 AM, Rob Herring wrote:
> > > > > > > > > > > Why are you resending this rather than replying to Frank's last
> > > > > > > > > > > comments on the original?
> > > > > > > > > >
> > > > > > > > > > Adding on a different aspect...  The independent replies from three different
> > > > > > > > > > maintainers (Rob, Mark, myself) pointed out architectural issues with the
> > > > > > > > > > patch series.  There were also some implementation issues brought out.
> > > > > > > > > > (Although I refrained from bringing up most of my implementation issues
> > > > > > > > > > as they are not relevant until architecture issues are resolved.)
> > > > > > > > >
> > > > > > > > > Right, I'm not too worried about the implementation issues before we
> > > > > > > > > settle on the architectural issues. Those are easy to fix.
> > > > > > > > >
> > > > > > > > > Honestly, the main points that the maintainers raised are:
> > > > > > > > > 1) This is a configuration property and not describing the device.
> > > > > > > > > Just use the implicit dependencies coming from existing bindings.
> > > > > > > > >
> > > > > > > > > I gave a bunch of reasons for why I think it isn't an OS configuration
> > > > > > > > > property. But even if that's not something the maintainers can agree
> > > > > > > > > to, I gave a concrete example (cyclic dependencies between clock
> > > > > > > > > provider hardware) where the implicit dependencies would prevent one
> > > > > > > > > of the devices from probing till the end of time. So even if the
> > > > > > > > > maintainers don't agree we should always look at "depends-on" to
> > > > > > > > > decide the dependencies, we still need some means to override the
> > > > > > > > > implicit dependencies where they don't match the real dependency. Can
> > > > > > > > > we use depends-on as an override when the implicit dependencies aren't
> > > > > > > > > correct?
> > > > > > > > >
> > > > > > > > > 2) This doesn't need to be solved because this is just optimizing
> > > > > > > > > probing or saving power ("we should get rid of this auto disabling"):
> > > > > > > > >
> > > > > > > > > I explained why this patch series is not just about optimizing probe
> > > > > > > > > ordering or saving power. And why we can't ignore auto disabling
> > > > > > > > > (because it's more than just auto disabling). The kernel is currently
> > > > > > > > > broken when trying to use modules in ARM SoCs (probably in other
> > > > > > > > > systems/archs too, but I can't speak for those).
> > > > > > > > >
> > > > > > > > > 3) Concerns about backwards compatibility
> > > > > > > > >
> > > > > > > > > I pointed out why the current scheme (depends-on being the only source
> > > > > > > > > of dependency) doesn't break compatibility. And if we go with
> > > > > > > > > "depends-on" as an override what we could do to keep backwards
> > > > > > > > > compatibility. Happy to hear more thoughts or discuss options.
> > > > > > > > >
> > > > > > > > > 4) How the "sync_state" would work for a device that supplies multiple
> > > > > > > > > functionalities but a limited driver.
> > > > > > > >
> > > > > > > > <snip>
> > > > > > > > To be clear, all of above are _real_ problems that stops us from efficiently
> > > > > > > > load device drivers as modules for Android.
> > > > > > > >
> > > > > > > > So, if 'depends-on' doesn't seem like the right approach and "going back to
> > > > > > > > the drawing board" is the ask, could you please point us in the right
> > > > > > > > direction?
> > > > > > >
> > > > > > > Use the dependencies which are already there in DT. That's clocks,
> > > > > > > pinctrl, regulators, interrupts, gpio at a minimum. I'm simply not
> > > > > > > going to accept duplicating all those dependencies in DT. The downside
> > > > > > > for the kernel is you have to address these one by one and can't have
> > > > > > > a generic property the driver core code can parse. After that's in
> > > > > > > place, then maybe we can consider handling any additional dependencies
> > > > > > > not already captured in DT. Once all that is in place, we can probably
> > > > > > > sort device and/or driver lists to optimize the probe order (maybe the
> > > > > > > driver core already does that now?).
> > > > > > >
> > > > > > > Get rid of the auto disabling of clocks and regulators in
> > > > > > > late_initcall. It's simply not a valid marker that boot is done when
> > > > > > > modules are involved. We probably can't get rid of it as lot's of
> > > > > > > platforms rely on that, so it will have to be opt out. Make it the
> > > > > > > platform's responsibility for ensuring a consistent state.
> > > > > > >
> > > > > > > Perhaps we need a 'boot done' or 'stop deferring probe' trigger from
> > > > > > > userspace in order to make progress if dependencies are missing.
> > > > > >
> > > > > > People have tried to do this multiple times, and you never really know
> > > > > > when "boot is done" due to busses that have discoverable devices and
> > > > > > async probing of other busses.
> > > > >
> > > > > Yes, I know which is why I proposed the second name with more limited
> > > > > meaning/function.
> > > >
> > > > I still don't want to have the kernel have to rely on this.
> > > >
> > > > > > You do know "something" when you pivot to a new boot disk, and when you
> > > > > > try to load init, but given initramfs and the fact that modules are
> > > > > > usually included on them, that's not really a good indication that
> > > > > > anything is "finished".
> > > > > >
> > > > > > I don't want userspace to be responsible for telling the kernel, "hey
> > > > > > you should be finished now!", as that's an async notification that is
> > > > > > going to be ripe for problems.
> > > > >
> > > > > The usecase I care about here is when the DT has the dependency
> > > > > information, but the kernel doesn't have the driver and the dependency
> > > > > is never resolved.
> > > >
> > > > Then we have the same situation as today and nothing different happens,
> > > > right?
> > >
> > > Huh?
> > >
> > > This works today, but not for modules.
> >
> > Replying to this a bit further down.
> >
> > >
> > > >
> > > > > The same problem has to be solved with a
> > > > > 'depends-on' property. This easily happens with a new DT with added
> > > > > dependencies like pinctrl and an old kernel that doesn't have the
> > > > > "new" driver.
> >
> > Isn't this the perfect example of an "implicit dependency" in a DT
> > node not being a mandatory dependency? The old kernel worked fine with
> > older DT without the added pinctrl dependency, so treating it as a
> > mandatory dependency seems wrong for that particular device?
> > depends-on avoids all this because the older kernel won't parse
> > depends-on. And for newer kernels, it'll parse only what depends-on
> > says are dependencies and not make wrong assumptions.
> >
> > > > > Another example is IOMMUs. We need some way to say stop
> > > > > waiting for dependencies. It is really just a debug option (of course,
> > > > > how to prevent a debug option from being used in production?). This
> > > > > works now for built-in cases with the same late_initcall abuse.
> > > >
> > > > What is a debug option?  We need something "for real".
> > > >
> > > > > Using late_initcall_sync as an indicator has all the same problems
> > > > > with userspace indicating boot finished. We should get rid of the
> > > > > late_initcall_sync abuses and stop trying to work around them.
> > > >
> > > > I agree, but that's not the issue here.
> > >
> > > It is because the cover letter mentions it and downstream work around it.
> >
> > This patch series is trying to remove the use of late_initcall_sync
> > and instead relying on dependency information coming from DT. So, you
> > are agreeing with the patchset.
> >
> > > > > > I really like the "depends-on" information, as it shows a topology that
> > > > > > DT doesn't seem to be able to show today, yet we rely on it in the
> > > > > > kernel with the whole deferred probing mess.  To me, there doesn't seem
> > > > > > to be any other way to properly "know" this.
> > > > >
> > > > > As I said, DT *does* have this dependency information already. The
> > > > > problem is the kernel probing doesn't use it. Fix that and then we can
> > > > > discuss dependencies the DT doesn't provide that the kernel needs.
> > > >
> > > > Where can the kernel probing be fixed to use it?  What am I missing that
> > > > can be done instead of what this patchset does?
> > >
> > > Somewhere, either in each subsystem or in the DT or core code creating
> > > struct devices, you need to iterate thru the dependencies. Take clocks
> > > as an example:
> > >
> > > for each node:
> > >   for each 'clocks' phandle
> > >     Lookup struct device from clock phandle
> > >     Add the clock provider struct device to node's struct device links
> > >
> > > Now, repeat this for regulators, interrupts, etc.
> >
> > I'm more than happy to do this if the maintainers can accept this as a
> > solution, but then we need to agree that we need an override property
> > if the implicit dependency isn't a mandatory dependency.
>
> I don't quite understand what you mean by "isn't a mandatory dependency"
> here.

A binding to a clock, regulator, etc without which the device could
still operate. Best example is clock providers with cyclic
dependencies. One of them definitely can operate without the other
(but the reverse is not true).

> I think IIUC, what Rob said will solve the probe order problem,
> correct?

No. I'll repeat my statement from earlier:
I'm more than happy to do this if the maintainers can accept this as a
solution, but then we need to agree that we need an override property
if the implicit dependency isn't a mandatory dependency. Clock
providers are just one good example case of devices with cyclic
dependencies. We need to at least have depends-on act as a way to
override implicit dependencies when they don't match the real
dependency.

> Is there a problem if we split this in two and handle the
> late_initcall_sync() + regulators separately and solve the probe ordering
> here as suggested above?

Based on his earlier email, my understanding is that he doesn't care
about the probe ordering being inefficient because he doesn't expect
it to add much to boot time and sees that as an OS issue and not a DT
issue. I believe this specific proposal from his is to explain why we
don't need a "depends-on" property (and I'm saying it's only partly
correct).

> I know the original intention of the series is to resolve the
> late_initcall_sync() assumption and probe order was a "side-effect". However,
> I think probing in the dependency order is still extremely valuable and can
> resolve boot time issues ahead of time.
>
> > We also need
> > to agree on how we do this without breaking backwards compatibility.
> > Either as a config option for this feature or have a property in the
> > "chosen" node to say it's okay to assume existing bindings imply
> > mandatory dependencies (it's just describing the DT more explicitly
> > and the kernel will use it to enable this feature).
> >
> > Although regulator binding are a "problem" because the kernel will
> > have to parse every property in a node to check if it ends with
> > -supply and then treat it as if it's a regulator binding (even though
> > it might not be). So regulators will need some kind of "opt out" in
> > the kernel (not DT).
>
> Agree and it is going to immediately conflict with 'power-supply' for
> example. If we are going this route, then we need to fix and agree on
> standard regulator bindings too and make the changes everywhere in the
> kernel.
>
> >
> > > This series is pretty much doing the same thing, you just have to
> > > parse each provider rather than only 'depends-on'.
> > >
> > > One issue is the struct device for the dependency may not be created
> > > yet. I think this series would have the same issue, but haven't dug
> > > into how it avoids that or whether it just ignores it and falls back
> > > to deferring probe.
> >
> > The patch series handles this properly and doesn't fall back to
> > deferred probing.
> >
> > > I'm also not clear on how you create struct devices and add
> > > dependencies before probing gets attempted. If a driver is already
> > > registered, probe is going to be attempted before any dependencies are
> > > added. I guess the issue is avoided with drivers being modules, but
> > > any solution should work for built-in too.
> >
> > This is also handled properly in the series.
> >
> > I've actually boot tested both these scenarios you call out and the
> > patch series handles them properly.
> >
> > But you are missing the main point here. The goal isn't to just
> > eliminate deferred probing (it's a great side effect even it it just
> > stops 99% of them), but also remove the bad assumption that
> > late_initcall_sync() means all the devices are probed. The suppliers
> > need a better signal (which the patch series provides) to tell when
> > they can "unfreeze" the resources left on at boot.
> >
>
> Is the summary here that we need to figure out a different approach / fix
> regulator framework, or something else ? It wasn't clear from all other
> emails from this thread, sorry for noise if I missed it.

No, the comment about regulators was just about how unfriendly that
binding is for parsing dependencies. That has nothing to do with the
general design though (which is, do we need depends-on?).

-Saravana

Patch
diff mbox series

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 04ad312fd85b..1115a8d80a33 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -42,6 +42,8 @@  static int of_dev_node_match(struct device *dev, void *data)
 	return dev->of_node == data;
 }
 
+static DEFINE_SPINLOCK(of_dev_lock);
+
 /**
  * of_find_device_by_node - Find the platform_device associated with a node
  * @np: Pointer to device tree node
@@ -55,7 +57,18 @@  struct platform_device *of_find_device_by_node(struct device_node *np)
 {
 	struct device *dev;
 
-	dev = bus_find_device(&platform_bus_type, NULL, np, of_dev_node_match);
+	/*
+	 * Spinlock needed to make sure np->dev doesn't get freed between NULL
+	 * check inside and kref count increment inside get_device(). This is
+	 * achieved by grabbing the spinlock before setting np->dev = NULL in
+	 * of_platform_device_destroy().
+	 */
+	spin_lock(&of_dev_lock);
+	dev = get_device(np->dev);
+	spin_unlock(&of_dev_lock);
+	if (!dev)
+		dev = bus_find_device(&platform_bus_type, NULL, np,
+				      of_dev_node_match);
 	return dev ? to_platform_device(dev) : NULL;
 }
 EXPORT_SYMBOL(of_find_device_by_node);
@@ -196,6 +209,7 @@  static struct platform_device *of_platform_device_create_pdata(
 		platform_device_put(dev);
 		goto err_clear_flag;
 	}
+	np->dev = &dev->dev;
 
 	return dev;
 
@@ -556,6 +570,10 @@  int of_platform_device_destroy(struct device *dev, void *data)
 	if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
 		device_for_each_child(dev, NULL, of_platform_device_destroy);
 
+	/* Spinlock is needed for of_find_device_by_node() to work */
+	spin_lock(&of_dev_lock);
+	dev->of_node->dev = NULL;
+	spin_unlock(&of_dev_lock);
 	of_node_clear_flag(dev->of_node, OF_POPULATED);
 	of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
 
diff --git a/include/linux/of.h b/include/linux/of.h
index 0cf857012f11..f2b4912cbca1 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -48,6 +48,8 @@  struct property {
 struct of_irq_controller;
 #endif
 
+struct device;
+
 struct device_node {
 	const char *name;
 	phandle phandle;
@@ -68,6 +70,7 @@  struct device_node {
 	unsigned int unique_id;
 	struct of_irq_controller *irq_trans;
 #endif
+	struct device *dev;		/* Device created from this node */
 };
 
 #define MAX_PHANDLE_ARGS 16