Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver

Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver

[Matthew Wilcox]

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

Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver

[Felipe W Damasio]

[-- 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);
 

Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver

[Jens Axboe]

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

[PATCH] blk API update (and bug fix) to CDU535 cdrom driver

[Felipe W Damasio]

[-- 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);