Re: [PATCH 1/5] scsi host / scsi target state model update

[Mike Anderson <andmike@us.ibm.com>]

Alan Stern [stern@rowland.harvard.edu] wrote:
> Mike:
> 
> In your first patch, you don't allow transitions from SHOST_RECOVERY to 
> SHOST_CANCEL nor the other way around.  So this section of the patch looks 
> suspicious:
> 

Yes the host state model may need to allow this transition. The rest of
the patch series removes scsi_host_cancel so I was not running this
function when the series was fully applied.

> @@ -60,12 +136,11 @@ static void scsi_host_cancel(struct Scsi
>  {
>         struct scsi_device *sdev;
>  
> -       set_bit(SHOST_CANCEL, &shost->shost_state);
> +       scsi_host_set_state(shost, SHOST_CANCEL);
>         shost_for_each_device(sdev, shost) {
>                 scsi_device_cancel(sdev, recovery);
>         }
> -       wait_event(shost->host_wait, (!test_bit(SHOST_RECOVERY,
> -                                               &shost->shost_state)));
> +       wait_event(shost->host_wait, (shost->shost_state != SHOST_RECOVERY));
>  }
> 
> 
> In fact there are lots of places in the patch where scsi_host_set_state 
> is called and the return value is not checked.  They may end up causing 
> trouble.
> 

Yes, I am not sure what checking a return value will do in all cases (like
in scsi_remove_host). I notice that most of scsi_device_set_state cases
are not checked or have void function wrappers. It would appear that these
functions (scsi_device_set_state and scsi_host_set_state) should be void
functions with WARN_ONs to go correct the state model or the calling
function.

> Also, is it a good idea to allow write access to the shost_state 
> attribute?  For debugging, yes, okay, but in general it doesn't seem like 
> a good thing.
> 

Yes, after debugging we should remove the write access. We allow write on
the device state (usually used to re-online a device that the error handler
marked offline), but there probably is no need to change the state of a
host from user space. In a future patch we could remove write access to
the scsi device state and possibly have an option to not offline the
devices in the error handler to cover the need I would assume most people
use the device state write access for.

-andmike
--
Michael Anderson
andmike@us.ibm.com