On 01:20 Mon 24 Oct , Gabriel Krisman Bertazi wrote:
> James,
>
> I fixed the things you pointed out on the previous review. As
> discussed, I didn't change the code to reuse the request yet. We can
> follow up on that later.
>
> Thanks,
>
> >8
>
> Usually, re-sending the SCSI command is enough to recover from a Unit
> Attention (UA). This adds a generic retry code to the SCSI command path
> in case of an UA, before giving up and returning the error condition to
> the caller.
>
> I added the UA verification into scsi_execute instead of
> scsi_execute_req because there are at least a few callers that invoke
> scsi_execute directly and would benefit from the internal UA retry.
> Also, I didn't use scsi_normalize_sense to not duplicate functionality
> with scsi_execute_req_flags. Instead, scsi_execute uses a small helper
> function that verifies only the UA condition directly from the raw sense
> buffer. If this design is not OK, I can refactor to use
> scsi_normalize_sense.
>
> This prevents us from duplicating the retry code in at least a few
> places. In particular, it fixes an issue found in some IBM enclosures,
> in which the device may return an Unit Attention during probe, breaking
> the bind with the ses module:
>
> scsi 1:0:7:0: Failed to get diagnostic page 0x8000002
> scsi 1:0:7:0: Failed to bind enclosure -19
>
> Link: https://patchwork.kernel.org/patch/9336763/
> Suggested-by: Brian King <[email protected]>
> Suggested-by: James Bottomley <[email protected]>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> ---
> drivers/scsi/scsi_lib.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index c71344aebdbb..4f6a91d6ee34 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -163,6 +163,16 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
> {
> __scsi_queue_insert(cmd, reason, 1);
> }
> +
> +static inline bool scsi_sense_unit_attention(const char *sense)
> +{
> + int resp = sense[0] & 0x7f;
> +
> + return ((resp & 0x70) &&
> + ((resp >= 0x72 && (sense[1] & 0xf) == UNIT_ATTENTION) ||
> + (resp < 0x72 && (sense[2] & 0xf) == UNIT_ATTENTION)));
> +}
> +
Just some minor nit-pick. In other places where we check for valid sense
(SCSI_SENSE_VALID(), scsi_sense_valid())
we always check for whether the upper nibble really only contains 0x70
((resp & 0x70) == 0x70).
I also find it strange that we now have 3 different and independent
functions/places that check for valid sense.
Beste Grüße / Best regards,
- Benjamin Block
> /**
> * scsi_execute - insert request and wait for the result
> * @sdev: scsi device
> @@ -187,7 +197,14 @@ int scsi_execute(struct scsi_device *sdev, const
> unsigned char *cmd,
> struct request *req;
> int write = (data_direction == DMA_TO_DEVICE);
> int ret = DRIVER_ERROR << 24;
> + unsigned char sense_buf[SCSI_SENSE_BUFFERSIZE];
>
> + if (!sense) {
> + sense = sense_buf;
> + memset(sense, 0, SCSI_SENSE_BUFFERSIZE);
> + }
> +
> + retry:
> req = blk_get_request(sdev->request_queue, write, __GFP_RECLAIM);
> if (IS_ERR(req))
> return ret;
> @@ -210,6 +227,13 @@ int scsi_execute(struct scsi_device *sdev, const
> unsigned char *cmd,
> */
> blk_execute_rq(req->q, NULL, req, 1);
>
> + if (scsi_sense_unit_attention(sense) && req->retries > 0) {
> + memset(sense, 0, SCSI_SENSE_BUFFERSIZE);
> + retries = req->retries - 1;
> + blk_put_request(req);
> + goto retry;
> + }
> +
> /*
> * Some devices (USB mass-storage in particular) may transfer
> * garbage data together with a residue indicating that the data
> --
> 2.7.4
--
Linux on z Systems Development / IBM Systems & Technology Group
IBM Deutschland Research & Development GmbH
Vorsitz. AufsR.: Martina Koederitz / Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
