Re: [PATCH] scsi_dh_alua: remove check for ASC 24h when ILLEGAL_REQUEST returned on RTPG w/extended header

From: "Ewan D. Milne" <emilne@redhat.com>
To: dgilbert@interlog.com, linux-scsi@vger.kernel.org
Cc: hare@suse.de
Subject: Re: [PATCH] scsi_dh_alua: remove check for ASC 24h when ILLEGAL_REQUEST returned on RTPG w/extended header
Date: Thu, 01 Apr 2021 08:21:46 -0400	[thread overview]
Message-ID: <8470762d5f9840f7d80389be3f89f7f53989a400.camel@redhat.com> (raw)
In-Reply-To: <2c505c60-9ba0-9ce6-46a6-e6edea2ada03@interlog.com>

On Thu, 2021-04-01 at 00:24 -0400, Douglas Gilbert wrote:
> On 2021-03-31 4:11 p.m., Ewan D. Milne wrote:
> > Some arrays return ILLEGAL_REQUEST with ASC 00h if they don't
> > support the
> > extended header, so remove the check for INVALID FIELD IN CDB.
> 
> Wow. Referring to the 18 byte sense buffer as an extended header
> sounds
> like it comes from the transition of SCSI-1 to SCSI-2, about 30 years
> ago.
> Those arrays need to be named and shamed.

Sorry, I was referring to the RTPG command, not the sense header.

        cdb[0] = MAINTENANCE_IN;
        if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
                cdb[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
        else
                cdb[1] = MI_REPORT_TARGET_PGS;

The array returned an error when the MI_EXT_HDR_PARAM_FMT bit was set
but worked fine without it.

It was an older array, it worked with older kernels until we removed
the ability to detach the device handler.  So RTPG needs to work.

-Ewan

> 
> Doug Gilbert
> Hmm, it is April first ...
> 
> > Signed-off-by: Ewan D. Milne <emilne@redhat.com>
> > ---
> >   drivers/scsi/device_handler/scsi_dh_alua.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> > b/drivers/scsi/device_handler/scsi_dh_alua.c
> > index e42390333c6e..c4c2f23cf79f 100644
> > --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> > @@ -587,10 +587,11 @@ static int alua_rtpg(struct scsi_device
> > *sdev, struct alua_port_group *pg)
> >   		 * even though it shouldn't according to T10.
> >   		 * The retry without rtpg_ext_hdr_req set
> >   		 * handles this.
> > +		 * Note:  some arrays return a sense key of
> > ILLEGAL_REQUEST
> > +		 * with ASC 00h if they don't support the extended
> > header.
> >   		 */
> >   		if (!(pg->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
> > -		    sense_hdr.sense_key == ILLEGAL_REQUEST &&
> > -		    sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {
> > +		    sense_hdr.sense_key == ILLEGAL_REQUEST) {
> >   			pg->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
> >   			goto retry;
> >   		}
> > 
> 
> 