Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1510402 > unrolled thread

Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-10-27 17:40 +0200
    Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Lukas Wunner <lukas@wunner.de> - 2016-10-28 12:00 +0200
    Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-07 22:30 +0100
      Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-08 07:50 +0100
        Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-08 20:30 +0100
          Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-08 20:50 +0100
            Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-08 22:00 +0100
              Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-09 07:50 +0100
                Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Andrzej Hajda <a.hajda@samsung.com> - 2016-11-09 10:40 +0100
                  Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-09 10:50 +0100
      Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-11-10 01:50 +0100
        Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-10 02:10 +0100
          Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-10 08:20 +0100
            Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-10 23:10 +0100
              Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-11-10 23:50 +0100
                Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-11-11 01:10 +0100
          Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support Geert Uytterhoeven <geert@linux-m68k.org> - 2016-11-10 09:50 +0100
            Re: [PATCH v5 2/5] driver core: Functional dependencies tracking  support "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-11-10 23:20 +0100

#1510402 — Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
> Hi Rafael,
> 
> sorry for not responding to v5 of your series earlier, just sending
> this out now in the hope that it reaches you before your travels.
> 
> On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
> > - Modify device_links_check_suppliers(), device_links_driver_bound(),
> >   device_links_no_driver(), device_links_driver_cleanup(), device_links_busy(),
> >   and device_links_unbind_consumers() to walk link lists under device_links_lock
> >   (to make the new "driver presence tracking" mechanism work reliably).
> 
> This change might increase boot time if drivers return -EPROBE_DEFER.

"might"?  Please verify this before guessing....

And don't make this more complex than needed before actually determining
a real issue.

thanks,

greg k-h

[toc] | [next] | [standalone]

#1511014

On Thu, Oct 27, 2016 at 05:25:51PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
> > On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
> > > - Modify device_links_check_suppliers(), device_links_driver_bound(),
> > >   device_links_no_driver(), device_links_driver_cleanup(), device_links_busy(),
> > >   and device_links_unbind_consumers() to walk link lists under device_links_lock
> > >   (to make the new "driver presence tracking" mechanism work reliably).
> > 
> > This change might increase boot time if drivers return -EPROBE_DEFER.
> 
> "might"?  Please verify this before guessing....

I can't, my machine only uses device links for Thunderbolt hotplug ports,
and their driver (portdrv) doesn't use deferred probing.  I'm only aware
of a single device in my system whose driver causes others to defer
probing (apple-gmux), but they don't use device links, so the mutex is
only acquired very briefly because the supplier/consumer lists are empty,
hence the performance impact can't be measured on my system.  But the
situation may be different for Marek's or Hanjun's use cases. I'm not
familiar with their systems, hence the "might".


> And don't make this more complex than needed before actually determining
> a real issue.

As pointed out it currently *is* more complex than needed because the
device_link_state is dispensable.  The whole issue is moot with the
changes I suggested because the mutex would only have to be taken for
addition/deletion of device links, not when probing.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1516599

On Thu, Oct 27, 2016 at 05:25:51PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
> > Hi Rafael,
> > 
> > sorry for not responding to v5 of your series earlier, just sending
> > this out now in the hope that it reaches you before your travels.
> > 
> > On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
> > > - Modify device_links_check_suppliers(), device_links_driver_bound(),
> > >   device_links_no_driver(), device_links_driver_cleanup(), device_links_busy(),
> > >   and device_links_unbind_consumers() to walk link lists under device_links_lock
> > >   (to make the new "driver presence tracking" mechanism work reliably).
> > 
> > This change might increase boot time if drivers return -EPROBE_DEFER.
> 
> "might"?  Please verify this before guessing....
> 
> And don't make this more complex than needed before actually determining
> a real issue.

As clarified by Rafael at Plumbers, this functional dependencies
framework assumes your driver / subsystem supports deferred probe,
if it does not support its not clear what will happen....

We have no explicit semantics to check if a driver / subsystem
supports deferred probe.

  Luis

[toc] | [prev] | [next] | [standalone]

#1516866

On Mon, Nov 07, 2016 at 10:22:50PM +0100, Luis R. Rodriguez wrote:
> We have no explicit semantics to check if a driver / subsystem
> supports deferred probe.

Why would we need such a thing?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1517512

On Tue, Nov 08, 2016 at 07:45:41AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 07, 2016 at 10:22:50PM +0100, Luis R. Rodriguez wrote:
> > We have no explicit semantics to check if a driver / subsystem
> > supports deferred probe.
> 
> Why would we need such a thing?

It depends on the impact of a driver/subsystem not properly supporting
deffered probe, if this is no-op then such a need is not critical but
would be good to proactively inform developers / users so they avoid 
its use, if this will cause issues its perhaps best to make this a
no-op through a check. AFAICT reviewing implications of not supporting
deferred probe on drivers/subsytsems for this framework is not clearly
spelled out, if we start considering re-using this framework for probe
ordering I'd hate to see issues come up without this corner case being
concretely considered.

Furthermore -- how does this framework compare to Andrzej's resource tracking
solution? I confess I have not had a chance yet to review yet but in light of
this question it would be good to know if Andrzej's framework also requires
deferred probe as similar concerns would exist there as well.

  Luis

