[PATCH] driver core: fix possible missing of device probe

[PATCH] driver core: fix possible missing of device probe

[Ming Lei]

Inside bus_add_driver(), one device might be added into
the bus or probed which is triggered by deferred probe
just after completing of driver_attach() and before
'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
so the device won't be probed by this driver.

This patch moves the below line

	'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'

before driver_attach() inside bus_add_driver().

So fixes the problem since the below way can guarantee that
no probe(dev) may be lost.

CPU0                            	CPU1
driver_register
	...
	write(bus->driver_list)
	smp_mb()
	read(bus->device_list)
	...
					device_add
						/* bus_add_device */
						write(bus->device_list)
						smp_mb()
						/* bus_probe_device*/
						read(bus->driver_list)

And the smp_mb() has been implicit by UNLOCK+LOCK
of 'klist' according to 'VARIETIES OF MEMORY BARRIER' part
of Documentation/memory-barriers.txt.

Reported-and-Tested-by: Russell King <linux@arm.linux.org.uk>
Cc: <stable@vger.kernel.org>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 181ed26..3b5bddb 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -714,12 +714,12 @@ int bus_add_driver(struct device_driver *drv)
 	if (error)
 		goto out_unregister;
 
+	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	if (drv->bus->p->drivers_autoprobe) {
 		error = driver_attach(drv);
 		if (error)
 			goto out_unregister;
 	}
-	klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
 	module_add_driver(drv->owner, drv);
 
 	error = driver_create_file(drv, &driver_attr_uevent);
-- 
1.7.9.5

Re: [PATCH] driver core: fix possible missing of device probe

[anish singh]

Hello Ming,
Though I am not an expert in this driver core area but
I have been following this fix.So have some queries below:

On Fri, Sep 28, 2012 at 6:22 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Inside bus_add_driver(), one device might be added into
should it not be "driver might be added into"?
> the bus or probed which is triggered by deferred probe
> just after completing of driver_attach() and before
> 'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
> so the device won't be probed by this driver.
So the corresponding device will not be probed.
>
> This patch moves the below line
>
>         'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'
>
> before driver_attach() inside bus_add_driver().
>
> So fixes the problem since the below way can guarantee that
> no probe(dev) may be lost.
>
As I understand CPU0 is just calling bus_add_driver and driver_attach
is called and after that it was pre-empted and cpu1 came into picture.
Deferred probe started running on cpu1 and it didn't find the driver present
int the knode_bus and unloaded the driver(why it unloaded is already
explained by russell in his first post).
Hope my understanding is correct.
> CPU0                                    CPU1
> driver_register
>         ...
>         write(bus->driver_list)
>         smp_mb()
>         read(bus->device_list)
>         ...
>                                         device_add
>                                                 /* bus_add_device */
>                                                 write(bus->device_list)
>                                                 smp_mb()
>                                                 /* bus_probe_device*/
>                                                 read(bus->driver_list)
>
> And the smp_mb() has been implicit by UNLOCK+LOCK
> of 'klist' according to 'VARIETIES OF MEMORY BARRIER' part
> of Documentation/memory-barriers.txt.
>
> Reported-and-Tested-by: Russell King <linux@arm.linux.org.uk>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/bus.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 181ed26..3b5bddb 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -714,12 +714,12 @@ int bus_add_driver(struct device_driver *drv)
>         if (error)
>                 goto out_unregister;
>
> +       klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
>         if (drv->bus->p->drivers_autoprobe) {
>                 error = driver_attach(drv);
>                 if (error)
>                         goto out_unregister;
>         }
> -       klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
>         module_add_driver(drv->owner, drv);
>
>         error = driver_create_file(drv, &driver_attr_uevent);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] driver core: fix possible missing of device probe

[Russell King - ARM Linux]

