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

Groups > linux.kernel > #1480608 > unrolled thread

Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices

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: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-10 13:40 +0200
    Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-11 00:10 +0200
      Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-13 20:00 +0200
        Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-09-14 01:20 +0200
          Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices Lukas Wunner <lukas@wunner.de> - 2016-09-18 14:40 +0200

#1480608 — Re: [RFC/RFT][PATCH v2 0/7] Functional dependencies between devices

On Thu, Sep 08, 2016 at 11:25:44PM +0200, Rafael J. Wysocki wrote:
> As discussed in the recent "On-demand device probing" thread and in a
> Kernel Summit session earlier today, there is a problem with handling
> cases where functional dependencies between devices are involved.
> 
> What I mean by a "functional dependency" is when the driver of device B
> needs both device A and its driver to be present and functional to be
> able to work.  This implies that the driver of A needs to be working
> for B to be probed successfully and it cannot be unbound from the
> device before the B's driver.  This also has certain consequences for
> power management of these devices (suspend/resume and runtime PM
> ordering).

As a general observation, this series seems to conflate two
separate issues:

(a) non-hierarchical device dependencies
    (a device depends on another device which is not its parent
    with regards to (runtime) suspend/resume ordering)

(b) driver-presence dependencies
    (a device depends on another device to be bound to a driver
    before it can probe / must be unbound before the other device
    can unbind)

Those two issues are orthogonal.

E.g. a driver-presence dependency may exist between a child and a
parent, or between siblings, whereas a non-hierarchical device
dependency by definition cannot exist between child/parent.

Let's say I need a driver-presence dependency between parent/child.
The PM core already guarantees correct (runtime) suspend/resume
ordering between child. Device links duplicate that functionality.
Oops?

In a way, the approach taken here is somewhat contrary to the UNIX
philosophy to write programs that do one thing and do that well.
It's a single tool which addresses two distinct problems and I think
that makes it more difficult to understand the consequences of
device links, in particular if used between parent/child, and ensure
correctness.

Would it be worth to address issue (b) driver-presence dependencies
with a separate tool? I could envision adding two bits to struct device,
one to indicate that the device must be bound before its children/
consumers can bind, and another to indicate that the parent/suppliers
must be bound before the device can bind. The PM core could check those
bits to decide if it should defer probing. All the code for maintaining
device link state would probably no longer be necessary.

That approach would be more coarse-grained than setting up links
between two devices, but I imagine it would probably suffice in the
majority of use cases. If drivers need something more fine-grained,
they can always test boundness of suppliers in their ->probe hook
and return -EPROBE_DEFER. And they could handle unbinding before
suppliers by listening to BUS_NOTIFY_UNBIND_DRIVER.

As for (a) non-hierarchical device dependencies, that's a gaping
hole that we currently have. Off the cuff I can name 4 such device
dependencies on MacBooks alone where we currently try to make do
with various kludges to ensure correct suspend/resume ordering.
Being able to express such dependencies in a generic way and have
the PM core take care of them would be a godsend.