[toc] | [prev] | [next] | [standalone]

#1517528

On Tue, Nov 08, 2016 at 08:21:04PM +0100, Luis R. Rodriguez wrote:
> On Tue, Nov 08, 2016 at 07:45:41AM +0100, Greg Kroah-Hartman wrote:
> > On Mon, Nov 07, 2016 at 10:22:50PM +0100, Luis R. Rodriguez wrote:
> > > We have no explicit semantics to check if a driver / subsystem
> > > supports deferred probe.
> > 
> > Why would we need such a thing?
> 
> It depends on the impact of a driver/subsystem not properly supporting
> deffered probe, if this is no-op then such a need is not critical but
> would be good to proactively inform developers / users so they avoid 
> its use, if this will cause issues its perhaps best to make this a
> no-op through a check. AFAICT reviewing implications of not supporting
> deferred probe on drivers/subsytsems for this framework is not clearly
> spelled out, if we start considering re-using this framework for probe
> ordering I'd hate to see issues come up without this corner case being
> concretely considered.

It should not matter to the driver core if a subsystem, or a driver,
supports or does not support deferred probe.  It's a quick and simple
solution to a complex problem that works well.  Yes, you can iterate a
lot of times, but that's fine, we have time at boot to do that (and
really, it is fast.)

> Furthermore -- how does this framework compare to Andrzej's resource tracking
> solution? I confess I have not had a chance yet to review yet but in light of
> this question it would be good to know if Andrzej's framework also requires
> deferred probe as similar concerns would exist there as well.

I have no idea what "framework" you are talking about here, do you have
a pointer to patches?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1517579

On Tue, Nov 08, 2016 at 08:43:35PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Nov 08, 2016 at 08:21:04PM +0100, Luis R. Rodriguez wrote:
> > On Tue, Nov 08, 2016 at 07:45:41AM +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Nov 07, 2016 at 10:22:50PM +0100, Luis R. Rodriguez wrote:
> > > > We have no explicit semantics to check if a driver / subsystem
> > > > supports deferred probe.
> > > 
> > > Why would we need such a thing?
> > 
> > It depends on the impact of a driver/subsystem not properly supporting
> > deffered probe, if this is no-op then such a need is not critical but
> > would be good to proactively inform developers / users so they avoid 
> > its use, if this will cause issues its perhaps best to make this a
> > no-op through a check. AFAICT reviewing implications of not supporting
> > deferred probe on drivers/subsytsems for this framework is not clearly
> > spelled out, if we start considering re-using this framework for probe
> > ordering I'd hate to see issues come up without this corner case being
> > concretely considered.
> 
> It should not matter to the driver core if a subsystem, or a driver,
> supports or does not support deferred probe.

That was my impression as well -- however Rafael noted this as an area
worth highlighting. So perhaps he can elaborate.

But at least as per my notes I do have here Geert Uytterhoeven reminding
us that:

  "Some drivers / subsystems don’t support deferred probe yet, such failures
   usually don’t blow up, but cause subtle malfunctioning. Example, an Ethernet
   phy could not get its interrupt as the primary IRQ chip had not been probed
   yet, it reverted to polling though. Sub-optimal." [0]

[0] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003425.html

This sounds more like the existing deffered probe solution has detrimental
unexpected effects on some drivers / subsystems -- its not clear *why*, but
its worth reviewing if there are other drivers and seeing if we need this
annotation.

> It's a quick and simple solution to a complex problem that works well.

We all agree. The recent discussions over probe ordering are simply
optimization considerations -- should the time incurred to use deferred
probe be X and the time incurred when using an alternative is Y and we
determine the time Y is < X we have a winning alternative. Part of my
own big issue with deferred probe is a) its extremely non-deterministic,
b) it seems some folks have thought about similar problems and we might
really be able to do better. I'm not convinced that the functional
dependencies patches are the panacea for probe ordering, and while it
was not intended for that, some folks are assuming it could be. To really
vet this prospect we must really consider what other subsystem have done
and review other alternatives efforts.

> Yes, you can iterate a
> lot of times, but that's fine, we have time at boot to do that (and
> really, it is fast.)

Deferred probe is left for late_initcall() [1] -- this *assumes* that the
driver/subsystem can be loaded so late, and as per Andrzej this solution
isunacceptable/undesirable. So it would be unfair and incorrect to categorize
all drivers in the same boat, in fact given this lone fact it may be
worth revisiting the idea I mentioned about a capability aspect to support
a late deferred probe later. We tend to assume its fine, however it does
not seem to be the case.

[1] drivers/base/dd.c:late_initcall(deferred_probe_initcall);

> > Furthermore -- how does this framework compare to Andrzej's resource tracking
> > solution? I confess I have not had a chance yet to review yet but in light of
> > this question it would be good to know if Andrzej's framework also requires
> > deferred probe as similar concerns would exist there as well.
> 
> I have no idea what "framework" you are talking about here, do you have
> a pointer to patches?

I'm surprised given Andrzej did both Cc you on his patches [2] *and* chimed
in on Rafael's patches to indicate that we likely can integrate PM concerns
into his own "framework" [3]. There was no resolution to this discussion, however
its not IMHO sufficient to brush off Andrzej's points in particular because
Andrzej *is* indicating that his framework:

