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 <brking@xxxxxxxxxxxxxxxxxx> > Suggested-by: James Bottomley <jejb@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxxxxxxx> > --- > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
