Hi Peter,
W dniu 16.05.2014 11:00, Peter Chen pisze:
Hi Felipe & Alan,
To continue with topic discussed at
http://www.spinics.net/lists/linux-usb/msg105279.html,
I implement the gadget bus to bind udc to gadget driver follow
most ideas from your two.
While the idea is interesting I think some aspects of the implementation
must be thought over.
With the series applied g_ether fails. The offending commit is:
4c3efd6b1e227831c919bc2059d0aa080692f257 is the first bad commit
commit 4c3efd6b1e227831c919bc2059d0aa080692f257
Author: Peter Chen <[email protected]>
Date: Fri May 16 17:00:20 2014 +0800
usb: gadget: core: add implementation of gadget bus
What happens is this:
$ modprobe g_ether
[ 315.671192] using random self ethernet address
[ 315.674393] using random host ethernet address
[ 315.682421] usb0: HOST MAC 82:2a:41:f3:c0:12
[ 315.687382] usb0: MAC d2:64:6a:c1:bc:6f
[ 315.689788] using random self ethernet address
[ 315.701331] using random host ethernet address
[ 315.707344] g_ether gadget: Ethernet Gadget, version: Memorial Day 2008
[ 315.716012] g_ether gadget: g_ether ready
[ 315.730432] s3c-hsotg s3c-hsotg: bound driver g_ether
[ 315.748347] Unable to handle kernel NULL pointer dereference at virtual
address 00000000
[ 315.755008] s3c-hsotg s3c-hsotg: GINTSTS_USBSusp
[ 315.770636] pgd = e750c000
[ 315.771873] [00000000] *pgd=00000000
[ 315.777413] Internal error: Oops: 5 [#1] PREEMPT ARM
[ 315.781040] Modules linked in: usb_f_eem g_ether(+) usb_f_rndis u_ether
libcomposite
[ 315.788758] CPU: 0 PID: 2727 Comm: modprobe Not tainted 3.15.0-rc4+ #371
[ 315.795426] task: e77b5680 ti: e748a000 task.ti: e748a000
[ 315.800809] PC is at module_add_driver+0x48/0xd0
[ 315.805394] LR is at sysfs_do_create_link_sd+0x78/0xc8
[ 315.810505] pc : [<c02a6780>] lr : [<c0152ca0>] psr: 80000013
[ 315.810505] sp : e748bd58 ip : e748bd20 fp : e748bd74
[ 315.821941] r10: 00000000 r9 : bf0264f4 r8 : bf003290
[ 315.827140] r7 : 00000000 r6 : c05cd6e0 r5 : bf006d78 r4 : bf024510
[ 315.833640] r3 : bf024344 r2 : 00000000 r1 : c04b732c r0 : 000000d0
[ 315.840141] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 315.847246] Control: 10c5387d Table: 5750c019 DAC: 00000015
[ 315.852964] Process modprobe (pid: 2727, stack limit = 0xe748a238)
[ 315.859117] Stack: (0xe748bd58 to 0xe748c000)
[ 315.863452] bd40:
00000000 bf024510
[ 315.871601] bd60: e75d6780 c05cd6e0 e748bd9c e748bd78 c0297608 c02a6744
bf024344 e748bd88
[ 315.879746] bd80: bf024510 bf0015a8 bf001f04 bf003270 e748bdb4 e748bda0
c0298b08 c0297508
[ 315.887892] bda0: bf0244c8 bf0015a8 e748bdc4 e748bdb8 c02ca358 c0298a8c
e748bdec e748bdc8
[ 315.896037] bdc0: bf0013a8 c02ca320 00000000 e748bf48 00000001 bf024558
e6a4bfc0 e748a000
[ 315.904183] bde0: e748bdfc e748bdf0 bf026508 bf001308 e748be7c e748be00
c000891c bf026500
[ 315.912329] be00: e748be2c e748be10 c0058a04 c03b8ec0 ffffffff c05b41bc
00000000 bf02454c
[ 315.920475] be20: e748be3c e748be30 c0056bd4 c00589b4 e748be64 e748be40
c0047828 c0056bc8
[ 315.928620] be40: 00000000 e748be50 e748bf48 00000001 bf024558 e748bf48
00000001 bf024558
[ 315.936766] be60: e6a4bfc0 00000001 bf02454c c0077af8 e748bf3c e748be80
c007a560 c0008854
[ 315.944912] be80: bf024558 00007fff c0077e94 00000000 e748bebc eaa34000
00000001 bf024558
[ 315.953057] bea0: 00000000 e748bed4 bf02454c bf024594 e748a000 bf0246ac
ffffffff e748bf04
[ 315.961203] bec0: e748bfa4 e748bed0 c0013d28 c0092660 eaa5b000 00000000
00000000 00000000
[ 315.969348] bee0: 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000
[ 315.977494] bf00: 00000000 00000000 00000000 00000000 20000013 00028318
b6dc9000 b6f68114
[ 315.985640] bf20: 00000080 c000f948 e748a000 00000000 e748bfa4 e748bf40
c007ae34 c0078da0
[ 315.993786] bf40: c0108f00 00000000 eaa34000 00028318 eaa52228 eaa52071
eaa5bc08 000006f8
[ 316.001931] bf60: 00000ab8 00000000 00000000 00000000 0000002b 0000002c
00000015 00000000
[ 316.010077] bf80: 0000000f 00000000 00000000 b6fc0bf8 00040000 b6fc0cf8
00000000 e748bfa8
[ 316.018224] bfa0: c000f6a0 c007ad8c b6fc0bf8 00040000 b6dc9000 00028318
b6f68114 b6dc9000
[ 316.026369] bfc0: b6fc0bf8 00040000 b6fc0cf8 00000080 b6fbe1e8 00028318
b6f68114 00000000
[ 316.034515] bfe0: b6f72134 be955948 b6f5f190 b6ec1db0 60000010 b6dc9000
00000000 00040040
[ 316.042675] [<c02a6780>] (module_add_driver) from [<c0297608>]
(bus_add_driver+0x10c/0x214)
[ 316.050985] [<c0297608>] (bus_add_driver) from [<c0298b08>]
(driver_register+0x88/0x104)
[ 316.059049] [<c0298b08>] (driver_register) from [<c02ca358>]
(usb_gadget_probe_driver+0x44/0x54)
[ 316.067826] [<c02ca358>] (usb_gadget_probe_driver) from [<bf0013a8>]
(usb_composite_probe+0xac/0xd8 [libcomposite])
[ 316.078216] [<bf0013a8>] (usb_composite_probe [libcomposite]) from
[<bf026508>] (init+0x14/0x1c [g_ether])
[ 316.087819] [<bf026508>] (init [g_ether]) from [<c000891c>]
(do_one_initcall+0xd4/0x188)
[ 316.095879] [<c000891c>] (do_one_initcall) from [<c007a560>]
(load_module+0x17cc/0x1fec)
[ 316.103933] [<c007a560>] (load_module) from [<c007ae34>]
(SyS_init_module+0xb4/0x120)
[ 316.111732] [<c007ae34>] (SyS_init_module) from [<c000f6a0>]
(ret_fast_syscall+0x0/0x48)
[ 316.119786] Code: e5942004 e3a000d0 e59f107c e5943000 (e5922000)
[ 316.125906] s3c-hsotg s3c-hsotg: new device is high-speed
[ 316.131239] s3c-hsotg s3c-hsotg: s3c_hsotg_irq: USBRst
A simplified trace is like this:
usb_gadget_probe_driver()=>driver_register()=>bus_add_driver()=>driver_attach()=>__driver_attach()
=>driver_probe_device()=>really_probe().
In really_probe(), before line 302 "dev->bus->probe(dev);", the drv->bus is
non-NULL,
but after that line the drv->bus is NULL.
Then really_probe() ends, driver_probe_device() ends, __driver_attach() ends,
driver_attach() ends and we are back in bus_add_driver():
bus_add_driver()=>module_add_driver()=>make_driver_name() and we end up
dereferencing
drv->bus which is NULL.
With this patch applied it is also impossible to compose a similar gadget with
configfs.
Fortunately there is no NULL pointer dereference; just -ENODEV happens.
Let me show you an example sequence of shell commands to set up a gadget which
contains one configuration and provides ECM Ethernet gadget:
$ modprobe libcomposite
$ mount none cfg -t configfs
$ mkdir cfg/usb_gadget/g1
$ cd cfg/usb_gadget/g1
$ mkdir configs/c.1
$ mkdir functions/ecm.usb0
$ mkdir strings/0x409
$ mkdir configs/c.1/strings/0x409
$ echo 0xa4a1 > idProduct
$ echo 0x04e8 > idVendor
$ echo some_serial > strings/0x409/serialnumber
$ echo some_name > strings/0x409/manufacturer
$ echo ECM Gadget > strings/0x409/product
$ echo "Conf 1" > configs/c.1/strings/0x409/configuration
$ echo 120 > configs/c.1/MaxPower
$ ln -s functions/ecm.usb0 configs/c.1
$ echo s3c-hsotg > UDC # THIS FAILS WITH -ENODEV
A simplified trace:
gadget_dev_desc_UDC_store()=>udc_attach_driver()=>bus_find_device().
The latter returns NULL, so the branch "goto out" is taken and as a result
the gadget_dev_desc_UDC_store() fails.
If I used "udc-0" instead of "s3c-hsotg" I did bind such a gadget,
so you seem to have silently changed the configfs interface. I am not saying
the change is bad or good, I just want to let you know about some not-so-obvious
consequences of it.
But while unbinding:
$ echo > UDC
I got this:
[ 82.131606] ------------[ cut here ]------------
[ 82.134814] WARNING: CPU: 0 PID: 2716 at drivers/base/driver.c:190
driver_unregister+0x4c/0x58()
[ 82.143523] Unexpected driver unregister!
[ 82.147505] Modules linked in: usb_f_ecm u_ether libcomposite
[ 82.153215] CPU: 0 PID: 2716 Comm: bash Not tainted 3.15.0-rc4+ #372
[ 82.159583] [<c00163ac>] (unwind_backtrace) from [<c0013224>]
(show_stack+0x20/0x24)
[ 82.167284] [<c0013224>] (show_stack) from [<c03b4014>]
(dump_stack+0x20/0x28)
[ 82.174478] [<c03b4014>] (dump_stack) from [<c0024b20>]
(warn_slowpath_common+0x78/0x98)
[ 82.182530] [<c0024b20>] (warn_slowpath_common) from [<c0024bfc>]
(warn_slowpath_fmt+0x40/0x48)
[ 82.191195] [<c0024bfc>] (warn_slowpath_fmt) from [<c0298bf0>]
(driver_unregister+0x4c/0x58)
[ 82.199607] [<c0298bf0>] (driver_unregister) from [<c02ca274>]
(usb_gadget_unregister_driver+0x38/0x54)
[ 82.208994] [<c02ca274>] (usb_gadget_unregister_driver) from [<bf004cfc>]
(unregister_gadget+0x2c/0x50 [libcomposite])
[ 82.219644] [<bf004cfc>] (unregister_gadget [libcomposite]) from
[<bf004dac>] (gadget_dev_desc_UDC_store+0x8c/0xcc [libcomposite])
[ 82.231338] [<bf004dac>] (gadget_dev_desc_UDC_store [libcomposite]) from
[<bf00378c>] (gadget_info_attr_store+0x2c/0x38 [libcompo)
[ 82.243462] [<bf00378c>] (gadget_info_attr_store [libcomposite]) from
[<c0153edc>] (configfs_write_file+0x16c/0x188)
[ 82.253938] [<c0153edc>] (configfs_write_file) from [<c00eeb0c>]
(vfs_write+0xb8/0x190)
[ 82.261905] [<c00eeb0c>] (vfs_write) from [<c00eef04>] (SyS_write+0x4c/0x98)
[ 82.268924] [<c00eef04>] (SyS_write) from [<c000f6a0>]
(ret_fast_syscall+0x0/0x48)
[ 82.276454] ---[ end trace d75d7ef5ef4ad20d ]---
Also, if a function symlink is removed from a config's directory after the
gadget is unbound:
$ cd cfg/usb_gadget/g1/configs/c.1
$ rm ecm.usb0
I get this:
[ 111.384399] ------------[ cut here ]------------
[ 111.387703] WARNING: CPU: 0 PID: 2745 at drivers/usb/gadget/configfs.c:443
config_usb_cfg_unlink+0xdc/0xfc [libcomposite]()
[ 111.398696] Unable to locate function to unbind
[ 111.403182] Modules linked in: usb_f_ecm u_ether libcomposite
[ 111.408878] CPU: 0 PID: 2745 Comm: rm Tainted: G W 3.15.0-rc4+
#372
[ 111.416118] [<c00163ac>] (unwind_backtrace) from [<c0013224>]
(show_stack+0x20/0x24)
[ 111.423814] [<c0013224>] (show_stack) from [<c03b4014>]
(dump_stack+0x20/0x28)
[ 111.431009] [<c03b4014>] (dump_stack) from [<c0024b20>]
(warn_slowpath_common+0x78/0x98)
[ 111.439060] [<c0024b20>] (warn_slowpath_common) from [<c0024bfc>]
(warn_slowpath_fmt+0x40/0x48)
[ 111.447742] [<c0024bfc>] (warn_slowpath_fmt) from [<bf004ee4>]
(config_usb_cfg_unlink+0xdc/0xfc [libcomposite])
[ 111.457796] [<bf004ee4>] (config_usb_cfg_unlink [libcomposite]) from
[<c01566dc>] (configfs_unlink+0x110/0x1a0)
[ 111.467834] [<c01566dc>] (configfs_unlink) from [<c00fabf8>]
(vfs_unlink+0xcc/0x14c)
[ 111.475540] [<c00fabf8>] (vfs_unlink) from [<c00fae5c>]
(do_unlinkat+0x1e4/0x20c)
[ 111.482992] [<c00fae5c>] (do_unlinkat) from [<c00fd19c>]
(SyS_unlinkat+0x28/0x3c)
[ 111.490496] [<c00fd19c>] (SyS_unlinkat) from [<c000f6a0>]
(ret_fast_syscall+0x0/0x48)
[ 111.498240] ---[ end trace 56abac89912a4563 ]---
The reason is that probably configfs_composite_unbind() is not called at
gadget's unbind.
All in all, it seems that the series is missing integration with configfs
interface.
AP
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