- Eliminates deferred probe and resulting late_initcall(), consumer registers
callbacks informing when given resources (clock, regulator, etc) becomes
available
- Properly handle resource disappearance (driver unbind, hotplug)
- Track resources which are not vital to the device, but can influence behavior
- Offers simplified resource allocation
- Can be easily expanded to help with power management

Granted I have not reviewed this yet but it at least was on my radar, and
I do believe its worth reviewing this further given the generally expressed
interest to see if we can have a common framework to address both ordering
problems, suspend and probe. At a quick glance the "ghost provider" idea
seems like a rather crazy idea but hey, there may be some goods in there.

It was sad both Andrzej and yourself could not attend the complex dependencies
tracks -- I think it would have been useful. I don't expect us to address a
resolution to probe ordering immediately -- but I am in the hopes we at least
can keep an open mind about the similarity of the problems and see if we can
aim for a clean elegant solution that might help both.

[2] https://lwn.net/Articles/625454/
[3] http://thread.gmane.org/gmane.linux.kernel/2087152

  Luis

[toc] | [prev] | [next] | [standalone]

#1517790

On Tue, Nov 08, 2016 at 09:58:24PM +0100, Luis R. Rodriguez wrote:
> > > Furthermore -- how does this framework compare to Andrzej's resource tracking
> > > solution? I confess I have not had a chance yet to review yet but in light of
> > > this question it would be good to know if Andrzej's framework also requires
> > > deferred probe as similar concerns would exist there as well.
> > 
> > I have no idea what "framework" you are talking about here, do you have
> > a pointer to patches?
> 
> I'm surprised given Andrzej did both Cc you on his patches [2] *and* chimed
> in on Rafael's patches to indicate that we likely can integrate PM concerns
> into his own "framework" [3]. There was no resolution to this discussion, however
> its not IMHO sufficient to brush off Andrzej's points in particular because
> Andrzej *is* indicating that his framework:

Dude, those patches were from 2014!  I can't remember patches people
sent to me a month ago...

> - Eliminates deferred probe and resulting late_initcall(), consumer registers
> callbacks informing when given resources (clock, regulator, etc) becomes
> available
> - Properly handle resource disappearance (driver unbind, hotplug)
> - Track resources which are not vital to the device, but can influence behavior
> - Offers simplified resource allocation
> - Can be easily expanded to help with power management
> 
> Granted I have not reviewed this yet but it at least was on my radar, and
> I do believe its worth reviewing this further given the generally expressed
> interest to see if we can have a common framework to address both ordering
> problems, suspend and probe. At a quick glance the "ghost provider" idea
> seems like a rather crazy idea but hey, there may be some goods in there.

From what I remember, and I could be totally wrong, these patches were
way too complex and required that every subsystem change their
interfaces.  That's not going to work out well, but read the email
threads for the details...

> It was sad both Andrzej and yourself could not attend the complex dependencies
> tracks -- I think it would have been useful.

