diff mbox series

[v2,24/24] scsi: aic7xxx: aic79xx_osm: Remove set but unused variabes 'saved_scsiid' and 'saved_modes'

Message ID 20200713080001.128044-25-lee.jones@linaro.org
State New
Headers show
Series Set 3: Fix another set of SCSI related W=1 warnings | expand

Commit Message

Lee Jones July 13, 2020, 8 a.m. UTC
Haven't been used since 2006.

Fixes the following W=1 kernel build warning(s):

 drivers/scsi/aic7xxx/aic79xx_osm.c: In function ‘ahd_linux_queue_abort_cmd’:
 drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable ‘saved_modes’ set but not used [-Wunused-but-set-variable]
 drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>

---
 drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----
 1 file changed, 4 deletions(-)

-- 
2.25.1

Comments

Hannes Reinecke July 14, 2020, 7:46 a.m. UTC | #1
On 7/13/20 10:00 AM, Lee Jones wrote:
> Haven't been used since 2006.

> 

> Fixes the following W=1 kernel build warning(s):

> 

>  drivers/scsi/aic7xxx/aic79xx_osm.c: In function ‘ahd_linux_queue_abort_cmd’:

>  drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable ‘saved_modes’ set but not used [-Wunused-but-set-variable]

>  drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

> 

> Cc: Hannes Reinecke <hare@suse.com>

> Signed-off-by: Lee Jones <lee.jones@linaro.org>

> ---

>  drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

>  1 file changed, 4 deletions(-)

> 

> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c b/drivers/scsi/aic7xxx/aic79xx_osm.c

> index 3782a20d58885..140c4e74ddd7e 100644

> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

> @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)

>  	u_int  saved_scbptr;

>  	u_int  active_scbptr;

>  	u_int  last_phase;

> -	u_int  saved_scsiid;

>  	u_int  cdb_byte;

>  	int    retval;

>  	int    was_paused;

>  	int    paused;

>  	int    wait;

>  	int    disconnected;

> -	ahd_mode_state saved_modes;

>  	unsigned long flags;

>  

>  	pending_scb = NULL;

> @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)

>  		goto done;

>  	}

>  

> -	saved_modes = ahd_save_modes(ahd);

>  	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

>  	last_phase = ahd_inb(ahd, LASTPHASE);

>  	saved_scbptr = ahd_get_scbptr(ahd);

> @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)

>  	 * passed in command.  That command is currently active on the

>  	 * bus or is in the disconnected state.

>  	 */

> -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

>  	if (last_phase != P_BUSFREE

>  	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

>  

> 

Reviewed-by: Hannes Reinecke <hare@suse.de>


Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
James Bottomley July 14, 2020, 3:43 p.m. UTC | #2
On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:
> On 7/13/20 10:00 AM, Lee Jones wrote:

> > Haven't been used since 2006.

> > 

> > Fixes the following W=1 kernel build warning(s):

> > 

> >  drivers/scsi/aic7xxx/aic79xx_osm.c: In function

> > ‘ahd_linux_queue_abort_cmd’:

> >  drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

> > ‘saved_modes’ set but not used [-Wunused-but-set-variable]

> >  drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

> > ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

> > 

> > Cc: Hannes Reinecke <hare@suse.com>

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > ---

> >  drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

> >  1 file changed, 4 deletions(-)

> > 

> > diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > index 3782a20d58885..140c4e74ddd7e 100644

> > --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > *cmd)

> >  	u_int  saved_scbptr;

> >  	u_int  active_scbptr;

> >  	u_int  last_phase;

> > -	u_int  saved_scsiid;

> >  	u_int  cdb_byte;

> >  	int    retval;

> >  	int    was_paused;

> >  	int    paused;

> >  	int    wait;

> >  	int    disconnected;

> > -	ahd_mode_state saved_modes;

> >  	unsigned long flags;

> >  

> >  	pending_scb = NULL;

> > @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > *cmd)

> >  		goto done;

> >  	}

> >  

> > -	saved_modes = ahd_save_modes(ahd);

> >  	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

> >  	last_phase = ahd_inb(ahd, LASTPHASE);

> >  	saved_scbptr = ahd_get_scbptr(ahd);

> > @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > *cmd)

