On Thu, 2016-12-08 at 10:28 +0800, Wei Fang wrote:
> Hi, James, Ewan,
>
> On 2016/12/8 7:43, James Bottomley wrote:
> > On Wed, 2016-12-07 at 15:30 -0500, Ewan D. Milne wrote:
> > > On Wed, 2016-12-07 at 12:09 -0800, James Bottomley wrote:
> > > > Hm, it looks like the state set in scsi_sysfs_add_sdev() is
> > > > bogus.
> > > > We expect the state to have been properly set before that (in
> > > > scsi_add_lun), so can we not simply remove it?
> > > >
> > > > James
> > > >
> > >
> > > I was considering that, but...
> > >
> > > enum scsi_device_state {
> > > SDEV_CREATED = 1, /* device created but not added
> > > to
> > > sysfs
> > >
> > >
> > >
> > >
> > > * Only internal commands allowed
> > > (for inq) */
> > >
> > > So it seems the intent was for the state to not change until
> > > then.
> >
> > I think this is historical. There was a change somewhere that
> > moved
> > the sysfs state handling out of the sdev stat to is_visible, so the
> > sdev state no-longer reflects it.
> >
> > > The call to set the SDEV_RUNNING state earlier in scsi_add_lun()
> > > was added with:
> > >
> > > commit 6f4267e3bd1211b3d09130e626b0b3d885077610
> > > Author: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > > Date: Fri Aug 22 16:53:31 2008 -0500
> > >
> > > [SCSI] Update the SCSI state model to allow blocking in the
> > > created state
> > >
> > > Which allows the device to go into ->BLOCK (which is needed,
> > > since it
> > > actually happens).
> > >
> > > Should we remove the call from scsi_sysfs_add_sdev() and change
> > > the
> > > comment in scsi_device.h to reflect the intent?
>
> This sounds reasonable.
>
> > Assuming someone with the problem actually tests it, yes.
>
> This problem can be stably reproduced on Zengxi Chen's machine, who
> reported the bug. We can test it on this machine.
>
> The patch is as below, just for sure:
>
> 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;
> -
That's it, although not the second hunk: CREATED still means device not
added to sysfs. It's just that RUNNING now doesn't mean it is.
James
--
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