Sometimes real-life gets in the way of work, sorry :(

> I don't expect us to address a
> resolution to probe ordering immediately -- but I am in the hopes we at least
> can keep an open mind about the similarity of the problems and see if we can
> aim for a clean elegant solution that might help both.

I'll always review patches of what people come up with.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1517853

On 09.11.2016 07:45, Greg Kroah-Hartman wrote:
> On Tue, Nov 08, 2016 at 09:58:24PM +0100, Luis R. Rodriguez wrote:
>>>> Furthermore -- how does this framework compare to Andrzej's resource tracking
>>>> solution? I confess I have not had a chance yet to review yet but in light of
>>>> this question it would be good to know if Andrzej's framework also requires
>>>> deferred probe as similar concerns would exist there as well.
>>> I have no idea what "framework" you are talking about here, do you have
>>> a pointer to patches?
>> I'm surprised given Andrzej did both Cc you on his patches [2] *and* chimed
>> in on Rafael's patches to indicate that we likely can integrate PM concerns
>> into his own "framework" [3]. There was no resolution to this discussion, however
>> its not IMHO sufficient to brush off Andrzej's points in particular because
>> Andrzej *is* indicating that his framework:
> Dude, those patches were from 2014!  I can't remember patches people
> sent to me a month ago...
>
>> - Eliminates deferred probe and resulting late_initcall(), consumer registers
>> callbacks informing when given resources (clock, regulator, etc) becomes
>> available
>> - Properly handle resource disappearance (driver unbind, hotplug)
>> - Track resources which are not vital to the device, but can influence behavior
>> - Offers simplified resource allocation
>> - Can be easily expanded to help with power management
>>
>> Granted I have not reviewed this yet but it at least was on my radar, and
>> I do believe its worth reviewing this further given the generally expressed
>> interest to see if we can have a common framework to address both ordering
>> problems, suspend and probe. At a quick glance the "ghost provider" idea
>> seems like a rather crazy idea but hey, there may be some goods in there.
> >From what I remember, and I could be totally wrong, these patches were
> way too complex and required that every subsystem change their
> interfaces.  That's not going to work out well, but read the email
> threads for the details...

I haven't seen your comment on my patches, except few general questions
regarding one of earlier version of the framework.
So maybe you are talking about different framework.

Regarding complexity, if the subsystem have simple way of
'(un)publishing' resources it just adds single calls to restrack core:
restrack_up, restrack_down in proper places.
Additionally it adds quite simple stuff to encapsulate resource
description and allocation routines into generic *_restrack_desc
structure, see for example patch adding restrack to phy framework[1].

[1]:
https://lists.freedesktop.org/archives/dri-devel/2014-December/073759.html

Regards
Andrzej

>
>> It was sad both Andrzej and yourself could not attend the complex dependencies
>> tracks -- I think it would have been useful.
> Sometimes real-life gets in the way of work, sorry :(
>
>> I don't expect us to address a
>> resolution to probe ordering immediately -- but I am in the hopes we at least
>> can keep an open mind about the similarity of the problems and see if we can
>> aim for a clean elegant solution that might help both.
> I'll always review patches of what people come up with.
>
> thanks,
>
> greg k-h
>
>
>

[toc] | [prev] | [next] | [standalone]

#1517864

On Wed, Nov 09, 2016 at 10:36:54AM +0100, Andrzej Hajda wrote:
> On 09.11.2016 07:45, Greg Kroah-Hartman wrote:
> > On Tue, Nov 08, 2016 at 09:58:24PM +0100, Luis R. Rodriguez wrote:
> >>>> Furthermore -- how does this framework compare to Andrzej's resource tracking
> >>>> solution? I confess I have not had a chance yet to review yet but in light of
> >>>> this question it would be good to know if Andrzej's framework also requires
> >>>> deferred probe as similar concerns would exist there as well.
> >>> I have no idea what "framework" you are talking about here, do you have
> >>> a pointer to patches?
> >> I'm surprised given Andrzej did both Cc you on his patches [2] *and* chimed
> >> in on Rafael's patches to indicate that we likely can integrate PM concerns
> >> into his own "framework" [3]. There was no resolution to this discussion, however
> >> its not IMHO sufficient to brush off Andrzej's points in particular because
> >> Andrzej *is* indicating that his framework:
> > Dude, those patches were from 2014!  I can't remember patches people
> > sent to me a month ago...
> >
> >> - Eliminates deferred probe and resulting late_initcall(), consumer registers
> >> callbacks informing when given resources (clock, regulator, etc) becomes
> >> available
> >> - Properly handle resource disappearance (driver unbind, hotplug)
> >> - Track resources which are not vital to the device, but can influence behavior
> >> - Offers simplified resource allocation
> >> - Can be easily expanded to help with power management
> >>
> >> Granted I have not reviewed this yet but it at least was on my radar, and
> >> I do believe its worth reviewing this further given the generally expressed
> >> interest to see if we can have a common framework to address both ordering
> >> problems, suspend and probe. At a quick glance the "ghost provider" idea
> >> seems like a rather crazy idea but hey, there may be some goods in there.
> > >From what I remember, and I could be totally wrong, these patches were
> > way too complex and required that every subsystem change their
> > interfaces.  That's not going to work out well, but read the email
> > threads for the details...
> 
> I haven't seen your comment on my patches, except few general questions
> regarding one of earlier version of the framework.
> So maybe you are talking about different framework.
> 
> Regarding complexity, if the subsystem have simple way of
> '(un)publishing' resources it just adds single calls to restrack core:
> restrack_up, restrack_down in proper places.
> Additionally it adds quite simple stuff to encapsulate resource
> description and allocation routines into generic *_restrack_desc
> structure, see for example patch adding restrack to phy framework[1].

Ok, again, I have no idea what my response was to a 2 year-old patchset,
again, I can't remember my response to a patchset that was sent just a
month ago...

update it, and repost and we can all go from there if you think it is a
viable solution.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1518605 — Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

On Mon, Nov 7, 2016 at 10:22 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, Oct 27, 2016 at 05:25:51PM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
>> > Hi Rafael,
>> >
>> > sorry for not responding to v5 of your series earlier, just sending
>> > this out now in the hope that it reaches you before your travels.
>> >
>> > On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
>> > > - Modify device_links_check_suppliers(), device_links_driver_bound(),
>> > >   device_links_no_driver(), device_links_driver_cleanup(), device_links_busy(),
>> > >   and device_links_unbind_consumers() to walk link lists under device_links_lock
>> > >   (to make the new "driver presence tracking" mechanism work reliably).
>> >
>> > This change might increase boot time if drivers return -EPROBE_DEFER.
>>
>> "might"?  Please verify this before guessing....
>>
>> And don't make this more complex than needed before actually determining
>> a real issue.
>
> As clarified by Rafael at Plumbers, this functional dependencies
> framework assumes your driver / subsystem supports deferred probe,

It isn't particularly clear what you mean by "support" here.

I guess that you mean that it will allow the ->probe callback to be
invoked for multiple times for the same device/driver combination
without issues.  If that's the case, the way the new code uses
-EPROBE_DEFER doesn't interfere with this, because it will not invoke
the ->probe callbacks for consumers at all until their (required)
suppliers are ready.

> if it does not support its not clear what will happen....

I don't see any problems here, but if you see any, please just say
what they are.

> We have no explicit semantics to check if a driver / subsystem
> supports deferred probe.

That's correct, but then do we need it?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1518610 — Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

On Wed, Nov 9, 2016 at 4:43 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Nov 7, 2016 at 10:22 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> On Thu, Oct 27, 2016 at 05:25:51PM +0200, Greg Kroah-Hartman wrote:
>>> On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
>>> > Hi Rafael,
>>> >
>>> > sorry for not responding to v5 of your series earlier, just sending
>>> > this out now in the hope that it reaches you before your travels.
>>> >
>>> > On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
>>> > > - Modify device_links_check_suppliers(), device_links_driver_bound(),
>>> > >   device_links_no_driver(), device_links_driver_cleanup(), device_links_busy(),
>>> > >   and device_links_unbind_consumers() to walk link lists under device_links_lock
>>> > >   (to make the new "driver presence tracking" mechanism work reliably).
>>> >
>>> > This change might increase boot time if drivers return -EPROBE_DEFER.
>>>
>>> "might"?  Please verify this before guessing....
>>>
>>> And don't make this more complex than needed before actually determining
>>> a real issue.
>>
>> As clarified by Rafael at Plumbers, this functional dependencies
>> framework assumes your driver / subsystem supports deferred probe,
>
> It isn't particularly clear what you mean by "support" here.

I noted some folks had reported issues, and you acknowledged that if
deferred probe was used in some drivers and if this created an issue
the same issue would be seen with this framework. AFAICT there are two
possible issues to consider:

1) the one Geert Uytterhoeven noted. Again I'll note what he had mentioned [0].

  "Some drivers / subsystems don’t support deferred probe yet, such failures
   usually don’t blow up, but cause subtle malfunctioning. Example, an Ethernet
   phy could not get its interrupt as the primary IRQ chip had not been probed
   yet, it reverted to polling though. Sub-optimal." [0]

[0] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003425.html

Geert can you provide more details?

2) Since deferred probe relies on late_initcall() if your driver must
load earlier than this deferred probe can create an issue. Andrzej had
you identified a driver that ran into this and had issues ? If not
this seems like a semantics thing we should consider in extending the
documentation for drivers so that driver writers are aware of this
limitation. I would suppose candidates for this would be anything not
using module_init() or late_initcall() on their inits and have a
probe.