On Fri, Sep 28, 2012 at 09:01:13AM +0530, anish singh wrote:
> Hello Ming,
> Though I am not an expert in this driver core area but
> I have been following this fix.So have some queries below:
> 
> On Fri, Sep 28, 2012 at 6:22 AM, Ming Lei <ming.lei@canonical.com> wrote:
> > Inside bus_add_driver(), one device might be added into
> should it not be "driver might be added into"?
> > the bus or probed which is triggered by deferred probe
> > just after completing of driver_attach() and before
> > 'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
> > so the device won't be probed by this driver.
> So the corresponding device will not be probed.
> >
> > This patch moves the below line
> >
> >         'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'
> >
> > before driver_attach() inside bus_add_driver().
> >
> > So fixes the problem since the below way can guarantee that
> > no probe(dev) may be lost.
> >
> As I understand CPU0 is just calling bus_add_driver and driver_attach
> is called and after that it was pre-empted and cpu1 came into picture.
> Deferred probe started running on cpu1 and it didn't find the driver present
> int the knode_bus and unloaded the driver(why it unloaded is already
> explained by russell in his first post).
> Hope my understanding is correct.
> > CPU0                                    CPU1
> > driver_register
> >         ...
> >         write(bus->driver_list)
> >         smp_mb()
> >         read(bus->device_list)
> >         ...
> >                                         device_add
> >                                                 /* bus_add_device */
> >                                                 write(bus->device_list)
> >                                                 smp_mb()
> >                                                 /* bus_probe_device*/
> >                                                 read(bus->driver_list)

Actually, this description is rubbish.  It's not about the SMP barriers,
or read/write device/driver lists, or about two CPUs (my test setup only
has one CPU.)

It's about threads, and the relative timing of those threads through
the driver model code.  All in all, what with the error path issue, and
now the blatently wrong description, I'm not gaining much confidence in
Ming Lei.

The below is actually what's happening, according to my analysis - and
you'll notice that the device list has absolutely nothing to do with it:

Thread 0			Thread 1			Thread 2
driver_attach()
 bus_for_each_dev()
  __driver_attach(, devA)
   driver_probe_device(, devA)
    really_probe(devA, )
     driver_deferred_probe_add(devA)
				driver_attach()
				 bus_for_each_dev()
				  __driver_attach(, devA)
				   driver_probe_device(, devB)
				    really_probe(devB, )
				     driver_bound(devB)
				      driver_deferred_probe_trigger()
								deferred_probe_work_func()
								 bus_probe_device(devA)
								  device_attach(devA)
								   bus_for_each_drv()
								    __device_attach(, devA)
								     *fails to find driver*
								     *devA dropped from
								      deferred probe list*
klist_add_tail(klist_drivers)

The reason this fix works is because the order in thread 0 means that
when thread 2 comes to re-probe the device, it does find the driver,
and if the resources that the driver needs are still not found (and it
again returns -EPROBE_DEFER) the device will be placed back on the
deferred probe list.

Re: [PATCH] driver core: fix possible missing of device probe

[anish singh]

On Fri, Sep 28, 2012 at 2:16 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Sep 28, 2012 at 09:01:13AM +0530, anish singh wrote:
>> Hello Ming,
>> Though I am not an expert in this driver core area but
>> I have been following this fix.So have some queries below:
>>
>> On Fri, Sep 28, 2012 at 6:22 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> > Inside bus_add_driver(), one device might be added into
>> should it not be "driver might be added into"?
>> > the bus or probed which is triggered by deferred probe
>> > just after completing of driver_attach() and before
>> > 'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
>> > so the device won't be probed by this driver.
>> So the corresponding device will not be probed.
>> >
>> > This patch moves the below line
>> >
>> >         'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'
>> >
>> > before driver_attach() inside bus_add_driver().
>> >
>> > So fixes the problem since the below way can guarantee that
>> > no probe(dev) may be lost.
>> >
>> As I understand CPU0 is just calling bus_add_driver and driver_attach
>> is called and after that it was pre-empted and cpu1 came into picture.
>> Deferred probe started running on cpu1 and it didn't find the driver present
>> int the knode_bus and unloaded the driver(why it unloaded is already
>> explained by russell in his first post).
>> Hope my understanding is correct.
>> > CPU0                                    CPU1
>> > driver_register
>> >         ...
>> >         write(bus->driver_list)
>> >         smp_mb()
>> >         read(bus->device_list)
>> >         ...
>> >                                         device_add
>> >                                                 /* bus_add_device */
>> >                                                 write(bus->device_list)
>> >                                                 smp_mb()
>> >                                                 /* bus_probe_device*/
>> >                                                 read(bus->driver_list)
>
> Actually, this description is rubbish.  It's not about the SMP barriers,
> or read/write device/driver lists, or about two CPUs (my test setup only
> has one CPU.)
>
> It's about threads, and the relative timing of those threads through
> the driver model code.  All in all, what with the error path issue, and
> now the blatently wrong description, I'm not gaining much confidence in
> Ming Lei.
>
> The below is actually what's happening, according to my analysis - and
> you'll notice that the device list has absolutely nothing to do with it:
>
> Thread 0                        Thread 1                        Thread 2
> driver_attach()
>  bus_for_each_dev()
>   __driver_attach(, devA)
>    driver_probe_device(, devA)
>     really_probe(devA, )
>      driver_deferred_probe_add(devA)
>                                 driver_attach()
>                                  bus_for_each_dev()
>                                   __driver_attach(, devA)
>                                    driver_probe_device(, devB)
>                                     really_probe(devB, )
>                                      driver_bound(devB)
>                                       driver_deferred_probe_trigger()
>                                                                 deferred_probe_work_func()
>                                                                  bus_probe_device(devA)
>                                                                   device_attach(devA)
>                                                                    bus_for_each_drv()
>                                                                     __device_attach(, devA)
>                                                                      *fails to find driver*
>                                                                      *devA dropped from
>                                                                       deferred probe list*
> klist_add_tail(klist_drivers)
>
> The reason this fix works is because the order in thread 0 means that
> when thread 2 comes to re-probe the device, it does find the driver,
> and if the resources that the driver needs are still not found (and it
> again returns -EPROBE_DEFER) the device will be placed back on the
> deferred probe list.
Explanation of this problem can't be better than this.Thanks Russell.
I was totally confused by the explanation put forward by Ming.

