Re: [BUG] Deferred probing in driver model is racy, resulting in lost probes

[Joachim Eastwood]

Hi,

On Thu, Sep 27, 2012 at 4:03 PM, Russell King - ARM Linux
<linux@xxxxxxxxxxxxxxxx> wrote:
> On Thu, Sep 27, 2012 at 02:58:09PM +0100, Russell King - ARM Linux wrote:
>> On Thu, Sep 27, 2012 at 07:47:46AM +0800, Ming Lei wrote:
>> > On Thu, Sep 27, 2012 at 4:23 AM, Russell King - ARM Linux
>> > <linux@xxxxxxxxxxxxxxxx> wrote:
>> > > To be honest, I've not bothered to test the above patch, and now when I
>> > > look at it, I notice it's broken - in that on error it will corrupt the
>> > > driver list.  Take a look at the error path.
>> > >
>> > > priv is drv->p.  We add priv->knode_bus to the driver list.  If
>> > > driver_attach() returns an error, then we go to out_unregister, which
>> >
>> > In fact, driver_attach() always returns zero, so it does __not__ affect
>> > your test.
>>
>> It may not affect my test, but the fact is, that patch lays down the
>> foundations for bugs to be introduced.  Now, while I can test it (and
>> have done) I don't think it's suitable for mainline because of _that_.
>>
>> If driver_attach() always returns zero, there's no point in it returning
>> a value - it might as well return void and stop leading people to
>> believe that it might return an error.  So I suggest this alternative
>> patch instead, which gets rid of what is effectively dead code, and
>> probably a few warnings about not checking the return value from a
>> __must_check function (see usb-serial.c).
>>
>> With this patch, no one is lead into a false sense of security that,
>> after your patch, if driver_attach() ever returns an error, proper
>> cleanup will happen - it's blatently obvious to anyone modifying it
>> that if they do want it to return an error, they have to audit all the
>> users of it, which IMHO is a good thing.
>
> Sorry, old version of that patch.  Here's the right one.

<snip patch>

I am also having problems with ASoC modules and deferred probing. This
patch seems to improve the a lot situation, though. Now it work most
of the time, but occasionally I get the warning below.

Not sure if this is directly related to the issue Russell have been
seeing. Guess it can be races in the Atmel ASoC drivers as well(?).