>> if it does not support its not clear what will happen....
>
> I don't see any problems here, but if you see any, please just say
> what they are.
>
>> We have no explicit semantics to check if a driver / subsystem
>> supports deferred probe.
>
> That's correct, but then do we need it?

We can determine this by reviewing the two items above.

  Luis

[toc] | [prev] | [next] | [standalone]

#1518717 — Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

Hi Luis,

On Wednesday 09 Nov 2016 16:59:30 Luis R. Rodriguez wrote:
> On Wed, Nov 9, 2016 at 4:43 PM, Rafael J. Wysocki wrote:
> > On Mon, Nov 7, 2016 at 10:22 PM, Luis R. Rodriguez wrote:
> >> On Thu, Oct 27, 2016 at 05:25:51PM +0200, Greg Kroah-Hartman wrote:
> >>> On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
> >>>> Hi Rafael,
> >>>> 
> >>>> sorry for not responding to v5 of your series earlier, just sending
> >>>> this out now in the hope that it reaches you before your travels.
> >>>> 
> >>>> On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
> >>>>> - Modify device_links_check_suppliers(),
> >>>>> device_links_driver_bound(),
> >>>>> 
> >>>>>   device_links_no_driver(), device_links_driver_cleanup(),
> >>>>>   device_links_busy(), and device_links_unbind_consumers() to walk
> >>>>>   link lists under device_links_lock (to make the new "driver
> >>>>>   presence tracking" mechanism work reliably).
> >>>>
> >>>> This change might increase boot time if drivers return -EPROBE_DEFER.
> >>> 
> >>> "might"?  Please verify this before guessing....
> >>> 
> >>> And don't make this more complex than needed before actually determining
> >>> a real issue.
> >> 
> >> As clarified by Rafael at Plumbers, this functional dependencies
> >> framework assumes your driver / subsystem supports deferred probe,
> > 
> > It isn't particularly clear what you mean by "support" here.
> 
> I noted some folks had reported issues, and you acknowledged that if
> deferred probe was used in some drivers and if this created an issue
> the same issue would be seen with this framework. AFAICT there are two
> possible issues to consider:
> 
> 1) the one Geert Uytterhoeven noted. Again I'll note what he had mentioned
> [0].
> 
>   "Some drivers / subsystems don’t support deferred probe yet, such failures
> usually don’t blow up, but cause subtle malfunctioning. Example, an
> Ethernet phy could not get its interrupt as the primary IRQ chip had not
> been probed yet, it reverted to polling though. Sub-optimal." [0]
> 
> [0]
> https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003
> 425.html
> 
> Geert can you provide more details?

This is a more global issue. In many cases drivers depend on optional 
resources. They are able to operate in a degraded mode (reduced feature set, 
reduced performances, ...) when those resources are not present. They can 
easily determine at probe time whether those resources are present, but have 
no way to know, in case they're absent, whether they will be present at some 
point in the near future (due to another driver probing the device providing 
the resource for instance) or if they will never be present (for instance 
because the required driver is missing). In the first case it would make sense 
to defer probe, in the latter case deferring probe forever for missing 
optional resources would prevent the device from being probed successfully at 
all.