Re: [PATCH] driver core: fix possible missing of device probe

[Ming Lei]

On Fri, Sep 28, 2012 at 4:46 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> Actually, this description is rubbish.  It's not about the SMP barriers,
> or read/write device/driver lists, or about two CPUs (my test setup only
> has one CPU.)

Could you point out in detail what is wrong with my description?

Looks the description has been posted for about 12 days...

>
> It's about threads, and the relative timing of those threads through

I do not mention threads case in one CPU because the context in
which device_add runs will always see the driver added into
bus's driver list after applying the patch in this situation. And it
is obvious under UP case.

But in SMP case, it is not so obvious like UP, driver_register and
device_add will be run in two individual CPU,  so memory barrier
plays a role in making device_add see the new added driver in
the situation described in the commit log, and avoid the problem.

> the driver model code.  All in all, what with the error path issue, and
> now the blatently wrong description, I'm not gaining much confidence in
> Ming Lei.
>
> The below is actually what's happening, according to my analysis - and
> you'll notice that the device list has absolutely nothing to do with it:

I don't think your analysis is complete for the two reasons:

      - the problem isn't only triggered by deferred probe, and can be triggered
        by device_add too
      - your analysis doesn't cover SMP case

In fact, it is just a race between driver_register and device_add or
bus_probe_device, and should be nothing to do with deferred probe.

See my posted description again:

          Inside bus_add_driver(), one device might be added into
          the bus or probed which is triggered by deferred probe
          just after completing of driver_attach() and before
          'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
          so the device won't be probed by this driver.

>
> Thread 0                        Thread 1                        Thread 2
> driver_attach()
>  bus_for_each_dev()
>   __driver_attach(, devA)
>    driver_probe_device(, devA)
>     really_probe(devA, )
>      driver_deferred_probe_add(devA)
>                                 driver_attach()
>                                  bus_for_each_dev()
>                                   __driver_attach(, devA)
>                                    driver_probe_device(, devB)
>                                     really_probe(devB, )
>                                      driver_bound(devB)
>                                       driver_deferred_probe_trigger()
>                                                                 deferred_probe_work_func()
>                                                                  bus_probe_device(devA)
>                                                                   device_attach(devA)
>                                                                    bus_for_each_drv()
>                                                                     __device_attach(, devA)
>                                                                      *fails to find driver*
>                                                                      *devA dropped from
>                                                                       deferred probe list*
> klist_add_tail(klist_drivers)

Didn't my description cover the description above? ( bus_probe_device is
called between driver_attach and klist_add_tail)

Also it is only one of the two situations addressed by the patch.

>
> The reason this fix works is because the order in thread 0 means that
> when thread 2 comes to re-probe the device, it does find the driver,

You mean the driver_attach on same driver can happen at the same time?
If so, it is simply wrong.

> and if the resources that the driver needs are still not found (and it
> again returns -EPROBE_DEFER) the device will be placed back on the
> deferred probe list.

