Skip to contentSkip to conversation

bluetooth/bluecard_cs: remove

PATCH In review Nishanth Aravamudan 4 participants 7 messages Feb 2, 2005 – Feb 18, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 19/20 bluetooth/bluecard_cs: remove

Hello,

Please consider applying.

Description: Remove deprecated interruptible_sleep_on_timeout() function calls and replace with direct wait-queue usage. Some naming conflicts occurred, which I tried to resolve appropriately. Patch is compile-tested.

drivers/bluetooth/bluecard_cs.c +9 -6
@@ -35,6 +35,7 @@
 #include <linux/ioport.h>
 #include <linux/spinlock.h>
 #include <linux/moduleparam.h>
+#include <linux/wait.h>
 
 #include <linux/skbuff.h>
 #include <asm/io.h>
@@ -289,8 +290,8 @@ static void bluecard_write_wakeup(blueca
 		clear_bit(ready_bit, &(info->tx_state));
 
 		if (skb->pkt_type & 0x80) {
-
-			wait_queue_head_t wait;
+			DEFINE_WAIT(wait);
+			wait_queue_head_t wq;
 			unsigned char baud_reg;
 
 			switch (skb->pkt_type) {
@@ -311,8 +312,9 @@ static void bluecard_write_wakeup(blueca
 			}
 
 			/* Wait until the command reaches the baseband */
-			init_waitqueue_head(&wait);
-			interruptible_sleep_on_timeout(&wait, HZ / 10);
+			prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
+			schedule_timeout(HZ/10);
+			finish_wait(&wq, &wait);
 
 			/* Set baud on baseband */
 			info->ctrl_reg &= ~0x03;
@@ -324,8 +326,9 @@ static void bluecard_write_wakeup(blueca
 			outb(info->ctrl_reg, iobase + REG_CONTROL);
 
 			/* Wait before the next HCI packet can be send */
-			interruptible_sleep_on_timeout(&wait, HZ);
-
+			prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE);
+			schedule_timeout(HZ);
+			finish_wait(&wq, &wait);
 		}
 
 		if (len == skb->len) {

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

maintainer
#1 lore raw

Hi Nish,

Quoted 5 lines

Please consider applying.

Description: Remove deprecated interruptible_sleep_on_timeout() function calls and replace with direct wait-queue usage. Some naming conflicts occurred, which I tried to resolve appropriately. Patch is compile-tested.

you introduce more code than you remove. Is our code a special case or why don't exists a simple API function as replacement?

Regards

Marcel

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#2 lore raw
Quoted 10 lines · Marcel Holtmann

Hi Nish,

Quoted 5 lines

Please consider applying.

Description: Remove deprecated interruptible_sleep_on_timeout() function calls and replace with direct wait-queue usage. Some naming conflicts occurred, which I tried to resolve appropriately. Patch is compile-tested.

you introduce more code than you remove. Is our code a special case or why don't exists a simple API function as replacement?

Yes, I do. *sleep_on*() was the API, which has been replaced with wait_event*(). However, wait_event*() explicitly needs a condition (the "event"), which in thise case does not exist. If I had more background, I would have gone the wait_event*() route. I am looking into a better API to mask away all of the DEFINE_WAIT(), prepare_to_wait(), schedule()/schedule_timeout(), finish_wait() additions (the KJ list can attest to how many of these replacements I'm doing), but there simply does not exist such an API right now.

Any input you could provide, though, would be greatly appreciated.

-Nish

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

maintainer
#3 lore raw

Hi Nish,

Quoted 17 lines
Quoted 8 lines
Quoted 5 lines

Please consider applying.

Description: Remove deprecated interruptible_sleep_on_timeout() function calls and replace with direct wait-queue usage. Some naming conflicts occurred, which I tried to resolve appropriately. Patch is compile-tested.

you introduce more code than you remove. Is our code a special case or why don't exists a simple API function as replacement?

Yes, I do. *sleep_on*() was the API, which has been replaced with wait_event*(). However, wait_event*() explicitly needs a condition (the "event"), which in thise case does not exist. If I had more background, I would have gone the wait_event*() route. I am looking into a better API to mask away all of the DEFINE_WAIT(), prepare_to_wait(), schedule()/schedule_timeout(), finish_wait() additions (the KJ list can attest to how many of these replacements I'm doing), but there simply does not exist such an API right now.

Quoted 1 line

Any input you could provide, though, would be greatly appreciated.

I gave your patch a shot and the oops below happens on a 2.6.11-rc3 after plugging in the card. Without the patch it is working.

Regards

Marcel

Unable to handle kernel NULL pointer dereference at virtual address 00000004
 printing eip:
c0130b72
*pde = 00000000
Oops: 0002 [#1]
PREEMPT 
Modules linked in: bluecard_cs twofish rfcomm hidp l2cap pcmcia binfmt_misc md5 ipv6 capi kernelcapi xfrm_user xfrm4_tunnel ipcomp esp4 ah4 af_key ehci_hcd hci_usb bluetooth ohci_hcd usbcore i2c_ali1535 yenta_socket rsrc_nonstatic pcmcia_core ath_pci ath_rate_onoe wlan ath_hal 8250_pci 8250 serial_core snd_ali5451 snd_ac97_codec snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd soundcore snd_page_alloc evdev vfat fat nls_base i2c_sensor i2c_core radeon drm agpgart tg3 unix
CPU:    0
EIP:    0060:[<c0130b72>]    Tainted: P      VLI
EFLAGS: 00010046   (2.6.11-rc3) 

EIP is at prepare_to_wait+0x32/0xa0
eax: 00000000 ebx: d42f3ea0 ecx: 00000001 edx: d42f3eac
esi: d42f3e80 edi: 00000246 ebp: 00000001 esp: d42f3e4c
ds: 007b es: 007b ss: 0068
Process hcid (pid: 7206, threadinfo=d42f2000 task=d3bc7560)

Stack: dda0eb00 dc4f6840 c0130d10 00000005 ded652ee 00000340 00000000 dac21008 
       00000005 02000150 dc4f68fc 01000292 00000340 00000000 d3bc7560 c0130d10 
       d42f3eac d42f3eac 000000d0 00000150 c0148da0 00000000 d3bc7560 c0130d10 
Call Trace:
 [<c0130d10>] autoremove_wake_function+0x0/0x60
 [<ded652ee>] bluecard_write_wakeup+0x20e/0x2d0 [bluecard_cs]
 [<c0130d10>] autoremove_wake_function+0x0/0x60
 [<c0148da0>] do_wp_page+0x1d0/0x340
 [<c0130d10>] autoremove_wake_function+0x0/0x60
 [<c0258607>] alloc_skb+0x47/0xf0
 [<ded65a45>] bluecard_hci_set_baud_rate+0xe5/0x160 [bluecard_cs]
 [<ded65b53>] bluecard_hci_open+0x73/0x80 [bluecard_cs]
 [<dece72e1>] hci_dev_open+0x51/0x240 [bluetooth]
 [<c0254b59>] sock_ioctl+0xd9/0x260
 [<c016cfce>] do_ioctl+0x8e/0xa0
 [<c016d215>] vfs_ioctl+0x65/0x1f0
 [<c016d407>] sys_ioctl+0x67/0x90
 [<c01031af>] syscall_call+0x7/0xb
Code: d3 89 74 24 04 89 c6 89 6c 24 0c 89 cd 89 7c 24 08 83 22 fe 9c 5f fa b8 01 00 00 00 e8 68 71 fe ff 8d 53 0c 39 53 0c 75 0d 8b 06 <89> 50 04 89 43 0c 89 72 04 89 16 8b 43 04 85 c0 74 0b b8 00 e0 
 <6>note: hcid[7206] exited with preempt_count 1
scheduling while atomic: hcid/0x10000001/7206
 [<c02b2e42>] schedule+0x522/0x530
 [<c0147e6e>] unmap_page_range+0x7e/0xa0
 [<c02b36da>] cond_resched+0x2a/0x50
 [<c0148040>] unmap_vmas+0x1b0/0x210
 [<c014ceac>] exit_mmap+0x7c/0x170
 [<c01192d7>] mmput+0x37/0xb0
 [<c011dc83>] do_exit+0x93/0x3c0
 [<c010439b>] die+0x18b/0x190
 [<c011baf7>] printk+0x17/0x20
 [<c0115fea>] do_page_fault+0x2da/0x5d5
 [<c0115eac>] do_page_fault+0x19c/0x5d5
 [<c0117038>] recalc_task_prio+0x88/0x150
 [<c013ddf1>] __rmqueue+0xb1/0xf0
 [<c013de5e>] rmqueue_bulk+0x2e/0x90
 [<c013dd30>] prep_new_page+0x60/0x70
 [<c013e2af>] buffered_rmqueue+0xdf/0x210
 [<c0115d10>] do_page_fault+0x0/0x5d5
 [<c0103be7>] error_code+0x2b/0x30
 [<c0130b72>] prepare_to_wait+0x32/0xa0
 [<c0130d10>] autoremove_wake_function+0x0/0x60
 [<ded652ee>] bluecard_write_wakeup+0x20e/0x2d0 [bluecard_cs]
 [<c0130d10>] autoremove_wake_function+0x0/0x60
 [<c0148da0>] do_wp_page+0x1d0/0x340
 [<c0130d10>] autoremove_wake_function+0x0/0x60
 [<c0258607>] alloc_skb+0x47/0xf0
 [<ded65a45>] bluecard_hci_set_baud_rate+0xe5/0x160 [bluecard_cs]
 [<ded65b53>] bluecard_hci_open+0x73/0x80 [bluecard_cs]
 [<dece72e1>] hci_dev_open+0x51/0x240 [bluetooth]
 [<c0254b59>] sock_ioctl+0xd9/0x260
 [<c016cfce>] do_ioctl+0x8e/0xa0
 [<c016d215>] vfs_ioctl+0x65/0x1f0
 [<c016d407>] sys_ioctl+0x67/0x90
 [<c01031af>] syscall_call+0x7/0xb

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#4 lore raw
Quoted 19 lines · Marcel Holtmann

Hi Nish,

Quoted 17 lines
Quoted 8 lines
Quoted 5 lines

Please consider applying.

Description: Remove deprecated interruptible_sleep_on_timeout() function calls and replace with direct wait-queue usage. Some naming conflicts occurred, which I tried to resolve appropriately. Patch is compile-tested.

you introduce more code than you remove. Is our code a special case or why don't exists a simple API function as replacement?

Yes, I do. *sleep_on*() was the API, which has been replaced with wait_event*(). However, wait_event*() explicitly needs a condition (the "event"), which in thise case does not exist. If I had more background, I would have gone the wait_event*() route. I am looking into a better API to mask away all of the DEFINE_WAIT(), prepare_to_wait(), schedule()/schedule_timeout(), finish_wait() additions (the KJ list can attest to how many of these replacements I'm doing), but there simply does not exist such an API right now.

Quoted 4 lines
Quoted 1 line

Any input you could provide, though, would be greatly appreciated.

I gave your patch a shot and the oops below happens on a 2.6.11-rc3 after plugging in the card. Without the patch it is working.

Sorry for the long delay without a response!

That looks like I may have missed some wake-up related issues. I'll check on it and get back to you soon.

Thanks,
Nish

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

maintainer
#5 lore raw

Hi Nish,

Quoted 7 lines
Quoted 2 lines

I gave your patch a shot and the oops below happens on a 2.6.11-rc3 after plugging in the card. Without the patch it is working.

Sorry for the long delay without a response!

That looks like I may have missed some wake-up related issues. I'll check on it and get back to you soon.

take your time, because my 2.6.11 queue is closed. All of your other changes are merged back mainline.

Regards

Marcel

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#6 lore raw
In reply to Marcel Holtmann
Quoted 1 line · Marcel Holtmann

Hi Nish,

...

Quoted 2 lines
Unable to handle kernel NULL pointer dereference at virtual address 00000004
 printing eip:

...

Quoted 1 line

EIP is at prepare_to_wait+0x32/0xa0

...

I think this is because wait_queue_head_t variable is uninitialized. (task_list is at offset 4, __add_wait_queue from prepare_to_wait uses it)

DECLARE_WAIT_QUEUE_HEAD should take care of it.

Domen

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors