by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] Set 3: Fix another set of SCSI related W=1 warnings

On Thu, 16 Jul 2020, Lee Jones wrote:

> On Wed, 15 Jul 2020, Martin K. Petersen wrote:
>
> > On Mon, 13 Jul 2020 08:59:37 +0100, Lee Jones wrote:
> >
> > > This set is part of a larger effort attempting to clean-up W=1
> > > kernel builds, which are currently overwhelmingly riddled with
> > > niggly little warnings.
> > >
> > > Slowly working through the SCSI related ones. There are many.
> > >
> > > This brings the total of W=1 SCSI wanings from 1690 in v5.8-rc1 to 1109.
> > >
> > > [...]
> >
> > [01/24] scsi: aacraid: Repair two kerneldoc headers
> > https://git.kernel.org/mkp/scsi/c/b115958d91f5
> > [02/24] scsi: aacraid: Fix a few kerneldoc issues
> > https://git.kernel.org/mkp/scsi/c/cf93fffac261
> > [03/24] scsi: aacraid: Fix logical bug when !DBG
> > https://git.kernel.org/mkp/scsi/c/2fee77e5b820
> > [04/24] scsi: aacraid: Remove unused variable 'status'
> > https://git.kernel.org/mkp/scsi/c/0123c7c62d6c
> > [05/24] scsi: aacraid: Demote partially documented function header
> > https://git.kernel.org/mkp/scsi/c/71aa4d3e0e78
> > [06/24] scsi: aic94xx: Document 'lseq' and repair asd_update_port_links() header
> > https://git.kernel.org/mkp/scsi/c/966fdadf6fea
> > [07/24] scsi: aacraid: Fix a bunch of function header issues
> > https://git.kernel.org/mkp/scsi/c/f1134f0eb184
> > [08/24] scsi: aic94xx: Fix a couple of formatting and bitrot issues
> > https://git.kernel.org/mkp/scsi/c/d2e510505006
> > [09/24] scsi: aacraid: Fill in the very parameter descriptions for rx_sync_cmd()
> > https://git.kernel.org/mkp/scsi/c/ae272a95133a
> > [10/24] scsi: pm8001: Provide descriptions for the many undocumented 'attr's
> > https://git.kernel.org/mkp/scsi/c/e1c3e0f8a2ae
> > [11/24] scsi: ipr: Fix a mountain of kerneldoc misdemeanours
> > https://git.kernel.org/mkp/scsi/c/a96099e2c164
> > [12/24] scsi: virtio_scsi: Demote seemingly unintentional kerneldoc header
> > https://git.kernel.org/mkp/scsi/c/e31f2661ff41
> > [13/24] scsi: ipr: Remove a bunch of set but checked variables
> > https://git.kernel.org/mkp/scsi/c/4dc833999e37
> > [14/24] scsi: ipr: Fix struct packed-not-aligned issues
> > https://git.kernel.org/mkp/scsi/c/f3bdc59f9b11
> > [15/24] scsi: myrs: Demote obvious misuse of kerneldoc to standard comment blocks
> > https://git.kernel.org/mkp/scsi/c/8a692fdb1d04
>
> Something wrong with [16/24]?

I see that you reviewed v1. No need to respond, thanks.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

2020-07-21 06:39:41

by Hannes Reinecke

[permalink] [raw]

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

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 <[email protected]>
>>>>> Signed-off-by: Lee Jones <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>
>>>
>>> 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
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer