Hi,
On 13-01-16 22:37, Oliver Neukum wrote:
On Wed, 2016-01-13 at 19:38 +0100, Hans de Goede wrote:
Hi,
On 13-01-16 15:28, Oliver Neukum wrote:
Some devices send response ius when you'd expect a sense iu.
We cannot get away without handling for response ius.
Thanks for working on this!
Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx>
---
drivers/usb/storage/uas.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 48ca9c2..b5f9cb1 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -246,6 +246,27 @@ static void uas_xfer_data(struct urb *urb, struct scsi_cmnd *cmnd,
}
}
+static void uas_evaluate_response_iu(struct response_iu *riu, struct scsi_cmnd *cmnd)
+{
+ u8 response_code = riu->response_code;
+
+ uas_log_cmd_state(cmnd, "response iu", response_code);
Maybe only log in the default case ? For other cases the scsi
subsys should report a more appropriate error, and in some cases
(e.g. lun discovery), the error may be expected.
Isn't that contrary to the purpose of logging?
I mean it is supposed to be like a trace.
But I can sure change it.
uas_log_cmd_state is not for tracing, it is for printing messages
to dmesg when things go (likely) wrong, which is why it is using
PR_INFO. So please move this to the default case, so that we
do not end up spamming dmesg with this in case of expectec / normal
conditions like RC_INCORRECT_LUN.
+ switch(response_code) {
+ case RC_INCORRECT_LUN:
+ cmnd->result = DID_BAD_TARGET << 16;
+ break;
+ case RC_TMF_SUCCEEDED:
+ cmnd->result = DID_OK << 16;
+ break;
+ case RC_TMF_NOT_SUPPORTED:
+ cmnd->result = DID_TARGET_FAILURE << 16;
+ break;
+ default:
+ cmnd->result = DID_ERROR << 16;
+ break;
+ }
+}
+
static void uas_stat_cmplt(struct urb *urb)
{
struct iu *iu = urb->transfer_buffer;
@@ -313,13 +334,8 @@ static void uas_stat_cmplt(struct urb *urb)
uas_xfer_data(urb, cmnd, SUBMIT_DATA_OUT_URB);
break;
case IU_ID_RESPONSE:
- uas_log_cmd_state(cmnd, "unexpected response iu",
- ((struct response_iu *)iu)->response_code);
- /* Error, cancel data transfers */
- data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
- data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
I believe we should still cancel any pending data urbs, at least
for all values except RC_TMF_SUCCEEDED, are you actually seeing
RC_TMF_SUCCEEDED ?
I only see RC_INCORRECT_LUN. Going by the spec that could be
interpreted as actually permitted.
So should I do it unconditionally or just in some cases?
A good questions, for RC_INCORRECT_LUN / RC_TMF_NOT_SUPPORTED you
should definitely still kill the urbs, as those are certain to
not finish normally in that case.
RC_TMF_SUCCEEDED is tricker, I say lets just go with:
if (((struct response_iu *)iu)->response_code != RC_TMF_SUCCEEDED) {
/* Error, cancel data transfers */
data_in_urb = usb_get_urb(cmdinfo->data_in_urb);
data_out_urb = usb_get_urb(cmdinfo->data_out_urb);
}
Thanks & Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
