Re: oops on usb storage device disconnect with 2.6.14-rc1

Alan Stern
2005-09-18T20:37:55Z

On Sun, 18 Sep 2005, James Bottomley wrote:

> void scsi_remove_host(struct Scsi_Host *shost)
> {
> + unsigned long flags;
> down(&shost->scan_mutex);
> - scsi_host_set_state(shost, SHOST_CANCEL);
> + spin_lock_irqsave(shost->host_lock, flags);
> + if (scsi_host_set_state(shost, SHOST_CANCEL))
> + if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) {
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + up(&shost->scan_mutex);
> + return;
> + }
> + spin_unlock_irqrestore(shost->host_lock, flags);
> up(&shost->scan_mutex);
> scsi_forget_host(shost);

Personally I would prefer to see something like:

if (scsi_host_set_state(shost,
(shost->shost_state == SHOST_RECOVERY) ?
SHOST_CANCEL_RECOVERY : SHOST_CANCEL)) {
spin_unlock_irqrestore...

with similar changes in the other places. It avoids a function call and
prevents misleading error messages from showing up in the system log.

Actually, I would probably do

unsigned long flags;
int rc;

down(&shost->scan_mutex);
spin_lock_irqsave(shost->host_lock, flags);
rc = scsi_host_set_state(shost,
(shost->shost_state == SHOST_RECOVERY) ?
SHOST_CANCEL_RECOVERY : SHOST_CANCEL);
spin_unlock_irqrestore(shost->host_lock, flags);
up(&shost->scan_mutex);
if (rc)
return;
scsi_forget_host(shost);
...

just to avoid the extra spin_unlock call. Those macros can expand to a
surprising length.

Alan Stern

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at: