| Message ID | 20190617151820.241583-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | ac88c1f6730e73771dc36b9fd83804e7aa15941a |
| Headers | show |
| Series | Avoid that .queuecommand() gets called for a quiesced SCSI device | expand |
On 6/17/19 5:18 PM, Bart Van Assche wrote: > The ability to modify the SCSI device state was introduced by commit > 638127e579a4 ("[PATCH] Fix error handler offline behaviour"; v2.6.12). That > same commit introduced the following device states: > > { SDEV_CREATED, "created" }, > { SDEV_RUNNING, "running" }, > { SDEV_CANCEL, "cancel" }, > { SDEV_DEL, "deleted" }, > { SDEV_QUIESCE, "quiesce" }, > { SDEV_OFFLINE, "offline" }, > > The SDEV_BLOCK state was introduced later to avoid that an FC cable pull > would immediately result in an I/O error (commit 1094e682310e; "[PATCH] > suspending I/Os to a device"; v2.6.12). That same patch introduced the > ability to set the SDEV_BLOCK state from user space. I'm not sure whether > that ability was introduced on purpose or accidentally. > > Since there is agreement that only writing "running" or "offline" into > the SCSI sysfs device state attribute makes sense, restrict sysfs writes > to these values. > > This patch makes sure that SDEV_BLOCK is only used for its original > purpose, namely to allow transport drivers and LLDs to block further > .queuecommand() calls while transport layer or adapter recovery is in > progress. > > Note: a web search for "/sys/class/scsi_device" AND "device/state" > revealed several storage configuration guides. The instructions I found > in these guides tell users to write the value "running" or "offline" in > the SCSI device state sysfs attribute and no other values. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Hannes Reinecke <hare@suse.de> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > Cc: James Smart <james.smart@broadcom.com> > Cc: Ewan D. Milne <emilne@redhat.com> > Cc: Laurence Oberman <loberman@redhat.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/scsi_sysfs.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index 3b119ca0cc0c..850cdc731a1f 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -766,8 +766,13 @@ store_state_field(struct device *dev, struct device_attribute *attr, > break; > } > } > - if (!state) > + switch (state) { > + case SDEV_RUNNING: > + case SDEV_OFFLINE: > + break; > + default: > return -EINVAL; > + } > > mutex_lock(&sdev->state_mutex); > ret = scsi_device_set_state(sdev, state); > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 3b119ca0cc0c..850cdc731a1f 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -766,8 +766,13 @@ store_state_field(struct device *dev, struct device_attribute *attr, break; } } - if (!state) + switch (state) { + case SDEV_RUNNING: + case SDEV_OFFLINE: + break; + default: return -EINVAL; + } mutex_lock(&sdev->state_mutex); ret = scsi_device_set_state(sdev, state);
The ability to modify the SCSI device state was introduced by commit 638127e579a4 ("[PATCH] Fix error handler offline behaviour"; v2.6.12). That same commit introduced the following device states: { SDEV_CREATED, "created" }, { SDEV_RUNNING, "running" }, { SDEV_CANCEL, "cancel" }, { SDEV_DEL, "deleted" }, { SDEV_QUIESCE, "quiesce" }, { SDEV_OFFLINE, "offline" }, The SDEV_BLOCK state was introduced later to avoid that an FC cable pull would immediately result in an I/O error (commit 1094e682310e; "[PATCH] suspending I/Os to a device"; v2.6.12). That same patch introduced the ability to set the SDEV_BLOCK state from user space. I'm not sure whether that ability was introduced on purpose or accidentally. Since there is agreement that only writing "running" or "offline" into the SCSI sysfs device state attribute makes sense, restrict sysfs writes to these values. This patch makes sure that SDEV_BLOCK is only used for its original purpose, namely to allow transport drivers and LLDs to block further .queuecommand() calls while transport layer or adapter recovery is in progress. Note: a web search for "/sys/class/scsi_device" AND "device/state" revealed several storage configuration guides. The instructions I found in these guides tell users to write the value "running" or "offline" in the SCSI device state sysfs attribute and no other values. Cc: Christoph Hellwig <hch@lst.de> Cc: Ming Lei <ming.lei@redhat.com> Cc: Hannes Reinecke <hare@suse.de> Cc: Johannes Thumshirn <jthumshirn@suse.de> Cc: James Smart <james.smart@broadcom.com> Cc: Ewan D. Milne <emilne@redhat.com> Cc: Laurence Oberman <loberman@redhat.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/scsi_sysfs.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)