Date: Wed, 10 Sep 2003 20:58:31 +0100
From: Matthew Wilcox <willy@debian.org>
To: Felipe W Damasio <felipewd@terra.com.br>, Jens Axboe <axboe@suse.de>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] blk API update (and bug fix) to CDU535 cdrom driver
Message-ID: <20030910195831.GD21596@parcelfarce.linux.theplanet.co.uk>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.4.1i
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1386
Lines: 35


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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/