> >  	 * passed in command.  That command is currently active on

> > the

> >  	 * bus or is in the disconnected state.

> >  	 */

> > -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

> >  	if (last_phase != P_BUSFREE

> >  	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

> >  

> > 

> 

> Reviewed-by: Hannes Reinecke <hare@suse.de>


Hey, you don't get to do that ... I asked you to figure out why we're
missing an ahd_restore_modes().  Removing the ahd_save_modes() is
cosmetic: it gets rid of a warning but doesn't fix the problem.  I'd
rather keep the warning until the problem is fixed and the problem is
we need a mode save/restore around the ahd_set_modes() which is only
partially implemented in this function.

James
Lee Jones July 14, 2020, 9:39 p.m. UTC | #3
On Tue, 14 Jul 2020, James Bottomley wrote:

> On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:

> > On 7/13/20 10:00 AM, Lee Jones wrote:

> > > Haven't been used since 2006.

> > > 

> > > Fixes the following W=1 kernel build warning(s):

> > > 

> > >  drivers/scsi/aic7xxx/aic79xx_osm.c: In function

> > > ‘ahd_linux_queue_abort_cmd’:

> > >  drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

> > > ‘saved_modes’ set but not used [-Wunused-but-set-variable]

> > >  drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

> > > ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

> > > 

> > > Cc: Hannes Reinecke <hare@suse.com>

> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > > ---

> > >  drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

> > >  1 file changed, 4 deletions(-)

> > > 

> > > diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > index 3782a20d58885..140c4e74ddd7e 100644

> > > --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > > *cmd)

> > >  	u_int  saved_scbptr;

> > >  	u_int  active_scbptr;

> > >  	u_int  last_phase;

> > > -	u_int  saved_scsiid;

> > >  	u_int  cdb_byte;

> > >  	int    retval;

> > >  	int    was_paused;

> > >  	int    paused;

> > >  	int    wait;

> > >  	int    disconnected;

> > > -	ahd_mode_state saved_modes;

> > >  	unsigned long flags;

> > >  

> > >  	pending_scb = NULL;

> > > @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > > *cmd)

> > >  		goto done;

> > >  	}

> > >  

> > > -	saved_modes = ahd_save_modes(ahd);

> > >  	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

> > >  	last_phase = ahd_inb(ahd, LASTPHASE);

> > >  	saved_scbptr = ahd_get_scbptr(ahd);

> > > @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > > *cmd)

> > >  	 * passed in command.  That command is currently active on

> > > the

> > >  	 * bus or is in the disconnected state.

> > >  	 */

> > > -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

> > >  	if (last_phase != P_BUSFREE

> > >  	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

> > >  

> > > 

> > 

> > Reviewed-by: Hannes Reinecke <hare@suse.de>

> 

> Hey, you don't get to do that ... I asked you to figure out why we're

> missing an ahd_restore_modes().  Removing the ahd_save_modes() is

> cosmetic: it gets rid of a warning but doesn't fix the problem.  I'd

> rather keep the warning until the problem is fixed and the problem is

> we need a mode save/restore around the ahd_set_modes() which is only

> partially implemented in this function.


I had a look.  Traced it back to the dawn of time (time == Git), then
delved even further back by downloading and trawling through ~10-15
tarballs.  It looks as though drivers/scsi/aic7xxx/aic79xx_osm.c was
upstreamed in v2.5.60, nearly 20 years ago.  'saved_modes' has been
unused since at least then.  If no one has complained in 2 decades,
I'd say it probably isn't an issue worth perusing.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
James Bottomley July 14, 2020, 11:03 p.m. UTC | #4
On Tue, 2020-07-14 at 22:39 +0100, Lee Jones wrote:
> On Tue, 14 Jul 2020, James Bottomley wrote:

> 

> > On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:

> > > On 7/13/20 10:00 AM, Lee Jones wrote:

> > > > Haven't been used since 2006.

> > > > 

> > > > Fixes the following W=1 kernel build warning(s):

> > > > 

> > > >  drivers/scsi/aic7xxx/aic79xx_osm.c: In function

> > > > ‘ahd_linux_queue_abort_cmd’:

> > > >  drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

> > > > ‘saved_modes’ set but not used [-Wunused-but-set-variable]

> > > >  drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

> > > > ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

> > > > 

> > > > Cc: Hannes Reinecke <hare@suse.com>

> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > > > ---

> > > >  drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

> > > >  1 file changed, 4 deletions(-)

> > > > 

> > > > diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > index 3782a20d58885..140c4e74ddd7e 100644

> > > > --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct

> > > > scsi_cmnd

> > > > *cmd)

