Alan Stern [stern@xxxxxxxxxxxxxxxxxxx] 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@xxxxxxxxxx
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
