Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Wed, Sep 28, 2016 at 01:31:36PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 28, 2016 at 12:43 PM, Lukas Wunner wrote:
> > On Tue, Sep 27, 2016 at 01:52:48PM +0200, Rafael J. Wysocki wrote:
> >> On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner wrote:
> >> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> >> >> +void device_links_unbind_consumers(struct device *dev)
> >> >> +{
> >> >> + struct device_link *link;
> >> >> + int idx;
> >> >> +
> >> >> + start:
> >> >> + idx = device_links_read_lock();
> >> >> +
> >> >> + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
> >> >> + enum device_link_status status;
> >> >> +
> >> >> + if (link->flags & DEVICE_LINK_STATELESS)
> >> >> + continue;
> >> >> +
> >> >> + spin_lock(&link->lock);
> >> >> + status = link->status;
> >> >> + if (status == DEVICE_LINK_CONSUMER_PROBE) {
> >> >> + spin_unlock(&link->lock);
> >> >> +
> >> >> + device_links_read_unlock(idx);
> >> >> +
> >> >> + wait_for_device_probe();
> >> >> + goto start;
> >> >> + }
> >> >> + link->status = DEVICE_LINK_SUPPLIER_UNBIND;
> >> >
> >> > While revisiting this function it just occurred to me that there's
> >> > a theoretical infinite loop here if the consumer probes, is unbound
> >> > by the supplier, then reprobes again before the supplier had a chance
> >> > to update the link to DEVICE_LINK_SUPPLIER_UNBIND. Perhaps this isn't
> >> > a problem in practice, but noting anyway.
> >>
> >> But the consumer is unbound only after setting the link status to
> >> DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.
> >
> > Sorry, looking at the code with a fresh pair of eyeballs I realize the
> > scenario for the infinite loop is different from what I've written above:
> > The infinite loop can occur if the consumer probes continuously but never
> > succeeds, e.g. due to some unfulfilled condition in its ->probe hook.
>
> I'm not sure how that can happen.
>
> If it doesn't succeed, the driver's ->probe() will return an error, so
> that driver is not going to be tried again, unless the error is
> -EPROBE_DEFER, but that will cause it to wait for another driver to
> probe successfully in the meantime.
You're right, it seems that the code is safe. Sorry for the noise. :)
Best regards,
Lukas
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Wed, Sep 28, 2016 at 12:43 PM, Lukas Wunner wrote:
> On Tue, Sep 27, 2016 at 01:52:48PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner wrote:
>> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
>> >> +void device_links_unbind_consumers(struct device *dev)
>> >> +{
>> >> + struct device_link *link;
>> >> + int idx;
>> >> +
>> >> + start:
>> >> + idx = device_links_read_lock();
>> >> +
>> >> + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
>> >> + enum device_link_status status;
>> >> +
>> >> + if (link->flags & DEVICE_LINK_STATELESS)
>> >> + continue;
>> >> +
>> >> + spin_lock(&link->lock);
>> >> + status = link->status;
>> >> + if (status == DEVICE_LINK_CONSUMER_PROBE) {
>> >> + spin_unlock(&link->lock);
>> >> +
>> >> + device_links_read_unlock(idx);
>> >> +
>> >> + wait_for_device_probe();
>> >> + goto start;
>> >> + }
>> >> + link->status = DEVICE_LINK_SUPPLIER_UNBIND;
>> >
>> > While revisiting this function it just occurred to me that there's
>> > a theoretical infinite loop here if the consumer probes, is unbound
>> > by the supplier, then reprobes again before the supplier had a chance
>> > to update the link to DEVICE_LINK_SUPPLIER_UNBIND. Perhaps this isn't
>> > a problem in practice, but noting anyway.
>>
>> But the consumer is unbound only after setting the link status to
>> DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.
>
> Sorry, looking at the code with a fresh pair of eyeballs I realize the
> scenario for the infinite loop is different from what I've written above:
> The infinite loop can occur if the consumer probes continuously but never
> succeeds, e.g. due to some unfulfilled condition in its ->probe hook.
I'm not sure how that can happen.
If it doesn't succeed, the driver's ->probe() will return an error, so
that driver is not going to be tried again, unless the error is
-EPROBE_DEFER, but that will cause it to wait for another driver to
probe successfully in the meantime.
Or do you have any particular example in which things work differently in mind?
Thanks,
Rafael
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Tue, Sep 27, 2016 at 01:52:48PM +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner wrote:
> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> >> +void device_links_unbind_consumers(struct device *dev)
> >> +{
> >> + struct device_link *link;
> >> + int idx;
> >> +
> >> + start:
> >> + idx = device_links_read_lock();
> >> +
> >> + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
> >> + enum device_link_status status;
> >> +
> >> + if (link->flags & DEVICE_LINK_STATELESS)
> >> + continue;
> >> +
> >> + spin_lock(&link->lock);
> >> + status = link->status;
> >> + if (status == DEVICE_LINK_CONSUMER_PROBE) {
> >> + spin_unlock(&link->lock);
> >> +
> >> + device_links_read_unlock(idx);
> >> +
> >> + wait_for_device_probe();
> >> + goto start;
> >> + }
> >> + link->status = DEVICE_LINK_SUPPLIER_UNBIND;
> >
> > While revisiting this function it just occurred to me that there's
> > a theoretical infinite loop here if the consumer probes, is unbound
> > by the supplier, then reprobes again before the supplier had a chance
> > to update the link to DEVICE_LINK_SUPPLIER_UNBIND. Perhaps this isn't
> > a problem in practice, but noting anyway.
>
> But the consumer is unbound only after setting the link status to
> DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.
Sorry, looking at the code with a fresh pair of eyeballs I realize the
scenario for the infinite loop is different from what I've written above:
The infinite loop can occur if the consumer probes continuously but never
succeeds, e.g. due to some unfulfilled condition in its ->probe hook.
That could be fixed by moving the assignment
link->status = DEVICE_LINK_SUPPLIER_UNBIND;
above the preceding if-block (but below "status = link->status;").
The next time the consumer probes, it will return with -EPROBE_DEFER
(return value of device_links_check_suppliers()).
However the semantics of DEVICE_LINK_SUPPLIER_UNBIND are "consumer not
bound and blocked from probing", with the above change it would become
"consumer may or may not be bound and blocked from probing".
Thus it would also be necessary to change device_links_driver_bound()
so that it doesn't update the status to DEVICE_LINK_ACTIVE. Also,
device_links_busy() and device_links_unbind_consumers() would have
to check boundness with device_is_bound() if the status is
DEVICE_LINK_SUPPLIER_UNBIND. Perhaps it would be easier to add
separate link states for this, or perhaps this problem is too
theoretical to bother dealing with it.
Thanks,
Lukas
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Mon, Sep 26, 2016 at 6:51 PM, Lukas Wunner wrote:
> On Fri, Sep 23, 2016 at 03:42:31PM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, September 20, 2016 12:46:30 AM Lukas Wunner wrote:
>> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
>> > > +void device_links_no_driver(struct device *dev)
>> > > +{
>> > > + struct device_link *link, *ln;
>> > > +
>> > > + mutex_lock(&device_links_lock);
>> > > +
>> > > + list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers,
>> > > c_node) {
>> > > + if (link->flags & DEVICE_LINK_STATELESS)
>> > > + continue;
>> > > +
>> > > + if (link->flags & DEVICE_LINK_AUTOREMOVE) {
>> > > + __device_link_del(link);
>> >
>> > The link will be autoremoved not only when the consumer unbinds,
>> > but also when probing the consumer fails.
>> >
>> > Looks like a bug.
>>
>> It really was intentional, because the use case I see for AUTOREMOVE (and
>> the only one to be honest) is when the link is created by the consumer
>> probe in which case it wants to avoid worrying about the cleanup part.
>>
>> Which also is applicable to the cleanup when the probe fails IMO.
>
> You're right, makes sense.
>
>
>> > > +void device_links_unbind_consumers(struct device *dev)
>> > > +{
>> > > + struct device_link *link;
>> > > + int idx;
>> > > +
>> > > + start:
>> > > + idx = device_links_read_lock();
>> > > +
>> > > + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
>> > > + enum device_link_status status;
>> > > +
>> > > + if (link->flags & DEVICE_LINK_STATELESS)
>> > > + continue;
>> > > +
>> > > + spin_lock(&link->lock);
>> > > + status = link->status;
>> > > + if (status == DEVICE_LINK_CONSUMER_PROBE) {
>> > > + spin_unlock(&link->lock);
>> > > +
>> > > + device_links_read_unlock(idx);
>> > > +
>> > > + wait_for_device_probe();
>> > > + goto start;
>> > > + }
>> > > + link->status = DEVICE_LINK_SUPPLIER_UNBIND;
>> > > + if (status == DEVICE_LINK_ACTIVE) {
>> > > + struct device *consumer = link->consumer;
>> > > +
>> > > + get_device(consumer);
>> > > + spin_unlock(&link->lock);
>> >
>> > The lock is released both at the beginning of this if-block and
>> > immediately after the if-block (in case the if-condition is false).
>> > Why not simply release the lock *before* the if-block?
>>
>> Because the get_device() needs to be done under the lock.
>
> According to the commit message, the spinlock only protects the status
> field and the consumer device is prevented from disappearing with the RCU.
> So the spin lock could be released before the if-block AFAICS.
> (But perhaps there are style/readability reasons to have the unlock both
> in the if-block and afterwards.)
OK
Apparently, I was worrying about that the link might go away after the
device_links_read_unlock() in the if () block, so the object pointed
to by "consumer" had to be prevented from going away as well at that
point, but you are right that it's sufficient to call the get_device()
before the device_links_read_unlock() for that and it doesn't have to
go under the spinlock.
>> > > @@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
>> > > {
>> > > struct device *parent = dev->parent;
>> > > struct class_interface *class_intf;
>> > > + struct device_link *link, *ln;
>> > >
>> > > /* Notify clients of device removal. This call must come
>> > >* before dpm_sysfs_remove().
>> > > @@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
>> > > if (dev->bus)
>> > > blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>> > >BUS_NOTIFY_DEL_DEVICE, dev);
>> > > +
>> > > + /*
>> > > + * Delete all of the remaining links from this device to any other
>> > > + * devices (either consumers or suppliers).
>> > > + *
>> > > + * This requires that all links be dormant, so warn if that's no the
>> > > + * case.
>> > > + */
>> > > + mutex_lock(&device_links_lock);
>> > > +
>> > > + list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers,
>> > > c_node) {
>> > > + WARN_ON(link->status != DEVICE_LINK_DORMANT &&
>> > > + !(link->flags & DEVICE_LINK_STATELESS));
>> > > + __device_link_del(link);
>> > > + }
>> >
>> > Shouldn't it also be legal for the supplier links to be in
>> > DEVICE_LINK_AVAILABLE state upon removal of a consumer device?
>> >
>> > (And perhaps also DEVICE_LINK_SUPPLIER_UNBIND?)
>> >
>> > Looks like a bug.
>>
>> But this is done after removing the supplier driver, so the state should be
>> DORMANT (unless the link is stateless), shouldn't it?
>
> The scenario I have in mind is that the supplier device is bound to a
> driver and the consumer device has no driver and is being removed.
> In that case the status will be DEVICE_LI
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner wrote:
> On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
>> +void device_links_unbind_consumers(struct device *dev)
>> +{
>> + struct device_link *link;
>> + int idx;
>> +
>> + start:
>> + idx = device_links_read_lock();
>> +
>> + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
>> + enum device_link_status status;
>> +
>> + if (link->flags & DEVICE_LINK_STATELESS)
>> + continue;
>> +
>> + spin_lock(&link->lock);
>> + status = link->status;
>> + if (status == DEVICE_LINK_CONSUMER_PROBE) {
>> + spin_unlock(&link->lock);
>> +
>> + device_links_read_unlock(idx);
>> +
>> + wait_for_device_probe();
>> + goto start;
>> + }
>
> While revisiting this function it just occurred to me that there's
> a theoretical infinite loop here if the consumer probes, is unbound
> by the supplier, then reprobes again before the supplier had a chance
> to update the link to DEVICE_LINK_SUPPLIER_UNBIND. Perhaps this isn't
> a problem in practice, but noting anyway.
But the consumer is unbound only after setting the link status to
DEVICE_LINK_SUPPLIER_UNBIND and then it won't probe again.
Or am I overlooking something?
Thanks,
Rafael
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> +void device_links_unbind_consumers(struct device *dev)
> +{
> + struct device_link *link;
> + int idx;
> +
> + start:
> + idx = device_links_read_lock();
> +
> + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
> + enum device_link_status status;
> +
> + if (link->flags & DEVICE_LINK_STATELESS)
> + continue;
> +
> + spin_lock(&link->lock);
> + status = link->status;
> + if (status == DEVICE_LINK_CONSUMER_PROBE) {
> + spin_unlock(&link->lock);
> +
> + device_links_read_unlock(idx);
> +
> + wait_for_device_probe();
> + goto start;
> + }
While revisiting this function it just occurred to me that there's
a theoretical infinite loop here if the consumer probes, is unbound
by the supplier, then reprobes again before the supplier had a chance
to update the link to DEVICE_LINK_SUPPLIER_UNBIND. Perhaps this isn't
a problem in practice, but noting anyway.
The problem is that the link state is written to both by the supplier
and consumer. If there was a separate bit in struct device_link to
indicate the supplier's desire to unbind, the problem would go away.
However a mix of such a bit plus the state machine would become
somewhat confusing...
Best regards,
Lukas
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Fri, Sep 23, 2016 at 03:42:31PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 20, 2016 12:46:30 AM Lukas Wunner wrote:
> > On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> > > +void device_links_no_driver(struct device *dev)
> > > +{
> > > + struct device_link *link, *ln;
> > > +
> > > + mutex_lock(&device_links_lock);
> > > +
> > > + list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers,
> > > c_node) {
> > > + if (link->flags & DEVICE_LINK_STATELESS)
> > > + continue;
> > > +
> > > + if (link->flags & DEVICE_LINK_AUTOREMOVE) {
> > > + __device_link_del(link);
> >
> > The link will be autoremoved not only when the consumer unbinds,
> > but also when probing the consumer fails.
> >
> > Looks like a bug.
>
> It really was intentional, because the use case I see for AUTOREMOVE (and
> the only one to be honest) is when the link is created by the consumer
> probe in which case it wants to avoid worrying about the cleanup part.
>
> Which also is applicable to the cleanup when the probe fails IMO.
You're right, makes sense.
> > > +void device_links_unbind_consumers(struct device *dev)
> > > +{
> > > + struct device_link *link;
> > > + int idx;
> > > +
> > > + start:
> > > + idx = device_links_read_lock();
> > > +
> > > + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
> > > + enum device_link_status status;
> > > +
> > > + if (link->flags & DEVICE_LINK_STATELESS)
> > > + continue;
> > > +
> > > + spin_lock(&link->lock);
> > > + status = link->status;
> > > + if (status == DEVICE_LINK_CONSUMER_PROBE) {
> > > + spin_unlock(&link->lock);
> > > +
> > > + device_links_read_unlock(idx);
> > > +
> > > + wait_for_device_probe();
> > > + goto start;
> > > + }
> > > + link->status = DEVICE_LINK_SUPPLIER_UNBIND;
> > > + if (status == DEVICE_LINK_ACTIVE) {
> > > + struct device *consumer = link->consumer;
> > > +
> > > + get_device(consumer);
> > > + spin_unlock(&link->lock);
> >
> > The lock is released both at the beginning of this if-block and
> > immediately after the if-block (in case the if-condition is false).
> > Why not simply release the lock *before* the if-block?
>
> Because the get_device() needs to be done under the lock.
According to the commit message, the spinlock only protects the status
field and the consumer device is prevented from disappearing with the RCU.
So the spin lock could be released before the if-block AFAICS.
(But perhaps there are style/readability reasons to have the unlock both
in the if-block and afterwards.)
> > > @@ -1233,6 +1680,7 @@ void device_del(struct device *dev)
> > > {
> > > struct device *parent = dev->parent;
> > > struct class_interface *class_intf;
> > > + struct device_link *link, *ln;
> > >
> > > /* Notify clients of device removal. This call must come
> > >* before dpm_sysfs_remove().
> > > @@ -1240,6 +1688,30 @@ void device_del(struct device *dev)
> > > if (dev->bus)
> > > blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> > >BUS_NOTIFY_DEL_DEVICE, dev);
> > > +
> > > + /*
> > > + * Delete all of the remaining links from this device to any other
> > > + * devices (either consumers or suppliers).
> > > + *
> > > + * This requires that all links be dormant, so warn if that's no the
> > > + * case.
> > > + */
> > > + mutex_lock(&device_links_lock);
> > > +
> > > + list_for_each_entry_safe_reverse(link, ln, &dev->links_to_suppliers,
> > > c_node) {
> > > + WARN_ON(link->status != DEVICE_LINK_DORMANT &&
> > > + !(link->flags & DEVICE_LINK_STATELESS));
> > > + __device_link_del(link);
> > > + }
> >
> > Shouldn't it also be legal for the supplier links to be in
> > DEVICE_LINK_AVAILABLE state upon removal of a consumer device?
> >
> > (And perhaps also DEVICE_LINK_SUPPLIER_UNBIND?)
> >
> > Looks like a bug.
>
> But this is done after removing the supplier driver, so the state should be
> DORMANT (unless the link is stateless), shouldn't it?
The scenario I have in mind is that the supplier device is bound to a
driver and the consumer device has no driver and is being removed.
In that case the status will be DEVICE_LINK_AVAILABLE and the user
will get a WARN splat, which seems gratuitous because it should be legal.
And the other scenario is when the supplier is unbinding. It iterates
over the links to consumers and puts them in DEVICE_LINK_SUPPLIER_UNBIND.
Let's say the link to consumer A was put into that state, but there's
a consumer B remaining which is bound. The RCU and spinlock are unlocked
before device_release_driver_internal() is called for that consumer.
If at that point consumer device A is removed for whatever reason,
the link will also
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Tuesday, September 20, 2016 12:46:30 AM Lukas Wunner wrote:
> On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki
> >
> > Currently, there is a problem with taking functional dependencies
> > between into account.
> ^
>devices
>
> > What I mean by a "functional dependency" is when the driver of device
> > B needs device A to be functional and (generally) its driver to be
> > present in order to work properly. This has certain consequences
> > for power management (suspend/resume and runtime PM ordering) and
> > shutdown ordering of these devices. In general, it also 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.
> ^^
> Trailing whitespace.
>
> > Support for representing those functional dependencies between
> > devices is added here to allow the driver core to track them and act
> > on them in certain cases where applicable.
> >
> > The argument for doing that in the driver core is that there are
> > quite a few distinct use cases involving device dependencies, they
> > are relatively hard to get right in a driver (if one wants to
> > address all of them properly) and it only gets worse if multiplied
> > by the number of drivers potentially needing to do it. Morever, at
> > least one case (asynchronous system suspend/resume) cannot be handled
> > in a single driver at all, because it requires the driver of A to
> > wait for B to suspend (during system suspend) and the driver of B to
> > wait for A to resume (during system resume).
> >
> > For this reason, represent dependencies between devices as "links",
> > with the help of struct device_link objects each containing pointers
> > to the "linked" devices, a list node for each of them, status
> > information, flags, a lock and an RCU head for synchronization.
> >
> > Also add two new list heads, links_to_consumers and links_to_suppliers,
> > to struct device to represent the lists of links to the devices that
> > depend on the given one (consumers) and to the devices depended on
> > by it (suppliers), respectively.
> >
> > The entire data structure consisting of all of the lists of link
> > objects for all devices is protected by SRCU (for list walking)
> > and a by mutex (for link object addition/removal). In addition
>
> by a
>
> > to that, each link object has an internal status field whose
> > value reflects what's happening to the devices pointed to by
> > the link. That status field is protected by an internal spinlock.
>
> More precisely, the status field tracks the driver boundness of
> the two devices comprising the link. ("what's happening to the
> devices" is a bit broad, this is really about the *drivers*.)
Basically OK, but it will be more than that most likely due to the bug
reported by Marek.
And thanks for the typo fixes. :-)
> > New links are added by calling device_link_add() which takes four
> > arguments: pointers to the devices in question, the initial status
> > of the link and flags. In particular, if DEVICE_LINK_STATELESS is
> > set in the flags, the link status is not to be taken into account
> > for this link and the driver core will not manage it. In turn, if
> > DEVICE_LINK_AUTOREMOVE is set in the flags, the driver core will
> > remove the link automatically when the consumer device driver
> > unbinds from it.
> >
> > One of the actions carried out by device_link_add() is to reorder
> > the lists used for device shutdown and system suspend/resume to
> > put the consumer device along with all of its children and all of
> > its consumers (and so on, recursively) to the ends of those list
> ^
> s
>
> > in order to ensure the right ordering between all of the supplier
> > and consumer devices.
> >
> > For this reason, it is not possible to create a link between two
> > devices if the would-be supplier device already depends on the
> > would-be consumer device as either a direct descendant of it or a
> > consumer of one of its direct descendants or one of its consumers
> > and so on.
> >
> > It also is impossible to create a link between a parent and a child
> > device (in any direction).
> >
> > There are two types of link objects, persistent and non-persistent.
> > The persistent ones stay around until one of the target devices is
> > deleted, while the non-persistent ones are removed automatically when
> > the consumer driver unbinds from its device (ie. they are assumed to
> > be valid only as long as the consumer device has a driver bound to
> > it). Persistent links are created by default and non-persistent
> > links are created when the DEVICE_LINK_AUTOREMOVE flag is passed
>
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Tue, Sep 20, 2016 at 12:46:30AM +0200, Lukas Wunner wrote:
> On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> > +void device_links_unbind_consumers(struct device *dev)
> > +{
> > + struct device_link *link;
> > + int idx;
> > +
> > + start:
> > + idx = device_links_read_lock();
> > +
> > + list_for_each_entry_rcu(link, &dev->links_to_consumers, s_node) {
> > + enum device_link_status status;
> > +
> > + if (link->flags & DEVICE_LINK_STATELESS)
> > + continue;
> > +
> > + spin_lock(&link->lock);
> > + status = link->status;
> > + if (status == DEVICE_LINK_CONSUMER_PROBE) {
> > + spin_unlock(&link->lock);
> > +
> > + device_links_read_unlock(idx);
> > +
> > + wait_for_device_probe();
> > + goto start;
> > + }
> > + link->status = DEVICE_LINK_SUPPLIER_UNBIND;
> > + if (status == DEVICE_LINK_ACTIVE) {
> > + struct device *consumer = link->consumer;
> > +
> > + get_device(consumer);
>
> As long as the struct device_link exists, a ref is held on the
> supplier and consumer. Why acquire another ref here?
I'm withdrawing this particular comment as I failed to see that
device_links_read_unlock() is called next, so nothing prevents
the device link from being deleted, same for the consumer, thus
the ref needs to be acquired for device_release_driver_internal()
and this portion of Rafael's code seems perfectly correct.
Thanks & sorry for the noise,
Lukas
Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional dependencies tracking support
On Fri, Sep 16, 2016 at 02:33:55PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
> Currently, there is a problem with taking functional dependencies
> between into account.
^
devices
> What I mean by a "functional dependency" is when the driver of device
> B needs device A to be functional and (generally) its driver to be
> present in order to work properly. This has certain consequences
> for power management (suspend/resume and runtime PM ordering) and
> shutdown ordering of these devices. In general, it also 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.
^^
Trailing whitespace.
> Support for representing those functional dependencies between
> devices is added here to allow the driver core to track them and act
> on them in certain cases where applicable.
>
> The argument for doing that in the driver core is that there are
> quite a few distinct use cases involving device dependencies, they
> are relatively hard to get right in a driver (if one wants to
> address all of them properly) and it only gets worse if multiplied
> by the number of drivers potentially needing to do it. Morever, at
> least one case (asynchronous system suspend/resume) cannot be handled
> in a single driver at all, because it requires the driver of A to
> wait for B to suspend (during system suspend) and the driver of B to
> wait for A to resume (during system resume).
>
> For this reason, represent dependencies between devices as "links",
> with the help of struct device_link objects each containing pointers
> to the "linked" devices, a list node for each of them, status
> information, flags, a lock and an RCU head for synchronization.
>
> Also add two new list heads, links_to_consumers and links_to_suppliers,
> to struct device to represent the lists of links to the devices that
> depend on the given one (consumers) and to the devices depended on
> by it (suppliers), respectively.
>
> The entire data structure consisting of all of the lists of link
> objects for all devices is protected by SRCU (for list walking)
> and a by mutex (for link object addition/removal). In addition
by a
> to that, each link object has an internal status field whose
> value reflects what's happening to the devices pointed to by
> the link. That status field is protected by an internal spinlock.
More precisely, the status field tracks the driver boundness of
the two devices comprising the link. ("what's happening to the
devices" is a bit broad, this is really about the *drivers*.)
> New links are added by calling device_link_add() which takes four
> arguments: pointers to the devices in question, the initial status
> of the link and flags. In particular, if DEVICE_LINK_STATELESS is
> set in the flags, the link status is not to be taken into account
> for this link and the driver core will not manage it. In turn, if
> DEVICE_LINK_AUTOREMOVE is set in the flags, the driver core will
> remove the link automatically when the consumer device driver
> unbinds from it.
>
> One of the actions carried out by device_link_add() is to reorder
> the lists used for device shutdown and system suspend/resume to
> put the consumer device along with all of its children and all of
> its consumers (and so on, recursively) to the ends of those list
^
s
> in order to ensure the right ordering between all of the supplier
> and consumer devices.
>
> For this reason, it is not possible to create a link between two
> devices if the would-be supplier device already depends on the
> would-be consumer device as either a direct descendant of it or a
> consumer of one of its direct descendants or one of its consumers
> and so on.
>
> It also is impossible to create a link between a parent and a child
> device (in any direction).
>
> There are two types of link objects, persistent and non-persistent.
> The persistent ones stay around until one of the target devices is
> deleted, while the non-persistent ones are removed automatically when
> the consumer driver unbinds from its device (ie. they are assumed to
> be valid only as long as the consumer device has a driver bound to
> it). Persistent links are created by default and non-persistent
> links are created when the DEVICE_LINK_AUTOREMOVE flag is passed
> to device_link_add().
>
> Both persistent and non-persistent device links can be deleted
> explicitly with the help of device_link_del().
>
> Links created without the DEVICE_LINK_STATELESS flag set are managed
> by the driver core using a simple state machine. There are 5 states
> each link can be in: DORMANT (unused), AVAILABLE (the supplier driver
> is present