As for (b) driver-presence dependencies, I only know of a single
use case: Apparently the BCM57785 Ethernet/SDHC controller needs
the Ethernet driver to be loaded for the SDHC reader to work
(https://bugzilla.kernel.org/show_bug.cgi?id=73241#c56). I imagine
some power well is needed for SDHC and is only turned on when the
Ethernet driver is loaded. The two bit solution proposed above
would suffice for this use case: The two devices are siblings
and have no children themselves. (They're functions 0 and 1 of
a PCI endpoint device.)

Thanks,

Lukas

[toc] | [next] | [standalone]

#1480717

On Sat, Sep 10, 2016 at 1:39 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Thu, Sep 08, 2016 at 11:25:44PM +0200, Rafael J. Wysocki wrote:
>> As discussed in the recent "On-demand device probing" thread and in a
>> Kernel Summit session earlier today, there is a problem with handling
>> cases where functional dependencies between devices are involved.
>>
>> What I mean by a "functional dependency" is when the driver of device B
>> needs both device A and its driver to be present and functional to be
>> able to work.  This implies that the driver of A needs to be working
>> for B to be probed successfully and it cannot be unbound from the
>> device before the B's driver.  This also has certain consequences for
>> power management of these devices (suspend/resume and runtime PM
>> ordering).
>
> As a general observation, this series seems to conflate two
> separate issues:
>
> (a) non-hierarchical device dependencies
>     (a device depends on another device which is not its parent
>     with regards to (runtime) suspend/resume ordering)

You need to say what it means that one device depends on another one.
Without that you don't get anything useful.

> (b) driver-presence dependencies
>     (a device depends on another device to be bound to a driver
>     before it can probe / must be unbound before the other device
>     can unbind)
>
> Those two issues are orthogonal.

No, they aren't.

At least for a number of devices (quite likely it would be safe to say
that for the majority of them I think) the supplier can only provide
any useful service to its consumers when it has a driver bound to it
and running and the consumers cannot operate correctly without that
service.  That's why the "unbind consumers before unbinding the
supplier" logic is in there.

And in the context of this series a "dependency" boils down to the
ordering of execution of callbacks of certain type.  Like, for
example, "can I execute ->runtime_suspend() for device A before
executing it for device B?"  If there's a link between A and B where
the former is the supplier, the answer is "no".  The reason why is the
assumption that after ->runtime_suspend() A will stop responding and
will not be able to provide the service in question to B any more.

Likewise, if the driver of A goes away, this device will not be able
to provide the service in question to B any more, so it doesn't make
sense for the driver of B to still be running then.

This is the case I wanted to cover with this series if it was not clear before.

Of course, you don't like what device_links_unbind_consumers() does
because of your "Thunderbolt on Macs" use case, but guess what, you
can add a link flag to keep the consumer around when the supplier
driver goes away.  I just don't want that to be the default behavior.

> E.g. a driver-presence dependency may exist between a child and a
> parent, or between siblings, whereas a non-hierarchical device
> dependency by definition cannot exist between child/parent.
>
> Let's say I need a driver-presence dependency between parent/child.
> The PM core already guarantees correct (runtime) suspend/resume
> ordering between child. Device links duplicate that functionality.
> Oops?

No, they don't and that should be clear.

They are for out-of-the-tree dependencies only and the parent-child
one is *in* the tree.

Thanks,
Rafael

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

#1482675

On Sun, Sep 11, 2016 at 12:04:36AM +0200, Rafael J. Wysocki wrote:
> On Sat, Sep 10, 2016 at 1:39 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Thu, Sep 08, 2016 at 11:25:44PM +0200, Rafael J. Wysocki wrote:
> >> As discussed in the recent "On-demand device probing" thread and in a
> >> Kernel Summit session earlier today, there is a problem with handling
> >> cases where functional dependencies between devices are involved.
> >>
> >> What I mean by a "functional dependency" is when the driver of device B
> >> needs both device A and its driver to be present and functional to be
> >> able to work.  This implies that the driver of A needs to be working
> >> for B to be probed successfully and it cannot be unbound from the
> >> device before the B's driver.  This also has certain consequences for
> >> power management of these devices (suspend/resume and runtime PM
> >> ordering).
> >
> > As a general observation, this series seems to conflate two
> > separate issues:
> >
> > (a) non-hierarchical device dependencies
> >     (a device depends on another device which is not its parent
> >     with regards to (runtime) suspend/resume ordering)
> 
> You need to say what it means that one device depends on another one.
> Without that you don't get anything useful.

As stated above, what it means is that "a device depends on another device
with regards to (runtime) suspend/resume ordering".


> > (b) driver-presence dependencies
> >     (a device depends on another device to be bound to a driver
> >     before it can probe / must be unbound before the other device
> >     can unbind)
> >
> > Those two issues are orthogonal.
> 
> No, they aren't.
> 
> At least for a number of devices (quite likely it would be safe to say
> that for the majority of them I think) the supplier can only provide

You got numbers to back up the majority claim?


> any useful service to its consumers when it has a driver bound to it
> and running and the consumers cannot operate correctly without that
> service.  That's why the "unbind consumers before unbinding the
> supplier" logic is in there.
> 
> And in the context of this series a "dependency" boils down to the
> ordering of execution of callbacks of certain type.  Like, for
> example, "can I execute ->runtime_suspend() for device A before
> executing it for device B?"  If there's a link between A and B where
> the former is the supplier, the answer is "no".  The reason why is the
> assumption that after ->runtime_suspend() A will stop responding and
> will not be able to provide the service in question to B any more.
> 
> Likewise, if the driver of A goes away, this device will not be able
> to provide the service in question to B any more, so it doesn't make
> sense for the driver of B to still be running then.
> 
> This is the case I wanted to cover with this series if it was not clear
> before.

That much was clear to me. I maintain that dependency with regards to
suspend/resume ordering and dependency with regards to driver presence
are two different things. The case you wanted to cover just happens to
be a combination of the two. The series would be more useful if either
type of dependency could be achieved without gratuitously also getting
the other type.

You could make the case that providing two separate APIs seems
wasteful since it would increase the amount of code and sometimes
both types of dependency are required at the same time anyway.
That would be a valid argument. But you should design the API thus
that either dependency type is optional.

Providing just dependency with regards to suspend/resume ordering
should be as simple as adding a "DEVICE_LINKS_DRIVER_DONT_CARE" flag.
Then the various calls that you're adding in dd.c would only have to
be called if the flag isn't set, same for the various places where
you check the link state.


> Of course, you don't like what device_links_unbind_consumers() does
> because of your "Thunderbolt on Macs" use case, but guess what,

To name a different use case: On hybrid graphics laptops, the discrete
GPU usually includes an HDA controller for external HDMI displays.
The GPU and the HDA controllers are siblings (functions 0 and 1 of a
PCI slot), yet in many laptops power is cut to both devices when _PS3
is executed for the GPU function. Currently we have a kludge where
the HDA controller is suspended before the GPU is powered down
(see vga_switcheroo_init_domain_pm_optimus_hdmi_audio()).

I envision the HDA controller to be a consumer of the GPU in those
cases, thus ensuring that it's suspended before power is cut.

The way your series works right now, the HDA controller can't be
bound unless the GPU is bound. That restriction is unnecessary.


I've also heard from the i915 folks that on some Intel chipsets, the
integrated HDA controller depends on power wells that are controlled
by the integrated GPU. I'm not sure if a driver for the GPU needs
to be present in this use case.


> you can add a link flag to keep the consumer around when the supplier
> driver goes away.  I just don't want that to be the default behavior.

I assume you're referring to the DEVICE_LINK_PERSISTENT flag and that
a separate patch is needed to make this work, as you've pointed out
in your e-mail of Sep 7.

According to the commit message, such links need to be added before
the consumer probes, which I fear will require addition of PCI quirks.
That's not very pretty, a DEVICE_LINKS_DRIVER_DONT_CARE flag would
seem preferrable.


> > E.g. a driver-presence dependency may exist between a child and a
> > parent, or between siblings, whereas a non-hierarchical device
> > dependency by definition cannot exist between child/parent.
> >
> > Let's say I need a driver-presence dependency between parent/child.
> > The PM core already guarantees correct (runtime) suspend/resume
> > ordering between child. Device links duplicate that functionality.
> > Oops?
> 
> No, they don't and that should be clear.
> 
> They are for out-of-the-tree dependencies only and the parent-child
> one is *in* the tree.

I'm sure there are situations where a driver presence dependency
is needed between parent/child and you should fully expect that
developers will try to employ device links for these use cases.
Which means that the code for suspend/resume device ordering is
executed twice.

Best regards,

Lukas

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

#1482841

On Tuesday, September 13, 2016 07:57:31 PM Lukas Wunner wrote:
> On Sun, Sep 11, 2016 at 12:04:36AM +0200, Rafael J. Wysocki wrote:
> > On Sat, Sep 10, 2016 at 1:39 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Sep 08, 2016 at 11:25:44PM +0200, Rafael J. Wysocki wrote:
> > >> As discussed in the recent "On-demand device probing" thread and in a
> > >> Kernel Summit session earlier today, there is a problem with handling
> > >> cases where functional dependencies between devices are involved.
> > >>
> > >> What I mean by a "functional dependency" is when the driver of device B
> > >> needs both device A and its driver to be present and functional to be
> > >> able to work.  This implies that the driver of A needs to be working
> > >> for B to be probed successfully and it cannot be unbound from the
> > >> device before the B's driver.  This also has certain consequences for
> > >> power management of these devices (suspend/resume and runtime PM
> > >> ordering).
> > >
> > > As a general observation, this series seems to conflate two
> > > separate issues:
> > >
> > > (a) non-hierarchical device dependencies
> > >     (a device depends on another device which is not its parent
> > >     with regards to (runtime) suspend/resume ordering)
> > 
> > You need to say what it means that one device depends on another one.
> > Without that you don't get anything useful.
> 
> As stated above, what it means is that "a device depends on another device
> with regards to (runtime) suspend/resume ordering".

But this series isn't only about suspend/resume, which seems to be your point,
right?

> > > (b) driver-presence dependencies
> > >     (a device depends on another device to be bound to a driver
> > >     before it can probe / must be unbound before the other device
> > >     can unbind)
> > >
> > > Those two issues are orthogonal.
> > 
> > No, they aren't.
> > 
> > At least for a number of devices (quite likely it would be safe to say
> > that for the majority of them I think) the supplier can only provide
> 
> You got numbers to back up the majority claim?

This isn't a claim (which is why I said "quite likely" and so on).

I have examples, though.  I2C/SPI/GPIO controllers providing connections/pins
to devices that aren't their children, IOMMUs.

> > any useful service to its consumers when it has a driver bound to it
> > and running and the consumers cannot operate correctly without that
> > service.  That's why the "unbind consumers before unbinding the
> > supplier" logic is in there.
> > 
> > And in the context of this series a "dependency" boils down to the
> > ordering of execution of callbacks of certain type.  Like, for
> > example, "can I execute ->runtime_suspend() for device A before
> > executing it for device B?"  If there's a link between A and B where
> > the former is the supplier, the answer is "no".  The reason why is the
> > assumption that after ->runtime_suspend() A will stop responding and
> > will not be able to provide the service in question to B any more.
> > 
> > Likewise, if the driver of A goes away, this device will not be able
> > to provide the service in question to B any more, so it doesn't make
> > sense for the driver of B to still be running then.
> > 
> > This is the case I wanted to cover with this series if it was not clear
> > before.
> 
> That much was clear to me. I maintain that dependency with regards to
> suspend/resume ordering and dependency with regards to driver presence
> are two different things. The case you wanted to cover just happens to
> be a combination of the two. The series would be more useful if either
> type of dependency could be achieved without gratuitously also getting
> the other type.

It looks like you're referring to the previous iteration of the series here.

> You could make the case that providing two separate APIs seems
> wasteful since it would increase the amount of code and sometimes
> both types of dependency are required at the same time anyway.
> That would be a valid argument.

Well, that was my thinking, but I guess it wasn't clearly stated.

Maybe except that IMO it is "sometimes one type of the dependency is not
required" rather than the other way around.

> But you should design the API thus that either dependency type is optional.
> 
> Providing just dependency with regards to suspend/resume ordering
> should be as simple as adding a "DEVICE_LINKS_DRIVER_DONT_CARE" flag.
> Then the various calls that you're adding in dd.c would only have to
> be called if the flag isn't set, same for the various places where
> you check the link state.

OK, fair enough.  But that could be added later too.

> > Of course, you don't like what device_links_unbind_consumers() does
> > because of your "Thunderbolt on Macs" use case, but guess what,
> 
> To name a different use case: On hybrid graphics laptops, the discrete
> GPU usually includes an HDA controller for external HDMI displays.
> The GPU and the HDA controllers are siblings (functions 0 and 1 of a
> PCI slot), yet in many laptops power is cut to both devices when _PS3
> is executed for the GPU function. Currently we have a kludge where
> the HDA controller is suspended before the GPU is powered down
> (see vga_switcheroo_init_domain_pm_optimus_hdmi_audio()).
> 
> I envision the HDA controller to be a consumer of the GPU in those
> cases, thus ensuring that it's suspended before power is cut.

So this example isn't a good one IMO.  That clearly is a case when two
(or more) devices share power resources controlled by a single on/off
switch.  Which is a clear use case for a PM domain.

> The way your series works right now, the HDA controller can't be
> bound unless the GPU is bound. That restriction is unnecessary.

That depends on when the link is created, actually.  If the link is created
in the "active" state from the supplier probe, there's no such restriction
AFAICS.

The restriction is only there if (a) that is a permanent link and (b) it
was created before probing both the supplier and the consumer.

> I've also heard from the i915 folks that on some Intel chipsets, the
> integrated HDA controller depends on power wells that are controlled
> by the integrated GPU. I'm not sure if a driver for the GPU needs
> to be present in this use case.
> 
> 
> > you can add a link flag to keep the consumer around when the supplier
> > driver goes away.  I just don't want that to be the default behavior.
> 
> I assume you're referring to the DEVICE_LINK_PERSISTENT flag and that
> a separate patch is needed to make this work, as you've pointed out
> in your e-mail of Sep 7.

No, I'm not.  I was thinking about something like the DEVICE_LINKS_DRIVER_DONT_CARE
you mentioned above.  It would be easy enough to add on top of this series,
but I can put it in there if you insist.

> According to the commit message, such links need to be added before
> the consumer probes, which I fear will require addition of PCI quirks.
> That's not very pretty, a DEVICE_LINKS_DRIVER_DONT_CARE flag would
> seem preferrable.

The commit message may be outdated.

With the series as is you can create a link in (almost) any state from
anywhere.

> > > E.g. a driver-presence dependency may exist between a child and a
> > > parent, or between siblings, whereas a non-hierarchical device
> > > dependency by definition cannot exist between child/parent.
> > >
> > > Let's say I need a driver-presence dependency between parent/child.
> > > The PM core already guarantees correct (runtime) suspend/resume
> > > ordering between child. Device links duplicate that functionality.
> > > Oops?
> > 
> > No, they don't and that should be clear.
> > 
> > They are for out-of-the-tree dependencies only and the parent-child
> > one is *in* the tree.
> 
> I'm sure there are situations where a driver presence dependency
> is needed between parent/child and you should fully expect that
> developers will try to employ device links for these use cases.
> Which means that the code for suspend/resume device ordering is
> executed twice.

Creating a link between a parent and child would be a bug.  I guess
device_link_add() should just return NULL on an attempt to do that.

Thanks,
Rafael

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

#1485783

On Wed, Sep 14, 2016 at 01:18:16AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 13, 2016 07:57:31 PM Lukas Wunner wrote:
> > To name a different use case: On hybrid graphics laptops, the discrete
> > GPU usually includes an HDA controller for external HDMI displays.
> > The GPU and the HDA controllers are siblings (functions 0 and 1 of a
> > PCI slot), yet in many laptops power is cut to both devices when _PS3
> > is executed for the GPU function. Currently we have a kludge where
> > the HDA controller is suspended before the GPU is powered down
> > (see vga_switcheroo_init_domain_pm_optimus_hdmi_audio()).
> > 
> > I envision the HDA controller to be a consumer of the GPU in those
> > cases, thus ensuring that it's suspended before power is cut.
> 
> So this example isn't a good one IMO.  That clearly is a case when two
> (or more) devices share power resources controlled by a single on/off
> switch.  Which is a clear use case for a PM domain.

TBH, I've never understood how a PM domain is supposed to solve this.
When power is cut at runtime for a struct dev_pm_domain, all devices
that were assigned this PM domain with dev_pm_domain_set() need to be
runtime suspended.  This requires that a list of devices is maintained
which were assigned the same PM domain, and that the PM domain's
->runtime_suspend hook isn't executed before all of these devices have
runtime_suspended.  Maybe I'm missing something but I don't see any code
to guarantee that in drivers/base/power/.  Rather, the PM domain's
->runtime_suspend hook is executed as soon as one of the devices in the
PM domain runtime suspends, *without* taking into consideration the
other devices in the PM domain.  They'll just be hanging in the air
with their device powered down.

From what I've seen, people simply use struct dev_pm_domain as a way to
override the bus callbacks.  At least that's what Dave Airlie does in
vga_switcheroo.  But fundamentally that has nothing to do with shared
power resources, it only has to do with enforcing a different behaviour
than the bus.

Thus I don't understand what you mean if you say this is a use case for
a PM domain.


> > I'm sure there are situations where a driver presence dependency
> > is needed between parent/child and you should fully expect that
> > developers will try to employ device links for these use cases.
> > Which means that the code for suspend/resume device ordering is
> > executed twice.
> 
> Creating a link between a parent and child would be a bug.  I guess
> device_link_add() should just return NULL on an attempt to do that.

To be clear, while linking a parent (as consumer) to a child
(as supplier) needs to be prevented since it introduces a dependency
loop, the converse should IMO be allowed.

That would be the case when someone needs a driver presence dependency,
but doesn't need a suspend/resume ordering dependency (because it's
already guaranteed by the PM core for parent/child).  In that case the
child will simultaneously be a consumer, which means e.g. that dpm_wait()
will be executed twice for the same device, but that overhead is probably
negligible.

Thanks,

Lukas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web