Thanks,
--
Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[Russell King - ARM Linux]

On Fri, Sep 28, 2012 at 09:42:40PM +0800, Ming Lei wrote:
> On Fri, Sep 28, 2012 at 4:46 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >
> > Actually, this description is rubbish.  It's not about the SMP barriers,
> > or read/write device/driver lists, or about two CPUs (my test setup only
> > has one CPU.)
> 
> Could you point out in detail what is wrong with my description?
> 
> Looks the description has been posted for about 12 days...
> 
> >
> > It's about threads, and the relative timing of those threads through
> 
> I do not mention threads case in one CPU because the context in
> which device_add runs will always see the driver added into

There you go again.  Look at my _much_ better description of the problem
and you'll notice that device_add has nothing to do with this.

> bus's driver list after applying the patch in this situation. And it
> is obvious under UP case.
> 
> But in SMP case, it is not so obvious like UP, driver_register and
> device_add will be run in two individual CPU,  so memory barrier

Again, device_add() has nothing to do with this.  Do you actually
understand the problem?  Plainly as you're bringing device_add() into
this, you do not.

> plays a role in making device_add see the new added driver in
> the situation described in the commit log, and avoid the problem.
> 
> > the driver model code.  All in all, what with the error path issue, and
> > now the blatently wrong description, I'm not gaining much confidence in
> > Ming Lei.
> >
> > The below is actually what's happening, according to my analysis - and
> > you'll notice that the device list has absolutely nothing to do with it:
> 
> I don't think your analysis is complete for the two reasons:
> 
>       - the problem isn't only triggered by deferred probe, and can be triggered
>         by device_add too
>       - your analysis doesn't cover SMP case

Total rubbish.  It does cover the SMP case, because those threads can run
on different CPUs.  The key issue is that there's a race between different
parts of the kernel, and IT IS NOT between the major device list and driver
list as you keep on bringing up.

> In fact, it is just a race between driver_register and device_add or
> bus_probe_device, and should be nothing to do with deferred probe.
> 
> See my posted description again:
> 
>           Inside bus_add_driver(), one device might be added into
>           the bus or probed which is triggered by deferred probe
>           just after completing of driver_attach() and before
>           'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
>           so the device won't be probed by this driver.

This paragraph makes no sense to me as a native English speaker.

> > Thread 0                        Thread 1                        Thread 2
> > driver_attach()
> >  bus_for_each_dev()
> >   __driver_attach(, devA)
> >    driver_probe_device(, devA)
> >     really_probe(devA, )
> >      driver_deferred_probe_add(devA)
> >                                 driver_attach()
> >                                  bus_for_each_dev()
> >                                   __driver_attach(, devA)
> >                                    driver_probe_device(, devB)
> >                                     really_probe(devB, )
> >                                      driver_bound(devB)
> >                                       driver_deferred_probe_trigger()
> >                                                                 deferred_probe_work_func()
> >                                                                  bus_probe_device(devA)
> >                                                                   device_attach(devA)
> >                                                                    bus_for_each_drv()
> >                                                                     __device_attach(, devA)
> >                                                                      *fails to find driver*
> >                                                                      *devA dropped from
> >                                                                       deferred probe list*
> > klist_add_tail(klist_drivers)
> 
> Didn't my description cover the description above? ( bus_probe_device is
> called between driver_attach and klist_add_tail)

Given that your description makes no sense (either to me or, as we've
also found out, to Anish Singh), it means your description is not able
to be understood.  So no, your description does not cover the situation
I reported by the mere fact that it was uninteligable.

> Also it is only one of the two situations addressed by the patch.
> 
> >
> > The reason this fix works is because the order in thread 0 means that
> > when thread 2 comes to re-probe the device, it does find the driver,
> 
> You mean the driver_attach on same driver can happen at the same time?
> If so, it is simply wrong.

Sigh... your lack of understanding is impressive.  I do not mean that
driver_attach() can happen on the same struct device_driver simultaneously.
Go back and look at my description of the three threads.  Do you see two
driver_attach() statements in that?  No.  So clearly I don't mean that.

> > and if the resources that the driver needs are still not found (and it
> > again returns -EPROBE_DEFER) the device will be placed back on the
> > deferred probe list.
> 
> Thanks,

Sorry, I don't think you know what you're talking about, and as such
please remove my reported and tested by from this patch.  I do not wish
to endorse a patch created by someone who seemingly has a total lack of
understanding of the problem I reported.

