Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Tue, Jun 18, 2019 at 1:47 PM Sandeep Patil 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 wrote:
> > >
> > > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > > 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
> > > > > 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
> > > > > > > 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
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > >
> > > > > > > > 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
> > > > > > > >
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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 wrote:
> >
> > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > 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
> > > > 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
> > > > > > 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
> > > > > > > > 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.
> > > > > > >
> > > > > > >
> > > > > > > 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Wed, Jun 12, 2019 at 2:12 PM Rob Herring wrote:
>
> On Wed, Jun 12, 2019 at 1:29 PM Saravana Kannan wrote:
> >
> > On Wed, Jun 12, 2019 at 11:19 AM Rob Herring wrote:
> > >
> > > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > > 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
> > > > > 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
> > > > > > > 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
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > >
> > > > > > > > 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?
> > > > > > >
> >
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Wed, Jun 12, 2019 at 1:29 PM Saravana Kannan wrote:
>
> On Wed, Jun 12, 2019 at 11:19 AM Rob Herring wrote:
> >
> > On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> > 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
> > > > 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
> > > > > > 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
> > > > > > > > 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.
> > > > > > >
> > > > > > >
> > > > > > > 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
> > >
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On 6/12/19 12:29 PM, Saravana Kannan wrote:
> On Wed, Jun 12, 2019 at 11:19 AM Rob Herring wrote:
>>
>> On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
>> 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
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
>> 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
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.
>>>
>>>
>>> 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Wed, Jun 12, 2019 at 11:19 AM Rob Herring wrote:
>
> On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
> 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
> > > 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
> > > > > 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
> > > > > > > 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.
> > > > > >
> > > > > >
> > > > > > 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On 6/12/19 6:53 AM, Rob Herring wrote:
> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil 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 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.
>>
>>
>> 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
> .
>
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Wed, Jun 12, 2019 at 11:08 AM Greg Kroah-Hartman
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
> > 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
> > > > 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
> > > > > > 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.
> > > > >
> > > > >
> > > > > 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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
> 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 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
> > > > > 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.
> > > >
> > > >
> > > > 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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 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
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.
>>>
>>>
>>> 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Wed, Jun 12, 2019 at 8:22 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 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
> > > > 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.
> > >
> > >
> > > 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
>
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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 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
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.
>>>
>>>
>>> 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
>
>>
>>
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On 6/12/19 6:53 AM, Rob Herring wrote:
> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil 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 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.
>>
>>
>> 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Wed, Jun 12, 2019 at 07:53:39AM -0600, Rob Herring wrote:
> On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil 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
> > > 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.
> >
> >
> > 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Tue, Jun 11, 2019 at 3:52 PM Sandeep Patil 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 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.
>
>
> 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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 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.
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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Tue, Jun 11, 2019 at 8:18 AM Frank Rowand 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_X
b) Can have a kernel commandline
c) NEW alternative to CONFIG_ 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 wrote:
> >>
> >> Add a pointer from device
Re: [PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Tue, Jun 11, 2019 at 7:51 AM Frank Rowand wrote:
>
> Hi Saravana,
>
> (I notice that I never seem to spell your name correctly. Apologies for that,
> both past and future).
Thanks for noticing :) One trick that might help with remembering my
name is that every other letter is an "a" :)
>
> This email was never answered.
Yeah, because this patch wasn't central to the functionality. At worse
case, I drop the patch and modules would still work. While waiting for
responses for the other emails -- I was working on measuring the speed
up.
If I just took the clock bindings (in a final solution we'll have to
look up clocks, resets, regulators, interrupts, etc) in a SDM845 and
made them into device links, that resulted in ~500+ searches for
devices by their nodes.
Looking at all 500+ of the lookups:
Total time:
With patch, : 2 milliseconds
Without patch: 12 milliseconds
Worst case single look up:
With patch: 39 microseconds
Without patch: 250 microseconds
Median of lookup times:
With patch: 208 nanoseconds
Without patch: 23 microseconds
Even if I assume there are 1000 device nodes, the increase in memory
from one additional pointer this patch adds is ~8KB.
2GB is the low ball on the amount of memory available in a typical
SDM845 device.
Boot time to userspace on the device Iooked at is: 1149 ms
So total boot time reduction is 0.8%
Total memory increase is 0.00038%
Once more device links are added I expect the boot time impact to be
larger. I think a 1% reduction in boot time for 0.00038% increase in
memory usage is a good trade off.
That 1% faster boot time can also be approximated to 1% reduction in
boot up power usage. That scaled to a million or billion devices
that'll run an Android or some form of ARM Linux kernel is still a
good chunk of energy savings for a small memory increase.
I still stand by the usefulness of this patch, but this is not the
hill I'm going to die on (so if dropping this patch is what it takes
to get modules working, I'll drop it for now).
-Saravana
>
> -Frank
>
>
> On 5/24/19 5:12 PM, Frank Rowand wrote:
> > On 5/24/19 11:21 AM, Saravana Kannan wrote:
> >> On Fri, May 24, 2019 at 10:56 AM Frank Rowand
> >> wrote:
> >>>
> >>> Hi Sarvana,
> >>>
> >>> I'm not reviewing patches 1-5 in any detail, given my reply to patch 0.
> >>>
> >>> But I had already skimmed through this patch before I received the
> >>> email for patch 0, so I want to make one generic comment below,
> >>> to give some feedback as you continue thinking through possible
> >>> implementations to solve the underlying problems.
> >>
> >> Appreciate the feedback Frank!
> >>
> >>>
> >>>
> >>> On 5/23/19 6:01 PM, Saravana Kannan 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
> ---
> 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(_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(_dev_lock);
> + dev = get_device(np->dev);
> + spin_unlock(_dev_lock);
> + if (!dev)
> + dev = bus_find_device(_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;
>
>
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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 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
>> ---
>> 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(_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(_dev_lock);
>> + dev = get_device(np->dev);
>> + spin_unlock(_dev_lock);
>> + if (!dev)
>> + dev = bus_find_device(_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;
>>
>> 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(_dev_lock);
>> + dev->of_node->dev = NULL;
>> + spin_unlock(_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
>>
> .
>
Re: [PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
Hi Saravana,
(I notice that I never seem to spell your name correctly. Apologies for that,
both past and future).
This email was never answered.
-Frank
On 5/24/19 5:12 PM, Frank Rowand wrote:
> On 5/24/19 11:21 AM, Saravana Kannan wrote:
>> On Fri, May 24, 2019 at 10:56 AM Frank Rowand wrote:
>>>
>>> Hi Sarvana,
>>>
>>> I'm not reviewing patches 1-5 in any detail, given my reply to patch 0.
>>>
>>> But I had already skimmed through this patch before I received the
>>> email for patch 0, so I want to make one generic comment below,
>>> to give some feedback as you continue thinking through possible
>>> implementations to solve the underlying problems.
>>
>> Appreciate the feedback Frank!
>>
>>>
>>>
>>> On 5/23/19 6:01 PM, Saravana Kannan 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
---
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(_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(_dev_lock);
+ dev = get_device(np->dev);
+ spin_unlock(_dev_lock);
+ if (!dev)
+ dev = bus_find_device(_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;
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(_dev_lock);
+ dev->of_node->dev = NULL;
+ spin_unlock(_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 */
>>>
>>> We have actively been working on shrinking the size of struct device_node,
>>> as part of reducing the devicetree memory usage. As such, we need strong
>>> justification for adding anything to this struct. For example, proof that
>>> there is a performance problem that can only be solved by increasing the
>>> memory usage.
>>
>> I didn't mean for people to focus on the deferred probe optimization.
>
> I was speaking specifically of the of_find_device_by_node() optimization.
> I did not chase any further back in the call chain to see how that would
> impact anything else. My comments stand, whether this patch is meant
> to optimize deferred probe optimization or to optimize something else.
>
>
>> In reality that was just a added side benefit of this series. The main
>> problem to solve is that of suppliers having to know when all their
>> consumers are up and
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Mon, Jun 10, 2019 at 1:15 PM Saravana Kannan 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
Re: [RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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 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
> ---
> 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(_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(_dev_lock);
> + dev = get_device(np->dev);
> + spin_unlock(_dev_lock);
> + if (!dev)
> + dev = bus_find_device(_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;
>
> 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(_dev_lock);
> + dev->of_node->dev = NULL;
> + spin_unlock(_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
>
[RESEND PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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
---
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(_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(_dev_lock);
+ dev = get_device(np->dev);
+ spin_unlock(_dev_lock);
+ if (!dev)
+ dev = bus_find_device(_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;
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(_dev_lock);
+ dev->of_node->dev = NULL;
+ spin_unlock(_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
Re: [PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On 5/24/19 11:21 AM, Saravana Kannan wrote:
> On Fri, May 24, 2019 at 10:56 AM Frank Rowand wrote:
>>
>> Hi Sarvana,
>>
>> I'm not reviewing patches 1-5 in any detail, given my reply to patch 0.
>>
>> But I had already skimmed through this patch before I received the
>> email for patch 0, so I want to make one generic comment below,
>> to give some feedback as you continue thinking through possible
>> implementations to solve the underlying problems.
>
> Appreciate the feedback Frank!
>
>>
>>
>> On 5/23/19 6:01 PM, Saravana Kannan 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
>>> ---
>>> 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(_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(_dev_lock);
>>> + dev = get_device(np->dev);
>>> + spin_unlock(_dev_lock);
>>> + if (!dev)
>>> + dev = bus_find_device(_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;
>>>
>>> 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(_dev_lock);
>>> + dev->of_node->dev = NULL;
>>> + spin_unlock(_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 */
>>
>> We have actively been working on shrinking the size of struct device_node,
>> as part of reducing the devicetree memory usage. As such, we need strong
>> justification for adding anything to this struct. For example, proof that
>> there is a performance problem that can only be solved by increasing the
>> memory usage.
>
> I didn't mean for people to focus on the deferred probe optimization.
I was speaking specifically of the of_find_device_by_node() optimization.
I did not chase any further back in the call chain to see how that would
impact anything else. My comments stand, whether this patch is meant
to optimize deferred probe optimization or to optimize something else.
> In reality that was just a added side benefit of this series. The main
> problem to solve is that of suppliers having to know when all their
> consumers are up and managing the resources actively, especially in a
> system with loadable modules where we can't depend on the driver to
> notify the supplier because the consumer driver module might not be
> available or loaded until much later.
>
> Having said that, I'm not saying we should go around and waste space
> willy-nilly. But, isn't the memory
Re: [PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
On Fri, May 24, 2019 at 10:56 AM Frank Rowand wrote:
>
> Hi Sarvana,
>
> I'm not reviewing patches 1-5 in any detail, given my reply to patch 0.
>
> But I had already skimmed through this patch before I received the
> email for patch 0, so I want to make one generic comment below,
> to give some feedback as you continue thinking through possible
> implementations to solve the underlying problems.
Appreciate the feedback Frank!
>
>
> On 5/23/19 6:01 PM, Saravana Kannan 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
> > ---
> > 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(_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(_dev_lock);
> > + dev = get_device(np->dev);
> > + spin_unlock(_dev_lock);
> > + if (!dev)
> > + dev = bus_find_device(_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;
> >
> > 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(_dev_lock);
> > + dev->of_node->dev = NULL;
> > + spin_unlock(_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 */
>
> We have actively been working on shrinking the size of struct device_node,
> as part of reducing the devicetree memory usage. As such, we need strong
> justification for adding anything to this struct. For example, proof that
> there is a performance problem that can only be solved by increasing the
> memory usage.
I didn't mean for people to focus on the deferred probe optimization.
In reality that was just a added side benefit of this series. The main
problem to solve is that of suppliers having to know when all their
consumers are up and managing the resources actively, especially in a
system with loadable modules where we can't depend on the driver to
notify the supplier because the consumer driver module might not be
available or loaded until much later.
Having said that, I'm not saying we should go around and waste space
willy-nilly. But, isn't the memory usage going to increase based on
the number of DT nodes present in DT? I'd think as the number of DT
nodes increase it's more likely for those devices have more memory? So
at least in this specific case I think adding the field is justified.
Also, right now the look up is O(n) complexity and if we are trying to
add device links to most of the devices, that whole process becomes
Re: [PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
Hi Sarvana,
I'm not reviewing patches 1-5 in any detail, given my reply to patch 0.
But I had already skimmed through this patch before I received the
email for patch 0, so I want to make one generic comment below,
to give some feedback as you continue thinking through possible
implementations to solve the underlying problems.
On 5/23/19 6:01 PM, Saravana Kannan 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
> ---
> 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(_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(_dev_lock);
> + dev = get_device(np->dev);
> + spin_unlock(_dev_lock);
> + if (!dev)
> + dev = bus_find_device(_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;
>
> 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(_dev_lock);
> + dev->of_node->dev = NULL;
> + spin_unlock(_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 */
We have actively been working on shrinking the size of struct device_node,
as part of reducing the devicetree memory usage. As such, we need strong
justification for adding anything to this struct. For example, proof that
there is a performance problem that can only be solved by increasing the
memory usage.
-Frank
> };
>
> #define MAX_PHANDLE_ARGS 16
>
[PATCH v1 1/5] of/platform: Speed up of_find_device_by_node()
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
---
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(_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(_dev_lock);
+ dev = get_device(np->dev);
+ spin_unlock(_dev_lock);
+ if (!dev)
+ dev = bus_find_device(_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;
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(_dev_lock);
+ dev->of_node->dev = NULL;
+ spin_unlock(_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
