On 12/06/16 19:41, Wei Fang wrote:
> On 2016/12/7 10:45, Bart Van Assche wrote:
>> The purpose of the scsi_device_set_state(sdev, SDEV_RUNNING) call in
>> scsi_sysfs_add_sdev() is to change the device state from SDEV_CREATED
>> into SDEV_RUNNING. Have you tried to modify scsi_sysfs_add_sdev() such
>> that it only changes the device state into SDEV_RUNNING if the current
>> state is SDEV_CREATED and also such that it changes SDEV_CREATED_BLOCK
>> into SDEV_BLOCK?
>
> Does those code in scsi_add_lun() have done this thing?
> ...
> ret = scsi_device_set_state(sdev, SDEV_RUNNING);
> if (ret) {
> ret = scsi_device_set_state(sdev, SDEV_BLOCK);
> ...
> }
> ...
> You mean we shouldn't change the device state from SDEV_BLOCK
> into SDEV_RUNNING in scsi_sysfs_add_sdev()?
>
> I thought it doesn't matter that the state is changed from SDEV_BLOCK
> to SDEV_RUNNING in scsi_sysfs_add_sdev(), if the queue can be unblocked
> in scsi_internal_device_unblock() in SDEV_RUNNING state. But it
> was broken since commit 5c10e63c943b.
Hello Wei,
I am aware that commit 5c10e63c943b caused the behavior change. But that
doesn't mean that a fix has to undo the changes introduced by that
commit. We do not only want to make sure that the SCSI core works as
intended but also that the SCSI core code is as easy to comprehend as
reasonably possible. Adding "&& sdev->sdev_state != SDEV_RUNNING" in
scsi_internal_device_unblock() would require a long comment to explain
why that code has been added. I think modifying scsi_sysfs_add_sdev()
such that it does not unblock devices will result in code that is easier
to understand.
Bart.
--
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
