On Mon, May 19, 2014 at 05:40:29PM +0200, Andrzej Pietrasiewicz wrote: > 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 <peter.chen@xxxxxxxxxxxxx> > 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. > I just tried g_ether, I did not meet below error, g_ether works ok. > > > 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. I intended to use udc controler name at the beginning, but it can't work due to udc controller (as device) has already been added at platform bus, it would be a duplicate name at /sys if I use the same name for device at gadget bus. I am afraid we may need to update doc when gadget bus is accepted in future, I will do that. > > 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 for above two is: udc_attach_driver does not register gadget driver on gadget bus, now basic function of configfs works. > 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 -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
