Hi James, Thanks for your review. Please see the v2 below. > 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. Hmm, I was aware of that, but I saw there were other places that may have run retries^2 times, like scsi_test_unit_ready and scsi_mode_sense, if I read the code correctly. But, I see your point and I fixed it on v2. I also updated the second patch to rework these cases. Another thing that got me confused is where the blk layer updates req->retries. What do you think about the v2 below? 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 <brking@xxxxxxxxxxxxxxxxxx> Suggested-by: James Bottomley <jejb@xxxxxxxxxxxxxxxxxx> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxxxxxxx> --- drivers/scsi/scsi_lib.c | 27 ++++++++++++++++++++++++--- include/scsi/scsi_common.h | 9 +++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index c71344aebdbb..9c6623abf120 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -187,15 +187,24 @@ 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; + bool priv_sense = false; + if (!sense) { + sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO); + if (!sense) + return ret; + priv_sense = true; + } + + retry: req = blk_get_request(sdev->request_queue, write, __GFP_RECLAIM); if (IS_ERR(req)) - return ret; + goto free_sense; blk_rq_set_block_pc(req); if (bufflen && blk_rq_map_kern(sdev->request_queue, req, buffer, bufflen, __GFP_RECLAIM)) - goto out; + goto put_req; req->cmd_len = COMMAND_SIZE(cmd[0]); memcpy(req->cmd, cmd, req->cmd_len); @@ -210,6 +219,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 @@ -222,9 +238,14 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, if (resid) *resid = req->resid_len; ret = req->errors; - out: + + put_req: blk_put_request(req); + free_sense: + if (priv_sense) + kfree(sense); + return ret; } EXPORT_SYMBOL(scsi_execute); 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); -- 2.7.4 -- 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
