scsi: Protect SCSI device state changes with a mutex

Serializing SCSI device state changes avoids that two state changes can
occur concurrently, e.g. the state changes in scsi_target_block() and
__scsi_remove_device(). This serialization is essential to make patch
"Make __scsi_remove_device go straight from BLOCKED to DEL" work
reliably.

Enable this mechanism for all scsi_target_*block() callers but not for
the scsi_internal_device_unblock() calls from the mpt3sas driver because
that driver can call scsi_internal_device_unblock() from atomic context.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Orabug: 27546768
(cherry picked from commit 0db6ca8a5e1ea585795db3643ec7d50fc8cb1aff)
Signed-off-by: Kyle Fortin <kyle.fortin@oracle.com>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Conflicts:
	drivers/scsi/scsi_error.c
	drivers/scsi/scsi_lib.c
	include/scsi/scsi_device.h

	Use kmalloc'd state_mutex_kabi in scsi_device to fit new field
	state_mutex in KABI reserved space.

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 841fdf745fcf5fab6766c10da356777bd93c451e..fe741661765d5d611417edd754a45bd6b92c2e18 100644 (file)
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1687,16 +1687,17 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
                                  struct list_head *done_q)
 {
        struct scsi_cmnd *scmd, *next;
+       struct scsi_device *sdev;
 
        list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
                sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
                            "not ready after error recovery\n");
-               scsi_device_set_state(scmd->device, SDEV_OFFLINE);
-               if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
-                       /*
-                        * FIXME: Handle lost cmds.
-                        */
-               }
+               sdev = scmd->device;
+
+               mutex_lock(sdev->state_mutex_kabi);
+               scsi_device_set_state(sdev, SDEV_OFFLINE);
+               mutex_unlock(sdev->state_mutex_kabi);
+
                scsi_eh_finish_cmd(scmd, done_q);
        }
        return;

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ab4d4f022d812427488dc7e1185f605f2a48f9ca..df9a7428168b6af40ad839ff53eadde13d7b49f5 100644 (file)
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2862,7 +2862,12 @@ EXPORT_SYMBOL_GPL(sdev_evt_send_simple);
 int
 scsi_device_quiesce(struct scsi_device *sdev)
 {
-       int err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+       int err;
+
+       mutex_lock(sdev->state_mutex_kabi);
+       err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+       mutex_unlock(sdev->state_mutex_kabi);
+
        if (err)
                return err;
 
@@ -2890,10 +2895,11 @@ void scsi_device_resume(struct scsi_device *sdev)
         * so assume the state is being managed elsewhere (for example
         * device deleted during suspend)
         */
-       if (sdev->sdev_state != SDEV_QUIESCE ||
-           scsi_device_set_state(sdev, SDEV_RUNNING))
-               return;
-       scsi_run_queue(sdev->request_queue);
+       mutex_lock(sdev->state_mutex_kabi);
+       if (sdev->sdev_state == SDEV_QUIESCE &&
+           scsi_device_set_state(sdev, SDEV_RUNNING) == 0)
+               scsi_run_queue(sdev->request_queue);
+       mutex_unlock(sdev->state_mutex_kabi);
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
@@ -3031,7 +3037,9 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
 static void
 device_block(struct scsi_device *sdev, void *data)
 {
+       mutex_lock(sdev->state_mutex_kabi);
        scsi_internal_device_block(sdev);
+       mutex_unlock(sdev->state_mutex_kabi);
 }
 
 static int
@@ -3057,7 +3065,9 @@ EXPORT_SYMBOL_GPL(scsi_target_block);
 static void
 device_unblock(struct scsi_device *sdev, void *data)
 {
+       mutex_lock(sdev->state_mutex_kabi);
        scsi_internal_device_unblock(sdev, *(enum scsi_device_state *)data);
+       mutex_unlock(sdev->state_mutex_kabi);
 }
 
 static int

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f605548054baac13a3a4d48264159840cc694e36..253cd8591d8adf99fec2e22e07076bc45937849a 100644 (file)
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -220,6 +220,13 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
        if (!sdev)
                goto out;
 
+       sdev->state_mutex_kabi = kzalloc(sizeof(*(sdev->state_mutex_kabi)),
+                                        GFP_ATOMIC);
+       if (!sdev->state_mutex_kabi) {
+               kfree(sdev);
+               goto out;
+       }
+
        sdev->vendor = scsi_null_device_strs;
        sdev->model = scsi_null_device_strs;
        sdev->rev = scsi_null_device_strs;
@@ -228,6 +235,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
        sdev->id = starget->id;
        sdev->lun = lun;
        sdev->channel = starget->channel;
+       mutex_init(sdev->state_mutex_kabi);
        sdev->sdev_state = SDEV_CREATED;
        INIT_LIST_HEAD(&sdev->siblings);
        INIT_LIST_HEAD(&sdev->same_target_siblings);
@@ -268,6 +276,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
                /* release fn is set up in scsi_sysfs_device_initialise, so
                 * have to free and put manually here */
                put_device(&starget->dev);
+               kfree(sdev->state_mutex_kabi);
                kfree(sdev);
                goto out;
        }
@@ -935,16 +944,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
        /* set the device running here so that slave configure
         * may do I/O */
+       mutex_lock(sdev->state_mutex_kabi);
        ret = scsi_device_set_state(sdev, SDEV_RUNNING);
