On 9/23/22 7:02 PM, Uday Shankar wrote:
> ---
> Looking for feedback on the "allowed source states" list. The bug I hit
> is solved by prohibiting transitions out of SDEV_BLOCKED, but I think
> most others shouldn't be allowed either.
I think it's ok to be restrictive:
1. BLOCKED is just broken. When the transport classes and scsi_lib transition
out of that state they do a lot more than just set the set. We are leaving
the kernel in mismatched state where the device state is running but the
block layerand transport classes are not ready for IO.
2. CREATED does not happen. We go into RUNNING then do scsi_sysfs_add_sdev so
userspace should not see the CREATED state.
3. I'm not 100% sure about SDEV_QUIESCE though. It looks like it has similar issues
as BLOCKED where scsi_device_resume will undo things it did in scsi_device_quiesce,
so we can't just set the state to RUNNING and expect things to work. I'm not
sure about the scsi_transport_spi cases.
It would be best for James or Hannes to comment because they know that code well.
4. The transport classes are the ones that put devs in SDEV_TRANSPORT_OFFLINE
and then transition them when they are ready. The block layer is at least in
the correct state, but the transport classes may not be ready for IO since they
are not expecting IO to be queued to them at that time.
>
> drivers/scsi/scsi_sysfs.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 9dad2fd5297f..b38c30fe681d 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -822,6 +822,14 @@ store_state_field(struct device *dev, struct device_attribute *attr,
> }
>
> mutex_lock(&sdev->state_mutex);
> + switch (sdev->sdev_state) {
> + case SDEV_RUNNING:
> + case SDEV_OFFLINE:
> + break;
> + default:
> + mutex_unlock(&sdev->state_mutex);
> + return -EINVAL;
> + }
> if (sdev->sdev_state == SDEV_RUNNING && state == SDEV_RUNNING) {
> ret = 0;
> } else {
>
> base-commit: 7f615c1b5986ff08a725ee489e838c90f8197bcd