> > > >  	u_int  saved_scbptr;

> > > >  	u_int  active_scbptr;

> > > >  	u_int  last_phase;

> > > > -	u_int  saved_scsiid;

> > > >  	u_int  cdb_byte;

> > > >  	int    retval;

> > > >  	int    was_paused;

> > > >  	int    paused;

> > > >  	int    wait;

> > > >  	int    disconnected;

> > > > -	ahd_mode_state saved_modes;

> > > >  	unsigned long flags;

> > > >  

> > > >  	pending_scb = NULL;

> > > > @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct

> > > > scsi_cmnd

> > > > *cmd)

> > > >  		goto done;

> > > >  	}

> > > >  

> > > > -	saved_modes = ahd_save_modes(ahd);

> > > >  	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

> > > >  	last_phase = ahd_inb(ahd, LASTPHASE);

> > > >  	saved_scbptr = ahd_get_scbptr(ahd);

> > > > @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct

> > > > scsi_cmnd

> > > > *cmd)

> > > >  	 * passed in command.  That command is currently

> > > > active on

> > > > the

> > > >  	 * bus or is in the disconnected state.

> > > >  	 */

> > > > -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

> > > >  	if (last_phase != P_BUSFREE

> > > >  	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

> > > >  

> > > > 

> > > 

> > > Reviewed-by: Hannes Reinecke <hare@suse.de>

> > 

> > Hey, you don't get to do that ... I asked you to figure out why

> > we're missing an ahd_restore_modes().  Removing the

> > ahd_save_modes() is cosmetic: it gets rid of a warning but doesn't

> > fix the problem.  I'd rather keep the warning until the problem is

> > fixed and the problem is we need a mode save/restore around the

> > ahd_set_modes() which is only partially implemented in this

> > function.

> 

> I had a look.  Traced it back to the dawn of time (time == Git), then

> delved even further back by downloading and trawling through ~10-15

> tarballs.  It looks as though drivers/scsi/aic7xxx/aic79xx_osm.c was

> upstreamed in v2.5.60, nearly 20 years ago.  'saved_modes' has been

> unused since at least then.  If no one has complained in 2 decades,

> I'd say it probably isn't an issue worth perusing.


Well, we have what looks like a fix now.  The reason it matters is that
 if the bus is not in AHD_MODE_SCSI when the routine is called, it ends
up being in a wrong state when the routine exits, so you abort one
command and screw up another.  Ultimately I bet escalation to bus reset
fixes it, so everything appears to work, but it might have worked a lot
better if the original mode were restored.

This is error handling code, so most installations run just fine
without ever invoking it.

James
Hannes Reinecke July 15, 2020, 5:58 a.m. UTC | #5
On 7/14/20 11:39 PM, Lee Jones wrote:
> On Tue, 14 Jul 2020, James Bottomley wrote:

> 

>> On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:

>>> On 7/13/20 10:00 AM, Lee Jones wrote:

>>>> Haven't been used since 2006.

>>>>

>>>> Fixes the following W=1 kernel build warning(s):

>>>>

>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c: In function

>>>> ‘ahd_linux_queue_abort_cmd’:

>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

>>>> ‘saved_modes’ set but not used [-Wunused-but-set-variable]

>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

>>>> ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

>>>>

>>>> Cc: Hannes Reinecke <hare@suse.com>

>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>

>>>> ---

>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

>>>>   1 file changed, 4 deletions(-)

>>>>

>>>> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>> b/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>> index 3782a20d58885..140c4e74ddd7e 100644

>>>> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>> @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

>>>> *cmd)

>>>>   	u_int  saved_scbptr;

>>>>   	u_int  active_scbptr;

>>>>   	u_int  last_phase;

>>>> -	u_int  saved_scsiid;

>>>>   	u_int  cdb_byte;

