Re: [RFC] IBM Power RAID driver (ipr)

[Muli Ben-Yehuda <mulix@mulix.org>]

[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]

On Fri, Jan 16, 2004 at 03:59:54PM -0600, Brian King wrote:

> http://www-124.ibm.com/storageio/ipr/patch-2.6.1-ipr-2.0.0-1.gz

Hi, I took a look and I have some comments / questions. I will try to
go over all of it in the next few days, but here's what I went over so
far: 

> +static int ipr_unsafe = 1;		/* xxx set to 0 for ship */

Should it be set to 0 or to 1 for inclusion in the kernel? any driver
that is first included is considered experimental, so just how unsafe
is ipr_unsafe?

> +module_param_named(unsafe, ipr_unsafe, int, 0);
> +MODULE_PARM_DESC(unsafe, "Do not use!");

Same question, I guess, and if you decide to keep it, please add a
more descriptive MODULE_PARM_DESC string... 

> +static void ipr_sleep_no_lock(signed long delay)
> +{
> +	DECLARE_WAIT_QUEUE_HEAD(internal_wait);
> +
> +	sleep_on_timeout(&internal_wait, (delay * HZ) / 1000);
> +	return;

just a nitpick, but the Linux coding style says no 'return' from void
functions. 

>+static void ipr_sleep(struct ipr_ioa_cfg *ioa_cfg, signed long delay)
> +{
> +	spin_unlock_irq(ioa_cfg->host->host_lock);
> +	ipr_sleep_no_lock(delay);
> +	spin_lock_irq(ioa_cfg->host->host_lock);

Hmpf... it's called from ipr_diagnostic_ioctl(), which does
spin_lock_irqsave()
ipr_sleep()
spin_unlock_irqrestore()

So I guess you really do intend to enable interrupts before going to
sleep? it's somewhat fragile... also, is the dropping and
reacquisition of the lock safe here (in the sense that none of the
data the lock is protecting that we care about can change while the
lock is not held?)

Same questions wrt ipr_interruptible_sleep_on(), which does the same
thing. By the way, ipr_dump_ioctl does not check the return value from
ipr_interruptible_sleep_on(). If this is intentional, perhaps call it
using (void)ipr_interruptible_sleep_on()?

> +static void ipr_block_requests(struct ipr_ioa_cfg *ioa_cfg)
> +{
> +	ENTER;
> +
> +	if (0 == ioa_cfg->block_host_ops++) {
> +	   ioa_cfg->allow_cmds = 0;

This is racy unless a lock is held. I think a lock is held here in all
callers of this function, but a comment to that effect would be nice
before the function. This also applies to every other function which
either must be called with a lock taken, or modifies the "locking
status" - takes a lock which is released elsewhere, enables interrupts
unconditionally, etc. 

+static void ipr_unblock_requests(struct ipr_ioa_cfg *ioa_cfg)
+{
+	ENTER;
+
+	if (0 == --ioa_cfg->block_host_ops) {
+	   ioa_cfg->allow_cmds = 1;
+	   spin_unlock_irq(ioa_cfg->host->host_lock);
+	   scsi_unblock_requests(ioa_cfg->host);
+	   spin_lock_irq(ioa_cfg->host->host_lock);

Eeek! What happens if on another CPU a call comes in that does
ipr_block_requests() after the lock is released and before it is taken
again?

That's as far as I got, for now. 

Cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

"the nucleus of linux oscillates my world" - gccbot@#offtopic


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]