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

Groups > linux.kernel > #1491769 > unrolled thread

Re: [Update][RFC/RFT][PATCH v3 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: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-27 11:00 +0200
    Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-27 14:00 +0200
      Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-28 12:50 +0200
        Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support "Rafael J. Wysocki" <rafael@kernel.org> - 2016-09-28 13:40 +0200
          Re: [Update][RFC/RFT][PATCH v3 2/5] driver core: Functional  dependencies tracking support Lukas Wunner <lukas@wunner.de> - 2016-09-29 12:40 +0200

#1491769 — 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

[toc] | [next] | [standalone]

#1491856

On Tue, Sep 27, 2016 at 10:54 AM, Lukas Wunner <lukas@wunner.de> 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

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

#1492528

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 <lukas@wunner.de> 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

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

#1492539

On Wed, Sep 28, 2016 at 12:43 PM, Lukas Wunner <lukas@wunner.de> 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 <lukas@wunner.de> 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

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

#1493323

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 <lukas@wunner.de> 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 <lukas@wunner.de> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web