>>>>   	int    retval;

>>>>   	int    was_paused;

>>>>   	int    paused;

>>>>   	int    wait;

>>>>   	int    disconnected;

>>>> -	ahd_mode_state saved_modes;

>>>>   	unsigned long flags;

>>>>   

>>>>   	pending_scb = NULL;

>>>> @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

>>>> *cmd)

>>>>   		goto done;

>>>>   	}

>>>>   

>>>> -	saved_modes = ahd_save_modes(ahd);

>>>>   	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

>>>>   	last_phase = ahd_inb(ahd, LASTPHASE);

>>>>   	saved_scbptr = ahd_get_scbptr(ahd);

>>>> @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

>>>> *cmd)

>>>>   	 * passed in command.  That command is currently active on

>>>> the

>>>>   	 * bus or is in the disconnected state.

>>>>   	 */

>>>> -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

>>>>   	if (last_phase != P_BUSFREE

>>>>   	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

>>>>   

>>>>

>>>

>>> Reviewed-by: Hannes Reinecke <hare@suse.de>

>>

>> Hey, you don't get to do that ... I asked you to figure out why we're

>> missing an ahd_restore_modes().  Removing the ahd_save_modes() is

>> cosmetic: it gets rid of a warning but doesn't fix the problem.  I'd

>> rather keep the warning until the problem is fixed and the problem is

>> we need a mode save/restore around the ahd_set_modes() which is only

>> partially implemented in this function.

> 

> I had a look.  Traced it back to the dawn of time (time == Git), then

> delved even further back by downloading and trawling through ~10-15

> tarballs.  It looks as though drivers/scsi/aic7xxx/aic79xx_osm.c was

> upstreamed in v2.5.60, nearly 20 years ago.  'saved_modes' has been

> unused since at least then.  If no one has complained in 2 decades,

> I'd say it probably isn't an issue worth perusing.

> 

That's not really the point; this function is the first stage of error 
recovery. And the only real way of exercising this is to inject a 
command timeout, which is nearly impossible without dedicated hardware.
So this function will have a very limited exposure, but nevertheless a 
quite crucial function.
Hence I'm not quite agree with your reasoning, and rather would have it 
fixed.
But as we're having an alternative fix now, it might be best if you 
could drop it from your patchset and we'll fix it separately.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
Lee Jones July 15, 2020, 6:29 a.m. UTC | #6
On Tue, 14 Jul 2020, James Bottomley wrote:

> On Tue, 2020-07-14 at 22:39 +0100, Lee Jones wrote:

> > On Tue, 14 Jul 2020, James Bottomley wrote:

> > 

> > > On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:

> > > > On 7/13/20 10:00 AM, Lee Jones wrote:

> > > > > Haven't been used since 2006.

> > > > > 

> > > > > Fixes the following W=1 kernel build warning(s):

> > > > > 

> > > > >  drivers/scsi/aic7xxx/aic79xx_osm.c: In function

> > > > > ‘ahd_linux_queue_abort_cmd’:

> > > > >  drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

> > > > > ‘saved_modes’ set but not used [-Wunused-but-set-variable]

> > > > >  drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

> > > > > ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

> > > > > 

> > > > > Cc: Hannes Reinecke <hare@suse.com>

> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > > > > ---

> > > > >  drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

> > > > >  1 file changed, 4 deletions(-)

> > > > > 

> > > > > diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > index 3782a20d58885..140c4e74ddd7e 100644

> > > > > --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct

> > > > > scsi_cmnd

> > > > > *cmd)

> > > > >  	u_int  saved_scbptr;

> > > > >  	u_int  active_scbptr;

> > > > >  	u_int  last_phase;

> > > > > -	u_int  saved_scsiid;

> > > > >  	u_int  cdb_byte;

> > > > >  	int    retval;

> > > > >  	int    was_paused;

> > > > >  	int    paused;

> > > > >  	int    wait;

> > > > >  	int    disconnected;

> > > > > -	ahd_mode_state saved_modes;

> > > > >  	unsigned long flags;

> > > > >  

> > > > >  	pending_scb = NULL;

> > > > > @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct

> > > > > scsi_cmnd

> > > > > *cmd)

> > > > >  		goto done;