The functional dependencies tracking patch series isn't meant to address this 
issue. I can imagine a framework that would notify drivers of optional 
resource availability after probe time, but it would come at a high cost for 
drivers as switching between modes of operation at runtime based on the 
availability of such resources would be way more complex than a mechanism 
based on probe deferral.

> 2) Since deferred probe relies on late_initcall() if your driver must
> load earlier than this deferred probe can create an issue. Andrzej had
> you identified a driver that ran into this and had issues ? If not
> this seems like a semantics thing we should consider in extending the
> documentation for drivers so that driver writers are aware of this
> limitation. I would suppose candidates for this would be anything not
> using module_init() or late_initcall() on their inits and have a
> probe.
> 
> >> if it does not support its not clear what will happen....
> > 
> > I don't see any problems here, but if you see any, please just say
> > what they are.
> > 
> >> We have no explicit semantics to check if a driver / subsystem
> >> supports deferred probe.
> > 
> > That's correct, but then do we need it?
> 
> We can determine this by reviewing the two items above.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1519391

On Thu, Nov 10, 2016 at 09:14:32AM +0200, Laurent Pinchart wrote:
> Hi Luis,
> 
> On Wednesday 09 Nov 2016 16:59:30 Luis R. Rodriguez wrote:
> > On Wed, Nov 9, 2016 at 4:43 PM, Rafael J. Wysocki wrote:
> > > On Mon, Nov 7, 2016 at 10:22 PM, Luis R. Rodriguez wrote:
> > >> On Thu, Oct 27, 2016 at 05:25:51PM +0200, Greg Kroah-Hartman wrote:
> > >>> On Wed, Oct 26, 2016 at 01:19:02PM +0200, Lukas Wunner wrote:
> > >>>> Hi Rafael,
> > >>>> 
> > >>>> sorry for not responding to v5 of your series earlier, just sending
> > >>>> this out now in the hope that it reaches you before your travels.
> > >>>> 
> > >>>> On Mon, Oct 10, 2016 at 02:51:04PM +0200, Rafael J. Wysocki wrote:
> > >>>>> - Modify device_links_check_suppliers(),
> > >>>>> device_links_driver_bound(),
> > >>>>> 
> > >>>>>   device_links_no_driver(), device_links_driver_cleanup(),
> > >>>>>   device_links_busy(), and device_links_unbind_consumers() to walk
> > >>>>>   link lists under device_links_lock (to make the new "driver
> > >>>>>   presence tracking" mechanism work reliably).
> > >>>>
> > >>>> This change might increase boot time if drivers return -EPROBE_DEFER.
> > >>> 
> > >>> "might"?  Please verify this before guessing....
> > >>> 
> > >>> And don't make this more complex than needed before actually determining
> > >>> a real issue.
> > >> 
> > >> As clarified by Rafael at Plumbers, this functional dependencies
> > >> framework assumes your driver / subsystem supports deferred probe,
> > > 
> > > It isn't particularly clear what you mean by "support" here.
> > 
> > I noted some folks had reported issues, and you acknowledged that if
> > deferred probe was used in some drivers and if this created an issue
> > the same issue would be seen with this framework. AFAICT there are two
> > possible issues to consider:
> > 
> > 1) the one Geert Uytterhoeven noted. Again I'll note what he had mentioned
> > [0].
> > 
> >   "Some drivers / subsystems don’t support deferred probe yet, such failures
> > usually don’t blow up, but cause subtle malfunctioning. Example, an
> > Ethernet phy could not get its interrupt as the primary IRQ chip had not
> > been probed yet, it reverted to polling though. Sub-optimal." [0]
> > 
> > [0]
> > https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003
> > 425.html
> > 
> > Geert can you provide more details?
> 
> This is a more global issue. In many cases drivers depend on optional 
> resources. They are able to operate in a degraded mode (reduced feature set, 
> reduced performances, ...) when those resources are not present. They can 
> easily determine at probe time whether those resources are present, but have 
> no way to know, in case they're absent, whether they will be present at some 
> point in the near future (due to another driver probing the device providing 
> the resource for instance) or if they will never be present (for instance 
> because the required driver is missing).

I see thanks, so -EPROBE_DEFER assumes a late_initcall() would suffice to load
all necessary requirements.

> In the first case it would make sense  to defer probe,

So if the assumption is correct then it -EPROBE_DEFER should work.

> in the latter case deferring probe forever for missing 
> optional resources would prevent the device from being probed successfully at 
> all.

Right I see. And the driver core has no way to know what things *may* be
needed.

> The functional dependencies tracking patch series isn't meant to address this 
> issue.

Right, however it does track functional dependencies for suspend/run time PM
and we were certainly in hope this could help with probe ordering *later* in
the future. This is a separate topic. But more on point, the issue here is that
this framework relies on -EPROBE_DEFER -- so the issues discussed with it still
exist and should be properly documented.