Thanks.

Re: [PATCH] driver core: fix possible missing of device probe

[Ming Lei]

On Fri, Sep 28, 2012 at 9:55 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>> I do not mention threads case in one CPU because the context in
>> which device_add runs will always see the driver added into
>
> There you go again.  Look at my _much_ better description of the problem
> and you'll notice that device_add has nothing to do with this.

OK, I explain it again:

CPU0						CPU1

driver_register
	...
	bus_add_driver
		driver_attach
						device_add(devb)

		klist_add_tail(klist_drivers)

When device_add(devb) is run just after completion of driver_attach
and before klist_add_tail(klist_drivers), the 'devb' can't be probed
in device_add because the driver hasn't been added into bus,
and it wasn't be probed in driver_attach because driver_attach didn't
see the device in the bus.

So the 'devb' will be missed to be probed in the bus, won't it?


Thanks,
--
Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[Russell King - ARM Linux]

On Fri, Sep 28, 2012 at 10:07:22PM +0800, Ming Lei wrote:
> On Fri, Sep 28, 2012 at 9:55 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> I do not mention threads case in one CPU because the context in
> >> which device_add runs will always see the driver added into
> >
> > There you go again.  Look at my _much_ better description of the problem
> > and you'll notice that device_add has nothing to do with this.
> 
> OK, I explain it again:
> 
> CPU0						CPU1
> 
> driver_register
> 	...
> 	bus_add_driver
> 		driver_attach
> 						device_add(devb)
> 
> 		klist_add_tail(klist_drivers)
> 
> When device_add(devb) is run just after completion of driver_attach
> and before klist_add_tail(klist_drivers), the 'devb' can't be probed
> in device_add because the driver hasn't been added into bus,
> and it wasn't be probed in driver_attach because driver_attach didn't
> see the device in the bus.
> 
> So the 'devb' will be missed to be probed in the bus, won't it?

Wait a moment.  You're describing a *totally* *different* problem to
the problem I reported.  I say - for the third time - that in the
problem I reported, device_add() has NOTHING TO DO WITH IT.

In your previous mail, you complained that my description did not
cover another case.  I throw that back at you and say to you that
_your_ description does _not_ cover my case, but refers to a
_different_ problem which happens to be fixed by the _same_ fix.

To attach my "reported-by" to a problem description which is not
the problem that I reported is bad practice, and actually creates
a lie.  If you wish to keep your problem description, then you must
remove my Reported-by, because the problem you refer to in your
description is not _my_ problem.

Re: [PATCH] driver core: fix possible missing of device probe

[Ming Lei]

On Fri, Sep 28, 2012 at 10:13 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Sep 28, 2012 at 10:07:22PM +0800, Ming Lei wrote:
>> On Fri, Sep 28, 2012 at 9:55 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> >> I do not mention threads case in one CPU because the context in
>> >> which device_add runs will always see the driver added into
>> >
>> > There you go again.  Look at my _much_ better description of the problem
>> > and you'll notice that device_add has nothing to do with this.
>>
>> OK, I explain it again:
>>
>> CPU0                                          CPU1
>>
>> driver_register
>>       ...
>>       bus_add_driver
>>               driver_attach
>>                                               device_add(devb)
>>
>>               klist_add_tail(klist_drivers)
>>
>> When device_add(devb) is run just after completion of driver_attach
>> and before klist_add_tail(klist_drivers), the 'devb' can't be probed
>> in device_add because the driver hasn't been added into bus,
>> and it wasn't be probed in driver_attach because driver_attach didn't
>> see the device in the bus.
>>
>> So the 'devb' will be missed to be probed in the bus, won't it?
>
> Wait a moment.  You're describing a *totally* *different* problem to
> the problem I reported.  I say - for the third time - that in the

I don't see there is any difference between them. In my above description,
it will become same if the 'device_add' is changed to 'bus_probe_device'.

I have mentioned it in my commit log already.

> problem I reported, device_add() has NOTHING TO DO WITH IT.
>
> In your previous mail, you complained that my description did not
> cover another case.  I throw that back at you and say to you that
> _your_ description does _not_ cover my case, but refers to a
> _different_ problem which happens to be fixed by the _same_ fix.

As said above, if 'device_add()' is replaced with 'bus_probe_device()',
that becomes your problem, right?

