Return-Path: MIME-Version: 1.0 In-Reply-To: <1241293366.4412.14.camel@localhost.localdomain> References: <49FC2455020000BF000290DA@mailgw2.hevs.ch> <1241293366.4412.14.camel@localhost.localdomain> Date: Sat, 2 May 2009 13:19:40 -0700 Message-ID: Subject: Re: [BUG] 2.6.30-rc4 hid bluetooth not working From: Justin Mattock To: Marcel Holtmann Cc: Dave Young , Marc Pignat , jkosina@suse.cz, ext-roger.quadros@nokia.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 List-ID: On Sat, May 2, 2009 at 12:42 PM, Marcel Holtmann wrot= e: > Hi Dave, > >> >>> > Subject: bluetooth: Fix serialization when adding/deleting connect= ions in hci_sysfs >> >>> > >> >>> > add_conn and del_conn should be serialized, but flush_workqueue ca= n't be used >> >>> > by the worker thread on it's own queue, so use flush_work to seria= lize add_conn >> >>> > and del_conn against each other. >> >>> > >> >>> > Signed-off-by: Marc Pignat >> >>> >> >>> Acked-by: Jiri Kosina >> >>> >> >>> FWIW. >> >> >> >>nak from my side since I think it is the wrong fix. We really wanna wa= it >> >>for all works to finish here. This includes work from other connection >> >>attempts or terminations. >> > >> > IMHO, there is no need to wait for work currently running, since this = is a >> > singlethread workqueue. >> >> Yes, sounds right. >> >> > >> > But it is perhaps simpler to use a lock (mutex or watherver locking pr= imitive). >> >> I'm here a little bit late. Marcel, I'm quite busy recently, I just >> see the commit and then this thread. >> >> Let me explain why I add two workqueue originally, =A0because workqueue >> will be defered, so =A0 we must guarantee "connection deletion" finished >> before "connection adding with same bt addr", =A0or the "connection >> adding" will fail. >> >> On the other hand flush "adding" workqueue in "connection deletion" >> function is not necessary. >> >> To fix this bug, I think we can just use the two work struct for >> add/del, at the same time keeping the original two workqueue. >> >> Please see following patch for this, (building-test only, I have no >> bluetooth device at hand, I can test this the day after tommorrow) > > so I spent the whole day figuring out what is going on here and we keep > making the wrong assumptions over and over again. > > First of all, we only add the sysfs device when we have a successful > connection. And we identify it with the handle. This means that we can > NOT have any name clashes anymore since the controller has to make sure > a handle is only assigned once. Previously we did this on the BD_ADDR > value and that lead to it. That is no longer the case. > > Second of all the two work queues introduces way too much complexity for > a really simple task of adding and removing a sysfs device entry. > > The real problem we have right now are that we are not initializing the > sysfs device when creating the hci_conn. This is just wrong and can lead > to all kinds of weird invalid data access. And as a result the adding of > the sysfs device should only set the name and add it. > > We also check device_registered before making sure that device_add has > been run. And instead of adding more locking or crazy work queue > dependencies, we should use the single thread work queue to ensure the > correct order of things. > > The attached patch introduces a hci_conn_init_sysfs step to make sure we > setup the sysfs device correctly. I left the flush_work calls, but I > think they are not needed since a del_conn before add_conn is no longer > possible now. > > Regards > > Marcel > > well it seems your not the only one with a broken bluetooth(latest git pull this morning) this is what dmesg shows: [ 64.484350] ------------[ cut here ]------------ [ 64.484357] WARNING: at kernel/workqueue.c:371 flush_cpu_workqueue+0x26/0x6b() [ 64.484363] Hardware name: MacBookPro2,2 [ 64.484366] Modules linked in: radeon drm agpgart bnep snd_hda_codec_idt firewire_ohci firewire_core snd_hda_intel snd_hda_codec snd_hwdep snd_pcm ohci1394 snd_timer ath9k ieee1394 sky2 ehci_hcd snd_page_alloc i2c_i801 battery joydev evdev ac video uhci_hcd thermal button hci_uart sco rfcomm btusb hidp l2cap bluetooth ipmi_watchdog ipmi_msghandler uvcvideo isight_firmware uinput arpt_mangle arptable_filter arp_tables nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 iptable_mangle iptable_filter ip_tables x_tables coretemp acpi_cpufreq processor appletouch applesmc [ 64.484473] Pid: 762, comm: bluetooth Not tainted 2.6.30-rc4 #7 [ 64.484477] Call Trace: [ 64.484488] [] warn_slowpath+0x71/0x87 [ 64.484496] [] ? dequeue_entity+0x20/0x206 [ 64.484503] [] ? dequeue_entity+0x20/0x206 [ 64.484511] [] ? dequeue_task_fair+0x57/0x5c [ 64.484520] [] ? dequeue_task+0x12b/0x13e [ 64.484530] [] ? _raw_spin_unlock+0x75/0x7a [ 64.484539] [] ? _spin_unlock_irq+0x8/0x10 [ 64.484547] [] ? finish_task_switch+0x4d/0xa2 [ 64.484555] [] ? __schedule+0x80d/0x87d [ 64.484563] [] flush_cpu_workqueue+0x26/0x6b [ 64.484572] [] ? _raw_spin_unlock+0x75/0x7a [ 64.484579] [] flush_workqueue+0x2b/0x49 [ 64.484602] [] add_conn+0x10/0x34 [bluetooth] [ 64.484609] [] worker_thread+0x13b/0x1b9 [ 64.484628] [] ? add_conn+0x0/0x34 [bluetooth] [ 64.484637] [] ? autoremove_wake_function+0x0/0x2f [ 64.484644] [] ? worker_thread+0x0/0x1b9 [ 64.484652] [] kthread+0x46/0x6a [ 64.484659] [] ? kthread+0x0/0x6a [ 64.484667] [] kernel_thread_helper+0x7/0x10 [ 64.484673] ---[ end trace bd97c7b15860436c ]--- [ 64.643667] BUG: unable to handle kernel NULL pointer dereference at 000= 00020 [ 64.643678] IP: [] sysfs_addrm_start+0x21/0x8f [ 64.643692] *pde =3D 00000000 [ 64.643698] Oops: 0000 [#1] SMP [ 64.643705] last sysfs file: /sys/devices/platform/applesmc.768/light [ 64.643711] Modules linked in: radeon drm agpgart bnep snd_hda_codec_idt firewire_ohci firewire_core snd_hda_intel snd_hda_codec snd_hwdep snd_pcm ohci1394 snd_timer ath9k ieee1394 sky2 ehci_hcd snd_page_alloc i2c_i801 battery joydev evdev ac video uhci_hcd thermal button hci_uart sco rfcomm btusb hidp l2cap bluetooth ipmi_watchdog ipmi_msghandler uvcvideo isight_firmware uinput arpt_mangle arptable_filter arp_tables nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 iptable_mangle iptable_filter ip_tables x_tables coretemp acpi_cpufreq processor appletouch applesmc [ 64.643810] [ 64.643817] Pid: 1584, comm: bluetoothd Tainted: G W (2.6.30-rc4 #7) MacBookPro2,2 [ 64.643823] EIP: 0060:[] EFLAGS: 00010286 CPU: 0 [ 64.643829] EIP is at sysfs_addrm_start+0x21/0x8f [ 64.643834] EAX: f5796000 EBX: 00000000 ECX: 00000000 EDX: c054acc0 [ 64.643840] ESI: f5797c60 EDI: f5797c70 EBP: f5797c54 ESP: f5797c48 [ 64.643845] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068 [ 64.643851] Process bluetoothd (pid: 1584, ti=3Df5796000 task=3Df56c9860 task.ti=3Df5796000) [ 64.643856] Stack: [ 64.643860] f4936058 f5797c60 00000000 f5797c7c c01bedaf fffffff4 00000000 00000000 [ 64.643874] 00000000 00000000 f4ef4d1c f48129b4 f48129b4 f5797c90 c01bee18 f5797c88 [ 64.643889] f5797c90 f4ef4d1c f5797cac c0230e4a ffffffff fffffffe f4ef4d1c f48129b4 [ 64.643906] Call Trace: [ 64.643910] [] ? create_dir+0x3a/0x76 [ 64.643918] [] ? sysfs_create_dir+0x2d/0x3d [ 64.643926] [] ? kobject_add_internal+0xb0/0x15f [ 64.643936] [] ? kobject_add_varg+0x31/0x3d [ 64.643945] [] ? kobject_add+0x43/0x49 [ 64.643952] [] ? device_add+0xd5/0x451 [ 64.643962] [] ? kvasprintf+0x38/0x43 [ 64.643971] [] ? kobject_set_name_vargs+0x46/0x4c [ 64.643979] [] ? hid_add_device+0x12b/0x147 [ 64.643991] [] ? hidp_add_connection+0x2c0/0x539 [hidp] [ 64.644006] [] ? hidp_sock_ioctl+0xe2/0x1e6 [hidp] [ 64.644020] [] ? check_object+0x136/0x190 [ 64.644029] [] ? avc_has_perm+0x3c/0x46 [ 64.644038] [] ? inode_has_perm+0x5b/0x65 [ 64.644048] [] ? sock_ioctl+0x1b9/0x1dd [ 64.644057] [] ? sock_ioctl+0x0/0x1dd [ 64.644064] [] ? vfs_ioctl+0x18/0x71 [ 64.644073] [] ? do_vfs_ioctl+0x461/0x49f [ 64.644082] [] ? file_has_perm+0x7c/0x85 [ 64.644091] [] ? sys_ioctl+0x41/0x61 [ 64.644099] [] ? security_file_ioctl+0x10/0x13 [ 64.644107] [] ? sys_ioctl+0x41/0x61 [ 64.644115] [] ? sysenter_do_call+0x12/0x28 [ 64.644125] Code: 45 f0 8d 65 f4 5b 5e 5f c9 c3 55 b9 04 00 00 00 89 e5 57 89 c7 56 89 c6 53 31 c0 89 d3 f3 ab b8 ac ac 54 c0 89 16 e8 a7 49 22 00 <8b> 53 20 b9 88 e4 1b c0 53 a1 80 0f 68 c0 e8 59 70 fd ff 89 c3 [ 64.644213] EIP: [] sysfs_addrm_start+0x21/0x8f SS:ESP 0068:f5797c48 [ 64.644223] CR2: 0000000000000020 [ 64.644229] ---[ end trace bd97c7b15860436d ]--- --=20 Justin P. Mattock