> I can imagine a framework that would notify drivers of optional 
> resource availability after probe time, but it would come at a high cost for 
> drivers as switching between modes of operation at runtime based on the 
> availability of such resources would be way more complex than a mechanism 
> based on probe deferral.

Right, I see.

This is more forward looking, but -- if we had an annotation in Kconfig/turned
to a mod info section, or to start off with just a driver MODULE_SUGGESTS() macro
to start off with it might suffice for the driver core to request_module()
annotated dependencies, such requests could be explicitly suggested as
synchronous so init + probe do run together (as-is today), after which it
could know that all possible drivers that needed to be loaded should now be
loaded. If this sounds plausible to help, do we have drivers where we can
test this on? For instance, since the functional dependency framework
annotates functional dependencies for consumers/providers for suspend/resume
and un time PM could such MODULE_SUGGESTS() annotations be considered on the
consumers to suggest the provider drivers so their own probe yields to their
providers to try first ?

  Luis

> > 2) Since deferred probe relies on late_initcall() if your driver must
> > load earlier than this deferred probe can create an issue. Andrzej had
> > you identified a driver that ran into this and had issues ? If not
> > this seems like a semantics thing we should consider in extending the
> > documentation for drivers so that driver writers are aware of this
> > limitation. I would suppose candidates for this would be anything not
> > using module_init() or late_initcall() on their inits and have a
> > probe.
> > 
> > >> if it does not support its not clear what will happen....
> > > 
> > > I don't see any problems here, but if you see any, please just say
> > > what they are.
> > > 
> > >> We have no explicit semantics to check if a driver / subsystem
> > >> supports deferred probe.
> > > 
> > > That's correct, but then do we need it?
> > 
> > We can determine this by reviewing the two items above.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

[toc] | [prev] | [next] | [standalone]

#1519423

On Thu, Nov 10, 2016 at 11:04:07PM +0100, Luis R. Rodriguez wrote:
> This is more forward looking, but -- if we had an annotation in Kconfig/turned
> to a mod info section, or to start off with just a driver MODULE_SUGGESTS() macro
> to start off with it might suffice for the driver core to request_module()
> annotated dependencies, such requests could be explicitly suggested as
> synchronous so init + probe do run together (as-is today), after which it
> could know that all possible drivers that needed to be loaded should now be
> loaded. If this sounds plausible to help, do we have drivers where we can
> test this on? For instance, since the functional dependency framework
> annotates functional dependencies for consumers/providers for suspend/resume
> and un time PM could such MODULE_SUGGESTS() annotations be considered on the
> consumers to suggest the provider drivers so their own probe yields to their
> providers to try first ?

No.

Stop.

First off, the "driver core" NEVER can "know" if "all possible drivers
that should be loaded, are loaded.  That way lies madness and
impossibility.

Secondly, yet-another-section isn't going to help anything here, we
alredy "suggest" to userspace a bunch of stuff, so we get the needed
modules loaded, at sometime in the future, if they are around, and
userspace feels like it.  That's the best we can ever do.

Don't try to make this more difficult than it is please. DEFER works
today really really well, and it's really really simple.
Inter-dependancy of modules and devices connected to each other are two
different things, be careful about this.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1519471 — Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

Hi Greg,

On Thursday 10 Nov 2016 23:40:54 Greg Kroah-Hartman wrote:
> On Thu, Nov 10, 2016 at 11:04:07PM +0100, Luis R. Rodriguez wrote:
> > This is more forward looking, but -- if we had an annotation in
> > Kconfig/turned to a mod info section, or to start off with just a driver
> > MODULE_SUGGESTS() macro to start off with it might suffice for the driver
> > core to request_module() annotated dependencies, such requests could be
> > explicitly suggested as synchronous so init + probe do run together
> > (as-is today), after which it could know that all possible drivers that
> > needed to be loaded should now be loaded. If this sounds plausible to
> > help, do we have drivers where we can test this on? For instance, since
> > the functional dependency framework annotates functional dependencies for
> > consumers/providers for suspend/resume and un time PM could such
> > MODULE_SUGGESTS() annotations be considered on the consumers to suggest
> > the provider drivers so their own probe yields to their providers to try
> > first ?
> 
> No.
> 
> Stop.
> 
> First off, the "driver core" NEVER can "know" if "all possible drivers
> that should be loaded, are loaded.  That way lies madness and
> impossibility.
> 
> Secondly, yet-another-section isn't going to help anything here, we
> alredy "suggest" to userspace a bunch of stuff, so we get the needed
> modules loaded, at sometime in the future, if they are around, and
> userspace feels like it.  That's the best we can ever do.
> 
> Don't try to make this more difficult than it is please. DEFER works
> today really really well, and it's really really simple.
> Inter-dependancy of modules and devices connected to each other are two
> different things, be careful about this.

One issue we don't address today is handling of optional dependencies. A 
simple example is an SPI controller that can use a DMA engine or work in PIO 
mode. At probe time the driver will request a DMA channel if the platform 
(ACPI, DT, platform data) specifies that DMA is available. This can fail for 
various reasons, one of them being that the DMA engine driver hasn't probed 
the DMA device yet. In that case the SPI controller driver will continue in 
PIO mode, ignoring the DMA engine that will later be probed. We can't defer 
probing of the SPI controller as the DMA engine driver might never get loaded, 
which would result in the SPI controller probe being deferred forever.

