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.
> > + 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?
Regards
Oliver
--
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