> > > > >  	}

> > > > >  

> > > > > -	saved_modes = ahd_save_modes(ahd);

> > > > >  	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

> > > > >  	last_phase = ahd_inb(ahd, LASTPHASE);

> > > > >  	saved_scbptr = ahd_get_scbptr(ahd);

> > > > > @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct

> > > > > scsi_cmnd

> > > > > *cmd)

> > > > >  	 * passed in command.  That command is currently

> > > > > active on

> > > > > the

> > > > >  	 * bus or is in the disconnected state.

> > > > >  	 */

> > > > > -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

> > > > >  	if (last_phase != P_BUSFREE

> > > > >  	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

> > > > >  

> > > > > 

> > > > 

> > > > Reviewed-by: Hannes Reinecke <hare@suse.de>

> > > 

> > > Hey, you don't get to do that ... I asked you to figure out why

> > > we're missing an ahd_restore_modes().  Removing the

> > > ahd_save_modes() is cosmetic: it gets rid of a warning but doesn't

> > > fix the problem.  I'd rather keep the warning until the problem is

> > > fixed and the problem is we need a mode save/restore around the

> > > ahd_set_modes() which is only partially implemented in this

> > > function.

> > 

> > I had a look.  Traced it back to the dawn of time (time == Git), then

> > delved even further back by downloading and trawling through ~10-15

> > tarballs.  It looks as though drivers/scsi/aic7xxx/aic79xx_osm.c was

> > upstreamed in v2.5.60, nearly 20 years ago.  'saved_modes' has been

> > unused since at least then.  If no one has complained in 2 decades,

> > I'd say it probably isn't an issue worth perusing.

> 

> Well, we have what looks like a fix now.  The reason it matters is that

>  if the bus is not in AHD_MODE_SCSI when the routine is called, it ends

> up being in a wrong state when the routine exits, so you abort one

> command and screw up another.  Ultimately I bet escalation to bus reset

> fixes it, so everything appears to work, but it might have worked a lot

> better if the original mode were restored.

> 

> This is error handling code, so most installations run just fine

> without ever invoking it.


Understood.

Obviously a *real* fix is better than this warning suppression one.  I
was just explaining that, try as I might, I couldn't find any
non-broken code to use to use as a reference to author a proper fix
for this.

This being the first mail in my inbox, so I don't know if the proper
fix you reference above was a) sent as a patch and/or b) sent to me,
but it's great news that you have one either way.

Please drop this patch.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones July 15, 2020, 6:30 a.m. UTC | #7
On Wed, 15 Jul 2020, Hannes Reinecke wrote:

> On 7/14/20 11:39 PM, Lee Jones wrote:

> > On Tue, 14 Jul 2020, James Bottomley wrote:

> > 

> > > On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:

> > > > On 7/13/20 10:00 AM, Lee Jones wrote:

> > > > > Haven't been used since 2006.

> > > > > 

> > > > > Fixes the following W=1 kernel build warning(s):

> > > > > 

> > > > >   drivers/scsi/aic7xxx/aic79xx_osm.c: In function

> > > > > ‘ahd_linux_queue_abort_cmd’:

> > > > >   drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

> > > > > ‘saved_modes’ set but not used [-Wunused-but-set-variable]

> > > > >   drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

> > > > > ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

> > > > > 

> > > > > Cc: Hannes Reinecke <hare@suse.com>

> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>

> > > > > ---

> > > > >   drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

> > > > >   1 file changed, 4 deletions(-)

> > > > > 

> > > > > diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > index 3782a20d58885..140c4e74ddd7e 100644

> > > > > --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

> > > > > @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > > > > *cmd)

> > > > >   	u_int  saved_scbptr;

> > > > >   	u_int  active_scbptr;

> > > > >   	u_int  last_phase;

> > > > > -	u_int  saved_scsiid;

> > > > >   	u_int  cdb_byte;

> > > > >   	int    retval;

> > > > >   	int    was_paused;

> > > > >   	int    paused;

> > > > >   	int    wait;

> > > > >   	int    disconnected;

> > > > > -	ahd_mode_state saved_modes;

> > > > >   	unsigned long flags;

> > > > >   	pending_scb = NULL;

> > > > > @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > > > > *cmd)