-       if (ret) {
+       if (ret)
                ret = scsi_device_set_state(sdev, SDEV_BLOCK);
+       mutex_unlock(sdev->state_mutex_kabi);
 
-               if (ret) {
-                       sdev_printk(KERN_ERR, sdev,
-                                   "in wrong state %s to complete scan\n",
-                                   scsi_device_state_name(sdev->sdev_state));
-                       return SCSI_SCAN_NO_RESPONSE;
-               }
+       if (ret) {
+               sdev_printk(KERN_ERR, sdev,
+                           "in wrong state %s to complete scan\n",
+                           scsi_device_state_name(sdev->sdev_state));
+               return SCSI_SCAN_NO_RESPONSE;
        }
 
        if (*bflags & BLIST_MS_192_BYTES_FOR_3F)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 9a7c0bf43195e53a866d91d3570fa05e3d9ce0f7..78fbc35326a2c68c1a561437c1953afaf65ef058 100644 (file)
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -424,6 +424,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
        kfree(sdev->vpd_pg83);
        kfree(sdev->vpd_pg80);
        kfree(sdev->inquiry);
+       kfree(sdev->state_mutex_kabi);
        kfree(sdev);
 
        if (parent)
@@ -686,7 +687,7 @@ static ssize_t
 store_state_field(struct device *dev, struct device_attribute *attr,
                  const char *buf, size_t count)
 {
-       int i;
+       int i, ret;
        struct scsi_device *sdev = to_scsi_device(dev);
        enum scsi_device_state state = 0;
 
@@ -701,9 +702,11 @@ store_state_field(struct device *dev, struct device_attribute *attr,
        if (!state)
                return -EINVAL;
 
-       if (scsi_device_set_state(sdev, state))
-               return -EINVAL;
-       return count;
+       mutex_lock(sdev->state_mutex_kabi);
+       ret = scsi_device_set_state(sdev, state);
+       mutex_unlock(sdev->state_mutex_kabi);
+
+       return ret == 0 ? count : -EINVAL;
 }
 
 static ssize_t
@@ -1062,6 +1065,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
        struct device *dev = &sdev->sdev_gendev;
+       int res;
 
        /*
         * This cleanup path is not reentrant and while it is impossible
@@ -1072,7 +1076,15 @@ void __scsi_remove_device(struct scsi_device *sdev)
                return;
 
        if (sdev->is_visible) {
-               if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+               /*
+                * If scsi_internal_target_block() is running concurrently,
+                * wait until it has finished before changing the device state.
+                */
+               mutex_lock(sdev->state_mutex_kabi);
+               res = scsi_device_set_state(sdev, SDEV_CANCEL);
+               mutex_unlock(sdev->state_mutex_kabi);
+
+               if (res != 0)
                        return;
 
                bsg_unregister_queue(sdev->request_queue);
@@ -1087,7 +1099,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
         * scsi_run_queue() invocations have finished before tearing down the
         * device.
         */
+       mutex_lock(sdev->state_mutex_kabi);
        scsi_device_set_state(sdev, SDEV_DEL);
+       mutex_unlock(sdev->state_mutex_kabi);
+
        blk_cleanup_queue(sdev->request_queue);
        cancel_work_sync(&sdev->requeue_work);
 

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index f115f67a6ba58c65d1cf07cbfcb3a0630c9f6da9..ef734d68a1f1d622a81b9131ccb106fe2318e223 100644 (file)
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -586,11 +586,12 @@ int srp_reconnect_rport(struct srp_rport *rport)
                 * invoking scsi_target_unblock() won't change the state of
                 * these devices into running so do that explicitly.
                 */
-               spin_lock_irq(shost->host_lock);
-               __shost_for_each_device(sdev, shost)
+               shost_for_each_device(sdev, shost) {
+                       mutex_lock(sdev->state_mutex_kabi);
                        if (sdev->sdev_state == SDEV_OFFLINE)
                                sdev->sdev_state = SDEV_RUNNING;
-               spin_unlock_irq(shost->host_lock);
+                       mutex_unlock(sdev->state_mutex_kabi);
+               }
        } else if (rport->state == SRP_RPORT_RUNNING) {
                /*
                 * srp_reconnect_rport() has been invoked with fast_io_fail

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 94f48fcc27158873ec5818723f15ee825cdd7403..9cd46eca3482443c123ccda26f615225b3a68856 100644 (file)
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1565,8 +1565,9 @@ static const struct block_device_operations sd_fops = {
 static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 {
        struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
+       struct scsi_device *sdev = scmd->device;
 
-       if (!scsi_device_online(scmd->device) ||
+       if (!scsi_device_online(sdev) ||
            !scsi_medium_access_command(scmd) ||
            host_byte(scmd->result) != DID_TIME_OUT ||
            eh_disp != SUCCESS)
@@ -1589,7 +1590,9 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
        if (sdkp->medium_access_timed_out >= sdkp->max_medium_access_timeouts) {
                scmd_printk(KERN_ERR, scmd,
                            "Medium access timeout failure. Offlining disk!\n");
-               scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+               mutex_lock(sdev->state_mutex_kabi);
+               scsi_device_set_state(sdev, SDEV_OFFLINE);
+               mutex_unlock(sdev->state_mutex_kabi);
 
                return FAILED;
        }

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7cb449b18cec89ad4e5debdbcc89b0475e46abac..a55cb48340f78e41ba6381ee1ce458178952dc3e 100644 (file)
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -202,8 +202,13 @@ struct scsi_device {
           *
           * The following padding has been inserted before ABI freeze to
           * allow extending the structure while preserving ABI.
+         *
+         * The state_mutex struct wouldn't fit in KABI reserved space.
+         * Use a kmalloc'd storage for it and make it a pointer named
+         * as state_mutex_kabi to make future cherry-picks referencing
+         * state_mutex more obvious earlier.
           */
-        UEK_KABI_RESERVED(1)
+       UEK_KABI_USE(1, struct mutex *state_mutex_kabi)
         UEK_KABI_RESERVED(2)
        unsigned long           sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));