[PATCH v2] usb: implement proper subclass protocol translation

[PATCH v2] usb: implement proper subclass protocol translation

[Shi Hao]

Proper protocol translation for old legacy usb devices
to set proper cmd_value.

Earlier protocol translation was insufficient for old usb devices
which was padding all subclasses such as USB_SC_RBC, USB_SC_SCSI
and USB_SC_CYP_ATACB cmd_len to 6 byte value and because it wasn't
complete it had a FIXME comment stating to fix the protocol transl
-ation for those legacy usb devices.

As a result implement proper protocol translation as per their op
code. In addition to the old default value 6 those old legacy
usb-device subclasses needed cmd_len value to 10, 12, and 16
byte also.

Signed-off-by: Shi Hao <i.shihao.999@gmail.com>

---
changes v2:
- Update protocol translation comment
- Add comment for non-legacy subclasses
---
 drivers/usb/storage/transport.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 1aa1bd26c81f..edbada636f6c 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -718,12 +718,26 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)

 		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);

-		/* FIXME: we must do the protocol translation here */
+		/* Handle usb subclass protocol translation */
 		if (us->subclass == USB_SC_RBC || us->subclass == USB_SC_SCSI ||
-				us->subclass == USB_SC_CYP_ATACB)
-			srb->cmd_len = 6;
-		else
+		    us->subclass == USB_SC_CYP_ATACB) {
+			/* Determine cmd_len based on scsi opcode group */
+			if (srb->cmnd[0] <= 0x1F)
+				srb->cmd_len = 6;
+			else if (srb->cmnd[0] <= 0x7F)
+				srb->cmd_len = 10;
+			else if (srb->cmnd[0] <= 0x9F)
+				srb->cmd_len = 16;
+			else if (srb->cmnd[0] <= 0xBF)
+				srb->cmd_len = 12;
+			else if (srb->cmnd[0] <= 0xDF)
+				srb->cmd_len = 16;
+			else
+				srb->cmd_len = 6;
+		} else {
+			/* Use fixed value for non-legacy subclasses */
 			srb->cmd_len = 12;
+		}

 		/* issue the auto-sense command */
 		scsi_set_resid(srb, 0);
--
2.51.0

Re: [PATCH v2] usb: implement proper subclass protocol translation

[Alan Stern]

On Sat, Oct 11, 2025 at 03:09:37PM +0530, Shi Hao wrote:
> Proper protocol translation for old legacy usb devices
> to set proper cmd_value.
> 
> Earlier protocol translation was insufficient for old usb devices
> which was padding all subclasses such as USB_SC_RBC, USB_SC_SCSI
> and USB_SC_CYP_ATACB cmd_len to 6 byte value and because it wasn't
> complete it had a FIXME comment stating to fix the protocol transl
> -ation for those legacy usb devices.
> 
> As a result implement proper protocol translation as per their op
> code. In addition to the old default value 6 those old legacy
> usb-device subclasses needed cmd_len value to 10, 12, and 16
> byte also.
> 
> Signed-off-by: Shi Hao <i.shihao.999@gmail.com>
> 
> ---

I'm sorry that you went through all this work, but it turns out that 
this is almost totally unnecessary.  I should have realized this 
earlier, but I didn't check the context of the patch until now.

At the place where you're changing the code, we know that srb->cmnd[0] 
is always going to be REQUEST_SENSE = 0x03 (read the scsi_eh_prep_cmnd() 
routine in drivers/scsi/scsi_error.c).  And we also know that 
srb->cmnd_len will be set to 6, so there's no need to set it to 6 again.

All that this code needs to do is set srb->cmnd_len to 12 for the 
special subclasses.  Nothing else.  I'm not even sure what the FIXME 
is referring to.

Alan Stern

> changes v2:
> - Update protocol translation comment
> - Add comment for non-legacy subclasses
> ---
>  drivers/usb/storage/transport.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> index 1aa1bd26c81f..edbada636f6c 100644
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -718,12 +718,26 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
> 
>  		scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size);
> 
> -		/* FIXME: we must do the protocol translation here */
> +		/* Handle usb subclass protocol translation */
>  		if (us->subclass == USB_SC_RBC || us->subclass == USB_SC_SCSI ||
> -				us->subclass == USB_SC_CYP_ATACB)
> -			srb->cmd_len = 6;
> -		else
> +		    us->subclass == USB_SC_CYP_ATACB) {
> +			/* Determine cmd_len based on scsi opcode group */
> +			if (srb->cmnd[0] <= 0x1F)
> +				srb->cmd_len = 6;
> +			else if (srb->cmnd[0] <= 0x7F)
> +				srb->cmd_len = 10;
> +			else if (srb->cmnd[0] <= 0x9F)
> +				srb->cmd_len = 16;
> +			else if (srb->cmnd[0] <= 0xBF)
> +				srb->cmd_len = 12;
> +			else if (srb->cmnd[0] <= 0xDF)
> +				srb->cmd_len = 16;
> +			else
> +				srb->cmd_len = 6;
> +		} else {
> +			/* Use fixed value for non-legacy subclasses */
>  			srb->cmd_len = 12;
> +		}
> 
>  		/* issue the auto-sense command */
>  		scsi_set_resid(srb, 0);
> --
> 2.51.0
> 

Re: [PATCH v2] usb: implement proper subclass protocol translation

[ShiHao]

 
> I'm sorry that you went through all this work, but it turns out that 
> this is almost totally unnecessary.  I should have realized this 
> earlier, but I didn't check the context of the patch until now.
> 
> At the place where you're changing the code, we know that srb->cmnd[0] 
> is always going to be REQUEST_SENSE = 0x03 (read the scsi_eh_prep_cmnd() 
> routine in drivers/scsi/scsi_error.c).  And we also know that 
> srb->cmnd_len will be set to 6, so there's no need to set it to 6 again.
> 
> All that this code needs to do is set srb->cmnd_len to 12 for the 
> special subclasses.  Nothing else.  I'm not even sure what the FIXME 
> is referring to.
> 
> Alan Stern
> 
> > -		/* FIXME: we must do the protocol translation here */
> > +		/* Handle usb subclass protocol translation */
> >  		if (us->subclass == USB_SC_RBC || us->subclass == USB_SC_SCSI ||
> > -				us->subclass == USB_SC_CYP_ATACB)
> > -			srb->cmd_len = 6;
> > -		else
> > +		    us->subclass == USB_SC_CYP_ATACB) {
> > +			/* Determine cmd_len based on scsi opcode group */
> > +			if (srb->cmnd[0] <= 0x1F)
> > +				srb->cmd_len = 6;
> > +			else if (srb->cmnd[0] <= 0x7F)
> > +				srb->cmd_len = 10;
> > +			else if (srb->cmnd[0] <= 0x9F)
> > +				srb->cmd_len = 16;
> > +			else if (srb->cmnd[0] <= 0xBF)
> > +				srb->cmd_len = 12;
> > +			else if (srb->cmnd[0] <= 0xDF)
> > +				srb->cmd_len = 16;
> > +			else
> > +				srb->cmd_len = 6;
> > +		} else {
> > +			/* Use fixed value for non-legacy subclasses */
> >  			srb->cmd_len = 12;
> > +		}

Hi alan ,

Well alan thanks for the review . Thank you so much for 
giving your percious time to this matter . I am just a newbie
in the kernel and i was just trying to learn things just to be a
kernel hacker your review has taught me how to submit a good 
commit and write code that is acceptable to the kernel .Thanks 
both greg  and you for reviewing my work . Thank you so much 
looking forward to submit  bug fixes :) . 


thanks 
shihao