> > > > >   		goto done;

> > > > >   	}

> > > > > -	saved_modes = ahd_save_modes(ahd);

> > > > >   	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

> > > > >   	last_phase = ahd_inb(ahd, LASTPHASE);

> > > > >   	saved_scbptr = ahd_get_scbptr(ahd);

> > > > > @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct scsi_cmnd

> > > > > *cmd)

> > > > >   	 * passed in command.  That command is currently active on

> > > > > the

> > > > >   	 * bus or is in the disconnected state.

> > > > >   	 */

> > > > > -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

> > > > >   	if (last_phase != P_BUSFREE

> > > > >   	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

> > > > > 

> > > > 

> > > > Reviewed-by: Hannes Reinecke <hare@suse.de>

> > > 

> > > Hey, you don't get to do that ... I asked you to figure out why we're

> > > missing an ahd_restore_modes().  Removing the ahd_save_modes() is

> > > cosmetic: it gets rid of a warning but doesn't fix the problem.  I'd

> > > rather keep the warning until the problem is fixed and the problem is

> > > we need a mode save/restore around the ahd_set_modes() which is only

> > > partially implemented in this function.

> > 

> > I had a look.  Traced it back to the dawn of time (time == Git), then

> > delved even further back by downloading and trawling through ~10-15

> > tarballs.  It looks as though drivers/scsi/aic7xxx/aic79xx_osm.c was

> > upstreamed in v2.5.60, nearly 20 years ago.  'saved_modes' has been

> > unused since at least then.  If no one has complained in 2 decades,

> > I'd say it probably isn't an issue worth perusing.

> > 

> That's not really the point; this function is the first stage of error

> recovery. And the only real way of exercising this is to inject a command

> timeout, which is nearly impossible without dedicated hardware.

> So this function will have a very limited exposure, but nevertheless a quite

> crucial function.

> Hence I'm not quite agree with your reasoning, and rather would have it

> fixed.


100% agree.

> But as we're having an alternative fix now, it might be best if you could

> drop it from your patchset and we'll fix it separately.


Sounds good.  Thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Hannes Reinecke July 21, 2020, 6:38 a.m. UTC | #8
On 7/15/20 1:03 AM, James Bottomley wrote:
> On Tue, 2020-07-14 at 22:39 +0100, Lee Jones wrote:

>> On Tue, 14 Jul 2020, James Bottomley wrote:

>>

>>> On Tue, 2020-07-14 at 09:46 +0200, Hannes Reinecke wrote:

>>>> On 7/13/20 10:00 AM, Lee Jones wrote:

>>>>> Haven't been used since 2006.

>>>>>

>>>>> Fixes the following W=1 kernel build warning(s):

>>>>>

>>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c: In function

>>>>> ‘ahd_linux_queue_abort_cmd’:

>>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c:2155:17: warning: variable

>>>>> ‘saved_modes’ set but not used [-Wunused-but-set-variable]

>>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c:2148:9: warning: variable

>>>>> ‘saved_scsiid’ set but not used [-Wunused-but-set-variable]

>>>>>

>>>>> Cc: Hannes Reinecke <hare@suse.com>

>>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>

>>>>> ---

>>>>>   drivers/scsi/aic7xxx/aic79xx_osm.c | 4 ----

>>>>>   1 file changed, 4 deletions(-)

>>>>>

>>>>> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>>> b/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>>> index 3782a20d58885..140c4e74ddd7e 100644

>>>>> --- a/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>>> +++ b/drivers/scsi/aic7xxx/aic79xx_osm.c

>>>>> @@ -2141,14 +2141,12 @@ ahd_linux_queue_abort_cmd(struct

>>>>> scsi_cmnd

>>>>> *cmd)

>>>>>   	u_int  saved_scbptr;

>>>>>   	u_int  active_scbptr;

>>>>>   	u_int  last_phase;

>>>>> -	u_int  saved_scsiid;

>>>>>   	u_int  cdb_byte;

>>>>>   	int    retval;

>>>>>   	int    was_paused;

>>>>>   	int    paused;

>>>>>   	int    wait;

>>>>>   	int    disconnected;

>>>>> -	ahd_mode_state saved_modes;

>>>>>   	unsigned long flags;

>>>>>   

>>>>>   	pending_scb = NULL;

>>>>> @@ -2239,7 +2237,6 @@ ahd_linux_queue_abort_cmd(struct

>>>>> scsi_cmnd

>>>>> *cmd)

>>>>>   		goto done;

>>>>>   	}

