On Wed, 2016-10-12 at 02:25 -0300, Gabriel Krisman Bertazi wrote: > 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 > > Finally, should we have a NORETRY_UA flag to allow callers to disable > this mechanism? I think not: let's use retries for this. Allowing no retries would be the signal that you want the UA condition returned. > Link: https://patchwork.kernel.org/patch/9336763/ > Suggested-by: Brian King <brking@xxxxxxxxxxxxxxxxxx> > Suggested-by: James Bottomley <jejb@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxxxxxxx> > --- > drivers/scsi/scsi_lib.c | 47 > +++++++++++++++++++++++++++++++++++++++++----- > include/scsi/scsi_common.h | 9 +++++++++ > 2 files changed, 51 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index c71344aebdbb..a4af411de2a4 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -47,6 +47,9 @@ struct kmem_cache *scsi_sdb_cache; > */ > #define SCSI_QUEUE_DELAY 3 > > +/* Maximum number of retries when a scsi command triggers an Unit > Attention. */ > +#define UNIT_ATTENTION_RETRIES 5 We would also tick down the passed in retries so you don't need this artificial setting. > static void > scsi_set_blocked(struct scsi_cmnd *cmd, int reason) > { > @@ -164,7 +167,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int > reason) > __scsi_queue_insert(cmd, reason, 1); > } > /** > - * scsi_execute - insert request and wait for the result > + * __scsi_execute - insert request and wait for the result > * @sdev: scsi device > * @cmd: scsi command > * @data_direction: data direction > @@ -179,10 +182,10 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, > int reason) > * returns the req->errors value which is the scsi_cmnd result > * field. > */ > -int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > - int data_direction, void *buffer, unsigned bufflen, > - unsigned char *sense, int timeout, int retries, u64 > flags, > - int *resid) > +int __scsi_execute(struct scsi_device *sdev, const unsigned char > *cmd, > + int data_direction, void *buffer, unsigned > bufflen, > + unsigned char *sense, int timeout, int retries, > u64 flags, > + int *resid) > { > struct request *req; > int write = (data_direction == DMA_TO_DEVICE); > @@ -227,6 +230,40 @@ int scsi_execute(struct scsi_device *sdev, const > unsigned char *cmd, > > return ret; > } > + > +int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > + int data_direction, void *buffer, unsigned bufflen, > + unsigned char *sense, int timeout, int retries, u64 > flags, > + int *resid) > +{ > + int result; > + int retry = UNIT_ATTENTION_RETRIES; > + bool priv_sense = false; > + > + if (!sense) { > + sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO); > + priv_sense = true; > + if (!sense) > + return DRIVER_ERROR << 24; > + } > + > + while (retry--) { > + result = __scsi_execute(sdev, cmd, data_direction, > buffer, > + bufflen, sense, timeout, > retries, > + flags, resid); OK, so really this isn't what you want, because blk_execute_req may have used several of your retries, so you now get a maximum possible set of retries at UNIT_ATTENTION_RETRIES*retries. You need to start from the returned req->retries, which probably means this loop needs to be inside __scsi_execute. James > + > + if (!scsi_sense_unit_attention(sense)) > + break; > + > + if (retry) > + memset(sense, 0, SCSI_SENSE_BUFFERSIZE); > + } > + > + if (priv_sense) > + kfree(sense); > + > + return result; > +} > EXPORT_SYMBOL(scsi_execute); > > int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned > char *cmd, > diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h > index 20bf7eaef05a..747b632d5b57 100644 > --- a/include/scsi/scsi_common.h > +++ b/include/scsi/scsi_common.h > @@ -58,6 +58,15 @@ static inline bool scsi_sense_valid(const struct > scsi_sense_hdr *sshdr) > return (sshdr->response_code & 0x70) == 0x70; > } > > +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))); > +} > + > extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len, > struct scsi_sense_hdr *sshdr); > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