[   12.230000] mpa1600-audio mpa1600-audio.0: CODEC wm8776.0-001b not registered
[   12.390000] mpa1600-audio mpa1600-audio.0: snd_soc_register_card()
failed: -517
[   12.390000] platform mpa1600-audio.0: Driver mpa1600-audio requests
probe deferral
[   12.720000] ------------[ cut here ]------------
[   12.720000] WARNING: at fs/sysfs/dir.c:536 sysfs_add_one+0x7c/0xa0()
[   12.740000] mpa1600-audio mpa1600-audio.1: CODEC wm8776.0-001a not registered
[   12.910000] mpa1600-audio mpa1600-audio.1: snd_soc_register_card()
failed: -517
[   13.140000] sysfs: cannot create duplicate filename
'/devices/platform/ssc.0/atmel-ssc-dai.0'
[   13.140000] Modules linked in: snd_soc_mpa1600_wm8776(+)
snd_soc_atmel_ssc_dai snd_soc_core pcmcia regmap_i2c snd_pcm
snd_page_alloc ad525x_dpot_i2c snd_timer ad525x_dpot ohci_hcd
leds_pca9532 snd usbcore soundcore gpio_keys rege
[   13.630000] Backtrace:
[   13.630000] [<c000c46c>] (dump_backtrace+0x0/0x10c) from
[<c021ac48>] (dump_stack+0x18/0x1c)
[   13.980000]  r7:c3839ca8 r6:c00d0cf0 r5:c0278f34 r4:00000218
[   13.990000] [<c021ac30>] (dump_stack+0x0/0x1c) from [<c0015ad0>]
(warn_slowpath_common+0x54/0x6c)
[   14.240000] [<c0015a7c>] (warn_slowpath_common+0x0/0x6c) from
[<c0015b8c>] (warn_slowpath_fmt+0x38/0x40)
[   14.470000]  r8:c2d41110 r7:ffffffef r6:c3839ce8 r5:c3a47780 r4:c2c58000
[   14.480000] [<c0015b54>] (warn_slowpath_fmt+0x0/0x40) from
[<c00d0cf0>] (sysfs_add_one+0x7c/0xa0)
[   14.660000]  r3:c2c58000 r2:c0278f64
[   14.660000] [<c00d0c74>] (sysfs_add_one+0x0/0xa0) from [<c00d1898>]
(create_dir+0x6c/0xc0)
[   14.790000]  r7:00000000 r6:00000000 r5:c3a47780 r4:c3839ce8
[   14.790000] [<c00d182c>] (create_dir+0x0/0xc0) from [<c00d19c4>]
(sysfs_create_dir+0xd8/0xf0)
[   14.850000] [<c00d18ec>] (sysfs_create_dir+0x0/0xf0) from
[<c01252f8>] (kobject_add_internal+0xe4/0x1e0)
[   14.870000]  r7:c2d41b00 r6:c02cc2e0 r5:00000000 r4:c2d41110
[   14.880000] [<c0125214>] (kobject_add_internal+0x0/0x1e0) from
[<c0125644>] (kobject_add_varg+0x44/0x54)
[   14.890000] [<c0125600>] (kobject_add_varg+0x0/0x54) from
[<c01256dc>] (kobject_add+0x4c/0x58)
[   14.900000]  r6:00000000 r5:c2d41108 r4:c2d41100
[   14.910000] [<c0125690>] (kobject_add+0x0/0x58) from [<c015220c>]
(device_add+0xe4/0x5d0)
[   14.920000]  r3:c3839db4 r2:00000000
[   14.920000] [<c0152128>] (device_add+0x0/0x5d0) from [<c01561a8>]
(platform_device_add+0x10c/0x164)
[   14.930000] [<c015609c>] (platform_device_add+0x0/0x164) from
[<bf11a9f4>] (atmel_ssc_set_audio+0xac/0xdc [snd_soc_atmel_ssc_dai])
[   14.940000]  r7:c2d41b00 r6:00000000 r5:c2d41100 r4:c02ccf90
[   14.950000] [<bf11a948>] (atmel_ssc_set_audio+0x0/0xdc
[snd_soc_atmel_ssc_dai]) from [<bf11f038>]
(mpa1600_audio_probe+0x38/0x88 [snd_soc_mpa1600_wm8776])
[   14.960000]  r7:bf11f464 r6:c02ccf98 r5:bf11f4a0 r4:c02ccf90
[   14.960000] [<bf11f000>] (mpa1600_audio_probe+0x0/0x88
[snd_soc_mpa1600_wm8776]) from [<c0155b6c>]
(platform_drv_probe+0x20/0x24)
[   14.990000]  r6:c02ccf98 r5:c02ccf98 r4:bf11f464
[   14.990000] [<c0155b4c>] (platform_drv_probe+0x0/0x24) from
[<c0154680>] (driver_probe_device+0xb8/0x20c)
[   15.000000] [<c01545c8>] (driver_probe_device+0x0/0x20c) from
[<c01548a4>] (__device_attach+0x48/0x4c)
[   15.010000]  r7:c3839ea8 r6:c02ccf98 r5:c02ccf98 r4:bf11f464
[   15.020000] [<c015485c>] (__device_attach+0x0/0x4c) from
[<c0152dc0>] (bus_for_each_drv+0x5c/0x9c)
[   15.030000]  r5:c015485c r4:00000000
[   15.030000] [<c0152d64>] (bus_for_each_drv+0x0/0x9c) from
[<c015493c>] (device_attach+0x6c/0x8c)
[   15.040000]  r7:00000089 r6:c02ccfcc r5:c02d7518 r4:c02ccf98
[   15.050000] [<c01548d0>] (device_attach+0x0/0x8c) from [<c0153988>]
(bus_probe_device+0x34/0xa4)
[   15.060000]  r6:c02ccf98 r5:c02d7518 r4:c02ccf98
[   15.060000] [<c0153954>] (bus_probe_device+0x0/0xa4) from
[<c0154240>] (deferred_probe_work_func+0x60/0x94)
[   15.070000]  r6:c02e3750 r5:c02d7400 r4:c02ccf98
[   15.080000] [<c01541e0>] (deferred_probe_work_func+0x0/0x94) from
[<c002c338>] (process_one_work+0x310/0x4a0)
[   15.090000]  r5:c3804500 r4:c02d7408
[   15.100000] [<c002c028>] (process_one_work+0x0/0x4a0) from
[<c002cbc8>] (worker_thread+0x3b8/0x5e0)
[   15.110000] [<c002c810>] (worker_thread+0x0/0x5e0) from
[<c00336e4>] (kthread+0x8c/0x98)
[   15.110000] [<c0033658>] (kthread+0x0/0x98) from [<c001b4d4>]
(do_exit+0x0/0x720)
[   15.120000]  r7:00000013 r6:c001b4d4 r5:c0033658 r4:c3827ecc
[   15.130000] ---[ end trace d7472237284e57c2 ]---
[   15.130000] platform mpa1600-audio.1: Driver mpa1600-audio requests
probe deferral
[   15.150000] ------------[ cut here ]------------
[   15.150000] WARNING: at lib/kobject.c:196 kobject_add_internal+0x17c/0x1e0()
[   15.250000] kobject_add_internal failed for atmel-ssc-dai.0 with
-EEXIST, don't try to register things with the same name in the same
directory.
[   15.390000] Modules linked in: snd_soc_mpa1600_wm8776
snd_soc_atmel_ssc_dai snd_soc_core pcmcia regmap_i2c snd_pcm
snd_page_alloc ad525x_dpot_i2c snd_timer ad525x_dpot ohci_hcd
leds_pca9532 snd usbcore soundcore gpio_keys regmape
[   15.470000] Backtrace:
[   15.470000] [<c000c46c>] (dump_backtrace+0x0/0x10c) from
[<c021ac48>] (dump_stack+0x18/0x1c)
[   15.520000]  r7:c3839d28 r6:c0125390 r5:c027f187 r4:000000c4
[   15.520000] [<c021ac30>] (dump_stack+0x0/0x1c) from [<c0015ad0>]
(warn_slowpath_common+0x54/0x6c)
[   15.620000] [<c0015a7c>] (warn_slowpath_common+0x0/0x6c) from
[<c0015b8c>] (warn_slowpath_fmt+0x38/0x40)
[   15.640000]  r8:ffffffef r7:c2d41b00 r6:c02cc2e0 r5:ffffffef r4:c2d41110
[   15.650000] [<c0015b54>] (warn_slowpath_fmt+0x0/0x40) from
[<c0125390>] (kobject_add_internal+0x17c/0x1e0)
[   15.670000]  r3:c02247bc r2:c027f21f
[   15.680000] [<c0125214>] (kobject_add_internal+0x0/0x1e0) from
[<c0125644>] (kobject_add_varg+0x44/0x54)
[   15.700000] [<c0125600>] (kobject_add_varg+0x0/0x54) from
[<c01256dc>] (kobject_add+0x4c/0x58)
[   15.720000]  r6:00000000 r5:c2d41108 r4:c2d41100
[   15.720000] [<c0125690>] (kobject_add+0x0/0x58) from [<c015220c>]
(device_add+0xe4/0x5d0)
[   15.740000]  r3:c3839db4 r2:00000000
[   15.760000] [<c0152128>] (device_add+0x0/0x5d0) from [<c01561a8>]
(platform_device_add+0x10c/0x164)
[   15.760000] [<c015609c>] (platform_device_add+0x0/0x164) from
[<bf11a9f4>] (atmel_ssc_set_audio+0xac/0xdc [snd_soc_atmel_ssc_dai])
[   15.790000]  r7:c2d41b00 r6:00000000 r5:c2d41100 r4:c02ccf90
[   15.810000] [<bf11a948>] (atmel_ssc_set_audio+0x0/0xdc
[snd_soc_atmel_ssc_dai]) from [<bf11f038>]
(mpa1600_audio_probe+0x38/0x88 [snd_soc_mpa1600_wm8776])
[   15.840000]  r7:bf11f464 r6:c02ccf98 r5:bf11f4a0 r4:c02ccf90
[   15.840000] [<bf11f000>] (mpa1600_audio_probe+0x0/0x88
[snd_soc_mpa1600_wm8776]) from [<c0155b6c>]
(platform_drv_probe+0x20/0x24)
[   15.860000]  r6:c02ccf98 r5:c02ccf98 r4:bf11f464
[   15.880000] [<c0155b4c>] (platform_drv_probe+0x0/0x24) from
[<c0154680>] (driver_probe_device+0xb8/0x20c)
[   15.900000] [<c01545c8>] (driver_probe_device+0x0/0x20c) from
[<c01548a4>] (__device_attach+0x48/0x4c)
[   15.910000]  r7:c3839ea8 r6:c02ccf98 r5:c02ccf98 r4:bf11f464
[   15.920000] [<c015485c>] (__device_attach+0x0/0x4c) from
[<c0152dc0>] (bus_for_each_drv+0x5c/0x9c)
[   15.940000]  r5:c015485c r4:00000000
[   15.950000] [<c0152d64>] (bus_for_each_drv+0x0/0x9c) from
[<c015493c>] (device_attach+0x6c/0x8c)
[   15.960000]  r7:00000089 r6:c02ccfcc r5:c02d7518 r4:c02ccf98
[   15.970000] [<c01548d0>] (device_attach+0x0/0x8c) from [<c0153988>]
(bus_probe_device+0x34/0xa4)
[   16.000000]  r6:c02ccf98 r5:c02d7518 r4:c02ccf98
[   16.030000] [<c0153954>] (bus_probe_device+0x0/0xa4) from
[<c0154240>] (deferred_probe_work_func+0x60/0x94)
[   16.050000]  r6:c02e3750 r5:c02d7400 r4:c02ccf98
[   16.070000] [<c01541e0>] (deferred_probe_work_func+0x0/0x94) from
[<c002c338>] (process_one_work+0x310/0x4a0)
[   16.100000]  r5:c3804500 r4:c02d7408
[   16.100000] [<c002c028>] (process_one_work+0x0/0x4a0) from
[<c002cbc8>] (worker_thread+0x3b8/0x5e0)
[   16.120000] [<c002c810>] (worker_thread+0x0/0x5e0) from
[<c00336e4>] (kthread+0x8c/0x98)
[   16.140000] [<c0033658>] (kthread+0x0/0x98) from [<c001b4d4>]
(do_exit+0x0/0x720)
[   16.150000]  r7:00000013 r6:c001b4d4 r5:c0033658 r4:c3827ecc
[   16.160000] ---[ end trace d7472237284e57c3 ]---
[   16.160000] mpa1600-audio mpa1600-audio.0: Failed to set SSC for audio: -17
[   16.180000] mpa1600-audio: probe of mpa1600-audio.0 failed with error -17

regards
Joachim Eastwood
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/