* Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver @ 2003-09-10 19:58 Matthew Wilcox 0 siblings, 0 replies; 4+ messages in thread From: Matthew Wilcox @ 2003-09-10 19:58 UTC (permalink / raw) To: Felipe W Damasio, Jens Axboe; +Cc: linux-kernel This cli-sti removal seems exactly as broken as all the ones i've NAKed on kernel-janitors. There's no evidence that I can see for locking in the interrupt handler. Here's a race example: CPU 1 CPU 2 sony_sleep(); spin_lock_irq(&sonycd535_lock); enable_interrupts(); cdu535_interrupt(); disable_interrupts(); if (waitqueue_active(&cdu535_irq_wait)) { prepare_to_wait(&cdu535_irq_wait, &wait, TASK_INTERRUPTIBLE); spin_unlock_irq(&sonycd535_lock); Either you need to move prepare_to_wait before enable_interrupts or grab the sonycd535_lock in interrupt context. Hang on a minute. This driver is always polled, and never interrupt driven. There's no problem because this code path is never executed :-P Nevertheless, it's probaby worth fixing so other more complex drivers (eg cdu31a) don't copy it wrongly. BTW, I bet sony_sleep() shouldn't be calling the new-and-improved yield(). -- "It's not Hollywood. War is real, war is primarily not about defeat or victory, it is about death. I've seen thousands and thousands of dead bodies. Do you think I want to have an academic debate on this subject?" -- Robert Fisk ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <3F5DDEA8.6040901@terra.com.br>]
[parent not found: <20030909143341.GA18257@suse.de>]
[parent not found: <3F5DEA0D.6030701@terra.com.br>]
[parent not found: <20030909153536.GH18257@suse.de>]
* [PATCH] blk API update (and bug fix) to CDU535 cdrom driver [not found] ` <20030909153536.GH18257@suse.de> @ 2003-09-09 17:50 ` Felipe W Damasio 2003-09-09 19:55 ` Jens Axboe 0 siblings, 1 reply; 4+ messages in thread From: Felipe W Damasio @ 2003-09-09 17:50 UTC (permalink / raw) To: Jens Axboe; +Cc: Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 1050 bytes --] Hi Jens, Jens Axboe wrote: > held! Ugh, and the request function do_cdu535_request calls > schedule_timeout with the queue lock held (that is held when that > function is entered), that is very buggy as well. Should also use > set_current_state() right above that call, not open code it (that also > misses a memory barrier). Same function also has problems with request > handling. You should kill: > > if (!(req->flags & REQ_CMD)) > continue; /* FIXME */ > > that is very broken, make that: > > if (!blk_fs_request(req)) { > end_request(req, 0); > continue; > } > > and kill these two lines: > > if (rq_data_dir(req) != READ) > panic("Unknown SONY CD cmd"); > > they are screwy too. > > Care to fix the things I outlined above? This patch I think fixes all these, doesn't it? It applies on top of my latest cli-sti-removal patch that I sent you. The only place I'm not sure is on releasing the queue lock before calling schedule_timeout. Please apply if it looks good to you. Thanks for all your help! Felipe [-- Attachment #2: cdu535.patch --] [-- Type: text/plain, Size: 1331 bytes --] --- linux-2.6.0-test5/drivers/cdrom/sonycd535.c Tue Sep 9 14:42:52 2003 +++ linux-2.6.0-test5-fwd/drivers/cdrom/sonycd535.c Tue Sep 9 14:41:30 2003 @@ -812,14 +812,14 @@ block = req->sector; nsect = req->nr_sectors; - if (!(req->flags & REQ_CMD)) - continue; /* FIXME */ + if (!blk_fs_request(req)) { + end_request(req, 0); + continue; + } if (rq_data_dir(req) == WRITE) { end_request(req, 0); continue; } - if (rq_data_dir(req) != READ) - panic("Unknown SONY CD cmd"); /* * If the block address is invalid or the request goes beyond * the end of the media, return an error. @@ -896,8 +896,10 @@ } if (readStatus == BAD_STATUS) { /* Sleep for a while, then retry */ - current->state = TASK_INTERRUPTIBLE; + set_current_state(TASK_INTERRUPTIBLE); + spin_unlock_irq(&sonycd535_lock); schedule_timeout(RETRY_FOR_BAD_STATUS*HZ/10); + spin_lock_irq(&sonycd535_lock); } #if DEBUG > 0 printk(CDU535_MESSAGE_NAME @@ -1481,7 +1483,7 @@ /* look for the CD-ROM, follows the procedure in the DOS driver */ inb(select_unit_reg); /* wait for 40 18 Hz ticks (reverse-engineered from DOS driver) */ - current->state = TASK_INTERRUPTIBLE; + set_current_state(TASK_INTERRUPTIBLE); schedule_timeout((HZ+17)*40/18); inb(result_reg); ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver 2003-09-09 17:50 ` Felipe W Damasio @ 2003-09-09 19:55 ` Jens Axboe 2003-09-09 20:21 ` Felipe W Damasio 0 siblings, 1 reply; 4+ messages in thread From: Jens Axboe @ 2003-09-09 19:55 UTC (permalink / raw) To: Felipe W Damasio; +Cc: Linux Kernel Mailing List On Tue, Sep 09 2003, Felipe W Damasio wrote: > Hi Jens, > > Jens Axboe wrote: > >held! Ugh, and the request function do_cdu535_request calls > >schedule_timeout with the queue lock held (that is held when that > >function is entered), that is very buggy as well. Should also use > >set_current_state() right above that call, not open code it (that also > >misses a memory barrier). Same function also has problems with request > >handling. You should kill: > > > > if (!(req->flags & REQ_CMD)) > > continue; /* FIXME */ > > > >that is very broken, make that: > > > > if (!blk_fs_request(req)) { > > end_request(req, 0); > > continue; > > } > > > >and kill these two lines: > > > > if (rq_data_dir(req) != READ) > > panic("Unknown SONY CD cmd"); > > > >they are screwy too. > > > >Care to fix the things I outlined above? > > This patch I think fixes all these, doesn't it? > > It applies on top of my latest cli-sti-removal patch that I sent you. That needed changes too, as per my last mail. Please send one complete patch with all the fixes in, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver 2003-09-09 19:55 ` Jens Axboe @ 2003-09-09 20:21 ` Felipe W Damasio 0 siblings, 0 replies; 4+ messages in thread From: Felipe W Damasio @ 2003-09-09 20:21 UTC (permalink / raw) To: Jens Axboe; +Cc: Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 427 bytes --] Hi, Jens Axboe wrote: > That needed changes too, as per my last mail. Please send one complete > patch with all the fixes in, thanks. Sure thing, patch attached: - cli-sti removal - blk API update - set_current_state - Remove 'panic' line. Please don't forget to remove BROKEN_ON_SMP from drivers/cdrom/Kconfig on then cdu535 dependecies (I sent you a patch for that as well). Thanks for your help Jens, Felipe [-- Attachment #2: cdu535.patch --] [-- Type: text/plain, Size: 2233 bytes --] --- linux-2.6.0-test5/drivers/cdrom/sonycd535.c Tue Sep 9 17:01:08 2003 +++ linux-2.6.0-test5-fwd/drivers/cdrom/sonycd535.c Tue Sep 9 14:41:30 2003 @@ -36,6 +36,10 @@ * module_init & module_exit. * Torben Mathiasen <tmm@image.dk> * + * September 2003 - Fix SMP support by removing cli/sti calls. + * Using spinlocks with a wait_queue instead. + * Felipe Damasio <felipewd@terra.com.br> + * * Things to do: * - handle errors and status better, put everything into a single word * - use interrupts (code mostly there, but a big hole still missing) @@ -340,10 +344,14 @@ if (sony535_irq_used <= 0) { /* poll */ yield(); } else { /* Interrupt driven */ - cli(); + DEFINE_WAIT(wait); + + spin_lock_irq(&sonycd535_lock); enable_interrupts(); - interruptible_sleep_on(&cdu535_irq_wait); - sti(); + prepare_to_wait(&cdu535_irq_wait, &wait, TASK_INTERRUPTIBLE); + spin_unlock_irq(&sonycd535_lock); + schedule(); + finish_wait(&cdu535_irq_wait, &wait); } } @@ -804,14 +812,14 @@ block = req->sector; nsect = req->nr_sectors; - if (!(req->flags & REQ_CMD)) - continue; /* FIXME */ + if (!blk_fs_request(req)) { + end_request(req, 0); + continue; + } if (rq_data_dir(req) == WRITE) { end_request(req, 0); continue; } - if (rq_data_dir(req) != READ) - panic("Unknown SONY CD cmd"); /* * If the block address is invalid or the request goes beyond * the end of the media, return an error. @@ -888,8 +896,10 @@ } if (readStatus == BAD_STATUS) { /* Sleep for a while, then retry */ - current->state = TASK_INTERRUPTIBLE; + set_current_state(TASK_INTERRUPTIBLE); + spin_unlock_irq(&sonycd535_lock); schedule_timeout(RETRY_FOR_BAD_STATUS*HZ/10); + spin_lock_irq(&sonycd535_lock); } #if DEBUG > 0 printk(CDU535_MESSAGE_NAME @@ -1473,7 +1483,7 @@ /* look for the CD-ROM, follows the procedure in the DOS driver */ inb(select_unit_reg); /* wait for 40 18 Hz ticks (reverse-engineered from DOS driver) */ - current->state = TASK_INTERRUPTIBLE; + set_current_state(TASK_INTERRUPTIBLE); schedule_timeout((HZ+17)*40/18); inb(result_reg); ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2003-09-10 19:58 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-09-10 19:58 [PATCH] blk API update (and bug fix) to CDU535 cdrom driver Matthew Wilcox [not found] <3F5DDEA8.6040901@terra.com.br> [not found] ` <20030909143341.GA18257@suse.de> [not found] ` <3F5DEA0D.6030701@terra.com.br> [not found] ` <20030909153536.GH18257@suse.de> 2003-09-09 17:50 ` Felipe W Damasio 2003-09-09 19:55 ` Jens Axboe 2003-09-09 20:21 ` Felipe W Damasio
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).