Hi, James, Ewan, Bart,
On 2016/12/8 11:22, Wei Fang wrote:
> I looked through those code and found that if we fix this bug
> by removing setting the state in scsi_sysfs_add_sdev(), it
> can't be fixed completely:
>
> scsi_device_set_state(sdev, SDEV_RUNNING) in scsi_add_lun() and
> scsi_device_set_state(sdev, SDEV_CREATED_BLOCK) in scsi_internal_device_block()
> can be called simultaneously. Because there is no synchronization
> between scsi_device_set_state(), those calls may both return
> success, and the state may be SDEV_RUNNING after that, and the
> device queue is stopped.
Can we fix it in this way:
Add a state lock to make sure the result of simultaneously calling
of scsi_device_set_state() is not unpredictable.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 253ee74..80cb493 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2457,10 +2457,16 @@ EXPORT_SYMBOL(scsi_test_unit_ready);
int
scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
{
- enum scsi_device_state oldstate = sdev->sdev_state;
+ enum scsi_device_state oldstate;
+ unsigned long flags;
+
+ spin_lock_irqsave(&sdev->state_lock, flags);
+ oldstate = sdev->sdev_state;
- if (state == oldstate)
+ if (state == oldstate) {
+ spin_unlock_irqrestore(&sdev->state_lock, flags);
return 0;
+ }
switch (state) {
case SDEV_CREATED:
@@ -2558,9 +2564,11 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
}
sdev->sdev_state = state;
+ spin_unlock_irqrestore(&sdev->state_lock, flags);
return 0;
illegal:
+ spin_unlock_irqrestore(&sdev->state_lock, flags);
SCSI_LOG_ERROR_RECOVERY(1,
sdev_printk(KERN_ERR, sdev,
"Illegal state transition %s->%s",
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f..ba2f38f 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -238,6 +238,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
INIT_LIST_HEAD(&sdev->starved_entry);
INIT_LIST_HEAD(&sdev->event_list);
spin_lock_init(&sdev->list_lock);
+ spin_lock_init(&sdev->state_lock);
mutex_init(&sdev->inquiry_mutex);
INIT_WORK(&sdev->event_work, scsi_evt_thread);
INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 0734927..82dfe07 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1204,10 +1204,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;
- error = scsi_device_set_state(sdev, SDEV_RUNNING);
- if (error)
- return error;
-
error = scsi_target_add(starget);
if (error)
return error;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 8990e58..e00764e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -31,7 +31,7 @@ struct scsi_mode_data {
enum scsi_device_state {
SDEV_CREATED = 1, /* device created but not added to sysfs
* Only internal commands allowed (for inq) */
- SDEV_RUNNING, /* device properly configured
+ SDEV_RUNNING, /* device properly initialized
* All commands allowed */
SDEV_CANCEL, /* beginning to delete device
* Only error handler commands allowed */
@@ -207,6 +207,7 @@ struct scsi_device {
void *handler_data;
unsigned char access_state;
+ spinlock_t state_lock;
enum scsi_device_state sdev_state;
unsigned long sdev_data[0];
} __attribute__((aligned(sizeof(unsigned long))));
Haven't tested yet. Sending this for your opinion.
Thanks,
Wei
--
To unsubscribe from this list: 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