>
> To attach my "reported-by" to a problem description which is not
> the problem that I reported is bad practice, and actually creates
> a lie.  If you wish to keep your problem description, then you must
> remove my Reported-by, because the problem you refer to in your
> description is not _my_ problem.

I mentioned 'bus_probe_device'  case in my commit log.
If you still think that is not your problem, I can remove the reported-by.


Thanks,
--
Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[Ming Lei]

On Fri, Sep 28, 2012 at 11:31 AM, anish singh
<anish198519851985@gmail.com> wrote:
> Hello Ming,
> Though I am not an expert in this driver core area but
> I have been following this fix.So have some queries below:
>
> On Fri, Sep 28, 2012 at 6:22 AM, Ming Lei <ming.lei@canonical.com> wrote:
>> Inside bus_add_driver(), one device might be added into
> should it not be "driver might be added into"?
>> the bus or probed which is triggered by deferred probe
>> just after completing of driver_attach() and before
>> 'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
>> so the device won't be probed by this driver.
> So the corresponding device will not be probed.
>>
>> This patch moves the below line
>>
>>         'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'
>>
>> before driver_attach() inside bus_add_driver().
>>
>> So fixes the problem since the below way can guarantee that
>> no probe(dev) may be lost.
>>
> As I understand CPU0 is just calling bus_add_driver and driver_attach
> is called and after that it was pre-empted and cpu1 came into picture.
> Deferred probe started running on cpu1 and it didn't find the driver present
> int the knode_bus and unloaded the driver(why it unloaded is already
> explained by russell in his first post).

If the driver is unloaded, it is not a problem because the device will be
probed after the driver is loaded again, so it is nothing to do with driver
unloading. I don't know it was mentioned before.

The  problem is that the bus_probe_device or device_add context
may not see the driver added in driver_register context, in which
the device is not being seen by driver_attach too. Only when the
device is not probed in both the two contexts, the problem is
triggered.

If the device can be guaranteed to be probed always in one of the
two contexts, the problem will be solved. That is the idea in the patch.

> Hope my understanding is correct.

Hope I clarify the problem.


Thanks,
--
Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[anish kumar]

On Fri, 2012-09-28 at 21:53 +0800, Ming Lei wrote:
> On Fri, Sep 28, 2012 at 11:31 AM, anish singh
> <anish198519851985@gmail.com> wrote:
> > Hello Ming,
> > Though I am not an expert in this driver core area but
> > I have been following this fix.So have some queries below:
> >
> > On Fri, Sep 28, 2012 at 6:22 AM, Ming Lei <ming.lei@canonical.com> wrote:
> >> Inside bus_add_driver(), one device might be added into
> > should it not be "driver might be added into"?
> >> the bus or probed which is triggered by deferred probe
> >> just after completing of driver_attach() and before
> >> 'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
> >> so the device won't be probed by this driver.
> > So the corresponding device will not be probed.
> >>
> >> This patch moves the below line
> >>
> >>         'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'
> >>
> >> before driver_attach() inside bus_add_driver().
> >>
> >> So fixes the problem since the below way can guarantee that
> >> no probe(dev) may be lost.
> >>
> > As I understand CPU0 is just calling bus_add_driver and driver_attach
> > is called and after that it was pre-empted and cpu1 came into picture.
> > Deferred probe started running on cpu1 and it didn't find the driver present
> > int the knode_bus and unloaded the driver(why it unloaded is already
> > explained by russell in his first post).
> 
> If the driver is unloaded, it is not a problem because the device will be
> probed after the driver is loaded again, so it is nothing to do with driver
> unloading. I don't know it was mentioned before.
> 
> The  problem is that the bus_probe_device or device_add context
> may not see the driver added in driver_register context, in which
> the device is not being seen by driver_attach too. Only when the
> device is not probed in both the two contexts, the problem is
> triggered.
> 
> If the device can be guaranteed to be probed always in one of the
> two contexts, the problem will be solved. That is the idea in the patch.
> 
> > Hope my understanding is correct.
> 
> Hope I clarify the problem.
Ming,
Somehow I feel here that the problem reported by Russell is totally
different from your understanding of problem.I humbly urge you to have
a look at Russell's nice explanation.
I was easily able to understand the problem faced by him and the
reasoning put forward by him and moreover if the issue had been related
to SMP russell would have definitely mentioned it given his knowledge of
the same.
Thanks for your time.
> 
> 
> Thanks,
> --
> Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[Ming Lei]

