Takahiro Yasui wrote: > Matthew Wilcox wrote: >> On Mon, Apr 27, 2009 at 07:52:57PM -0400, Takahiro Yasui wrote: >>> James Bottomley wrote: >>>>> + if (sdev->sdev_state != SDEV_BLOCK) >>>> This isn't quite correct. There are two blocked states in the model >>>> currently: SDEV_BLOCK and SDEV_CREATED_BLOCK ... you'd need to check >>>> for both of them >>>> >>>>> + return 0; >>>> Traditionally the return for an attempted invalid state transition is >>>> -EINVAL. >>>> >>>> I suppose for lower down, if you check the state, now we know we go >>>> >>>> SDEV_CREATED_BLOCK -> SDEV_CREATED >>>> >>>> or >>>> >>>> SDEV_BLOCK -> SDEV_RUNNING >>>> >>>> so there's no need for the dual scsi_device_set_state. >>> Thank you for the comments. If I understand your comments correctly, >>> the state transition is better to be defined in scsi_device_set_state(), >>> and both transitions, "SDEV_CREATED_BLOCK -> SDEV_CREATED" and >>> "SDEV_BLOCK -> SDEV_RUNNING" need to be covered. I updated the patch >>> so that the transition of "SDEV_OFFLINE -> SDEV_RUNNING" is prohibited. >> I don't think that's what he meant. I think he meant something more like: I have one question about the order of comparisons. The original code tries to change to SDEV_BLOCK first and retries to SDEV_CREATED. If they are transformed just that order, I think that comparisons will be as follows. if (sdev->sdev_state == SDEV_BLOCK) sdev->sdev_state = SDEV_RUNNING; else if (sdev->sdev_state == SDEV_CREATED_BLOCK) sdev->sdev_state = SDEV_CREATED; else return -EINVAL; Is this wrong order? Regards, --- Takahiro Yasui Hitachi Computer Products (America), Inc. -- 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