>>>>>   

>>>>> -	saved_modes = ahd_save_modes(ahd);

>>>>>   	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);

>>>>>   	last_phase = ahd_inb(ahd, LASTPHASE);

>>>>>   	saved_scbptr = ahd_get_scbptr(ahd);

>>>>> @@ -2257,7 +2254,6 @@ ahd_linux_queue_abort_cmd(struct

>>>>> scsi_cmnd

>>>>> *cmd)

>>>>>   	 * passed in command.  That command is currently

>>>>> active on

>>>>> the

>>>>>   	 * bus or is in the disconnected state.

>>>>>   	 */

>>>>> -	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);

>>>>>   	if (last_phase != P_BUSFREE

>>>>>   	    && SCB_GET_TAG(pending_scb) == active_scbptr) {

>>>>>   

>>>>>

>>>>

>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>

>>>

>>> Hey, you don't get to do that ... I asked you to figure out why

>>> we're missing an ahd_restore_modes().  Removing the

>>> ahd_save_modes() is cosmetic: it gets rid of a warning but doesn't

>>> fix the problem.  I'd rather keep the warning until the problem is

>>> fixed and the problem is we need a mode save/restore around the

>>> ahd_set_modes() which is only partially implemented in this

>>> function.

>>

>> I had a look.  Traced it back to the dawn of time (time == Git), then

>> delved even further back by downloading and trawling through ~10-15

>> tarballs.  It looks as though drivers/scsi/aic7xxx/aic79xx_osm.c was

>> upstreamed in v2.5.60, nearly 20 years ago.  'saved_modes' has been

>> unused since at least then.  If no one has complained in 2 decades,

>> I'd say it probably isn't an issue worth perusing.

> 

> Well, we have what looks like a fix now.  The reason it matters is that

>   if the bus is not in AHD_MODE_SCSI when the routine is called, it ends

> up being in a wrong state when the routine exits, so you abort one

> command and screw up another.  Ultimately I bet escalation to bus reset

> fixes it, so everything appears to work, but it might have worked a lot

> better if the original mode were restored.

> 

> This is error handling code, so most installations run just fine

> without ever invoking it.

> 

And since you mention it, it might also explain why every time this 
routine got invoked it failed to fixup anything and got escalated to bus 
reset.
(And yes, I _did_ have some customer issues with aic79xx EH escalations 
over the years).

Testing will be tricky, though, as you'd have to inject timeouts onto 
the parallel bus or device.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
diff mbox series

Patch

diff --git a/drivers/scsi/aic7xxx/aic79xx_osm.c b/drivers/scsi/aic7xxx/aic79xx_osm.c
index 3782a20d58885..140c4e74ddd7e 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -2141,14 +2141,12 @@  ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)
 	u_int  saved_scbptr;
 	u_int  active_scbptr;
 	u_int  last_phase;
-	u_int  saved_scsiid;
 	u_int  cdb_byte;
 	int    retval;
 	int    was_paused;
 	int    paused;
 	int    wait;
 	int    disconnected;
-	ahd_mode_state saved_modes;
 	unsigned long flags;
 
 	pending_scb = NULL;
@@ -2239,7 +2237,6 @@  ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)
 		goto done;
 	}
 
-	saved_modes = ahd_save_modes(ahd);
 	ahd_set_modes(ahd, AHD_MODE_SCSI, AHD_MODE_SCSI);
 	last_phase = ahd_inb(ahd, LASTPHASE);
 	saved_scbptr = ahd_get_scbptr(ahd);
@@ -2257,7 +2254,6 @@  ahd_linux_queue_abort_cmd(struct scsi_cmnd *cmd)
 	 * passed in command.  That command is currently active on the
 	 * bus or is in the disconnected state.
 	 */
-	saved_scsiid = ahd_inb(ahd, SAVED_SCSIID);
 	if (last_phase != P_BUSFREE
 	    && SCB_GET_TAG(pending_scb) == active_scbptr) {