On Fri, Sep 28, 2012 at 10:31 PM, anish kumar
<anish198519851985@gmail.com> wrote:

> Ming,
> Somehow I feel here that the problem reported by Russell is totally
> different from your understanding of problem.I humbly urge you to have

I let Russell answer if it is same problem, see my last email.

> a look at Russell's nice explanation.

Sorry, how do you know I didn't look at Russell's explanation?


Thanks,
--
Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[anish kumar]

On Fri, 2012-09-28 at 22:44 +0800, Ming Lei wrote:
> On Fri, Sep 28, 2012 at 10:31 PM, anish kumar
> <anish198519851985@gmail.com> wrote:
> 
> > Ming,
> > Somehow I feel here that the problem reported by Russell is totally
> > different from your understanding of problem.I humbly urge you to have
> 
> I let Russell answer if it is same problem, see my last email.
> 
> > a look at Russell's nice explanation.
> 
> Sorry, how do you know I didn't look at Russell's explanation?
Sorry but I just had a cursory glance at your explanation and looks like
there was some argument between you and russell so thought about
that(shamelessly not even going through the explanation put forward by
you as I understood the problem and solution). Anyway as mentioned
earlier I am no expert on driver core but just wanted to learn the
wonderful driver core design.

Hope I didn't offend you.
> 
> 
> Thanks,
> --
> Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[Russell King - ARM Linux]

On Fri, Sep 28, 2012 at 10:44:13PM +0800, Ming Lei wrote:
> On Fri, Sep 28, 2012 at 10:31 PM, anish kumar
> <anish198519851985@gmail.com> wrote:
> 
> > Ming,
> > Somehow I feel here that the problem reported by Russell is totally
> > different from your understanding of problem.I humbly urge you to have
> 
> I let Russell answer if it is same problem, see my last email.

I'm tired of this thread.  It's really not worth spending any more time
on this crap.  You can't see the problem, and it seems you're unwilling
to listen to the feedback that's being given to you.

Re: [PATCH] driver core: fix possible missing of device probe

[Ming Lei]

On Fri, Sep 28, 2012 at 11:05 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Sep 28, 2012 at 10:44:13PM +0800, Ming Lei wrote:
>> On Fri, Sep 28, 2012 at 10:31 PM, anish kumar
>> <anish198519851985@gmail.com> wrote:
>>
>> > Ming,
>> > Somehow I feel here that the problem reported by Russell is totally
>> > different from your understanding of problem.I humbly urge you to have
>>
>> I let Russell answer if it is same problem, see my last email.
>
> I'm tired of this thread.  It's really not worth spending any more time
> on this crap.  You can't see the problem, and it seems you're unwilling
> to listen to the feedback that's being given to you.

Looks you did not reply my previous question, right? And you just said
it is wrong, and not mention what is wrong about my description.

          > Could you point out in detail what is wrong with my description?

OK, I understand that you think what my patch addressed is not same with
the problem you reported, so I will remove the reported-by.


Thanks,
--
Ming Lei

Re: [PATCH] driver core: fix possible missing of device probe

[Russell King - ARM Linux]

On Fri, Sep 28, 2012 at 08:52:43AM +0800, Ming Lei wrote:
> Inside bus_add_driver(), one device might be added into
> the bus or probed which is triggered by deferred probe
> just after completing of driver_attach() and before
> 'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)',
> so the device won't be probed by this driver.
> 
> This patch moves the below line
> 
> 	'klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers)'
> 
> before driver_attach() inside bus_add_driver().
> 
> So fixes the problem since the below way can guarantee that
> no probe(dev) may be lost.
> 
> CPU0                            	CPU1
> driver_register
> 	...
> 	write(bus->driver_list)
> 	smp_mb()
> 	read(bus->device_list)
> 	...
> 					device_add
> 						/* bus_add_device */
> 						write(bus->device_list)
> 						smp_mb()
> 						/* bus_probe_device*/
> 						read(bus->driver_list)
> 
> And the smp_mb() has been implicit by UNLOCK+LOCK
> of 'klist' according to 'VARIETIES OF MEMORY BARRIER' part
> of Documentation/memory-barriers.txt.
> 
> Reported-and-Tested-by: Russell King <linux@arm.linux.org.uk>

Please use rmk+kernel@arm.linux.org.uk here, thanks.