| Message ID | 20190404144722.31361-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SCSI: unconditionally clean was_reset | expand |
On Thu, 2019-04-04 at 16:47 +0200, Oliver Neukum wrote: > If a reset of a SCSI device requires no relocking of the door, the > flag must be reset anyway, lest it trigger unnecessary door action > later on. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/scsi/scsi_error.c | 5 ﭗ橸ṷ梧뇪觬(), 3 deletions(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 16eef068e9e9..afb8cf0cdfed 100644 > --- a/drivers/scsi/scsi_error.c > ﯦｶ뢯窻㾱쬢ﻇⲉꊷ > @@ -1990,10 ퟟ瓶쭚똧 void scsi_restart_operations(struct Scsi_Host *shost) > * is no point trying to lock the door of an off-line device. > */ > shost_for_each_device(sdev, shost) { > - if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) { > if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) > scsi_eh_lock_door(sdev); > - sdev->was_reset = 0; > - } > sdev->was_reset = 0; > } I think there are two issues with this patch: * Shouldn't sdev->was_reset be cleared only if a device is online instead of ignoring the device online state? * An analysis of the impact of this patch on the osst and st drivers is missing. These two drivers namely read the "was_reset" member variable. Thanks, Bart.
On Do, 2019-04-04 at 08:51 -0700, Bart Van Assche wrote: > On Thu, 2019-04-04 at 16:47 +0200, Oliver Neukum wrote: > > If a reset of a SCSI device requires no relocking of the door, the > > flag must be reset anyway, lest it trigger unnecessary door action > > later on. > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > --- > > drivers/scsi/scsi_error.c | 5 ﭗ橸ṷ梧뇪觬(), 3 deletions(-) > > > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > > index 16eef068e9e9..afb8cf0cdfed 100644 > > --- a/drivers/scsi/scsi_error.c > > ﯦｶ뢯窻㾱쬢ﻇⲉꊷ > > @@ -1990,10 ퟟ瓶쭚똧 void scsi_restart_operations(struct Scsi_Host *shost) > > * is no point trying to lock the door of an off-line device. > > */ > > shost_for_each_device(sdev, shost) { > > - if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) { > > if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) > > scsi_eh_lock_door(sdev); > > - sdev->was_reset = 0; > > - } > > sdev->was_reset = 0; > > } > > I think there are two issues with this patch: > * Shouldn't sdev->was_reset be cleared only if a device is online instead of > ignoring the device online state? > * An analysis of the impact of this patch on the osst and st drivers is missing. > These two drivers namely read the "was_reset" member variable. True. You are right. This patch would break st. Do you have any suggestions how resets should be cleanly indicated? Short of splitting the flag I see no good solution. Regards Oliver
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 16eef068e9e9..afb8cf0cdfed 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1990,10 +1990,9 @@ static void scsi_restart_operations(struct Scsi_Host *shost) * is no point trying to lock the door of an off-line device. */ shost_for_each_device(sdev, shost) { - if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) { + if (scsi_device_online(sdev) && sdev->was_reset && sdev->locked) scsi_eh_lock_door(sdev); - sdev->was_reset = 0; - } + sdev->was_reset = 0; } /*
If a reset of a SCSI device requires no relocking of the door, the flag must be reset anyway, lest it trigger unnecessary door action later on. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/scsi/scsi_error.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)