One solution for this type of dependency issue would be to notify the SPI 
controller driver after probe that the DMA channel is now available. I'd like 
to avoid that though, as it would drastically increase the complexity of lots 
of drivers and create lots of race conditions.

There are certain configurations that we could possibly consider as invalid. 
For instance if the SPI controller driver is built-in and the DMA engine 
driver built as a module, the user clearly shot themselves in the foot and the 
kernel can't be blamed.

For resources that can't be built as a module (IOMMUs for instance) we thus 
only have to consider the case where both drivers are built-in, as the 
resource built-in and consumer as a module should work properly from an 
ordering point of view (at least as long as we don't allow asynchronous 
probing of built-in drivers to be delayed enough for modules to be loaded...). 
In this case, if the resource driver isn't available when the consumer is 
probed, if will never be available at the consumer can safely proceed in a 
degraded mode. We would thus only need to solve the probe ordering issue.

I'm not sure how far these simple(r) solutions that consider certain cases as 
invalid would scale though, and whether we won't need a more generic solution 
at some point anyway.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1518746 — Re: [PATCH v5 2/5] driver core: Functional dependencies tracking support

On Thu, Nov 10, 2016 at 1:59 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Wed, Nov 9, 2016 at 4:43 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Mon, Nov 7, 2016 at 10:22 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>>> As clarified by Rafael at Plumbers, this functional dependencies
>>> framework assumes your driver / subsystem supports deferred probe,
>>
>> It isn't particularly clear what you mean by "support" here.
>
> I noted some folks had reported issues, and you acknowledged that if
> deferred probe was used in some drivers and if this created an issue
> the same issue would be seen with this framework. AFAICT there are two
> possible issues to consider:
>
> 1) the one Geert Uytterhoeven noted. Again I'll note what he had mentioned [0].
>
>   "Some drivers / subsystems don’t support deferred probe yet, such failures
>    usually don’t blow up, but cause subtle malfunctioning. Example, an Ethernet
>    phy could not get its interrupt as the primary IRQ chip had not been probed
>    yet, it reverted to polling though. Sub-optimal." [0]
>
> [0] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003425.html
>
> Geert can you provide more details?

Issue reported in "of_mdiobus_register_phy() and deferred probe"
(http://lkml.iu.edu/hypermail/linux/kernel/1510.2/05770.html)

Key point is:
"However, of_mdiobus_register_phy() uses irq_of_parse_and_map(), which plainly
 ignores EPROBE_DEFER, and it just continues."

At that time, the PHY driver fell back to polling, but as of commit d5c3d8465
("net: phy: Avoid polling PHY with PHY_IGNORE_INTERRUPTS") that's no longer the
case, and now the PHY fails to work completely.

Workaround is "[PATCH v2] irqchip/renesas-irqc: Postpone driver initialization"
(https://www.spinics.net/lists/netdev/msg403325.html), which seems to have
sparked some interest in fixing the issue for good ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1519393

On Thu, Nov 10, 2016 at 09:46:42AM +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 10, 2016 at 1:59 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Wed, Nov 9, 2016 at 4:43 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> On Mon, Nov 7, 2016 at 10:22 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >>> As clarified by Rafael at Plumbers, this functional dependencies
> >>> framework assumes your driver / subsystem supports deferred probe,
> >>
> >> It isn't particularly clear what you mean by "support" here.
> >
> > I noted some folks had reported issues, and you acknowledged that if
> > deferred probe was used in some drivers and if this created an issue
> > the same issue would be seen with this framework. AFAICT there are two
> > possible issues to consider:
> >
> > 1) the one Geert Uytterhoeven noted. Again I'll note what he had mentioned [0].
> >
> >   "Some drivers / subsystems don’t support deferred probe yet, such failures
> >    usually don’t blow up, but cause subtle malfunctioning. Example, an Ethernet
> >    phy could not get its interrupt as the primary IRQ chip had not been probed
> >    yet, it reverted to polling though. Sub-optimal." [0]
> >
> > [0] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-August/003425.html
> >
> > Geert can you provide more details?
> 
> Issue reported in "of_mdiobus_register_phy() and deferred probe"
> (http://lkml.iu.edu/hypermail/linux/kernel/1510.2/05770.html)
> 
> Key point is:
> "However, of_mdiobus_register_phy() uses irq_of_parse_and_map(), which plainly
>  ignores EPROBE_DEFER, and it just continues."
> 
> At that time, the PHY driver fell back to polling, but as of commit d5c3d8465
> ("net: phy: Avoid polling PHY with PHY_IGNORE_INTERRUPTS") that's no longer the
> case, and now the PHY fails to work completely.
> 
> Workaround is "[PATCH v2] irqchip/renesas-irqc: Postpone driver initialization"
> (https://www.spinics.net/lists/netdev/msg403325.html), which seems to have
> sparked some interest in fixing the issue for good ;-)

Ah playing with init levels. You are lucky here that this suffices, the
IOMMU folks already ran out with enough levels to play with so they cannot
resolve their issue this way, for instance.

 Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web