[PATCH 1/3] usb: storage: realtek_cr: Improve function parameter data types

[PATCH 1/3] usb: storage: realtek_cr: Improve function parameter data types

[Thorsten Blum]

In rts51x_bulk_transport() and rts51x_read_status(), change the function
parameters 'buf_len' and 'len' from 'int' to 'unsigned int' because
their values cannot be negative.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/usb/storage/realtek_cr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index 7dea28c2b8ee..d2c3e3a39693 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -199,7 +199,8 @@ static const struct us_unusual_dev realtek_cr_unusual_dev_list[] = {
 #undef UNUSUAL_DEV
 
 static int rts51x_bulk_transport(struct us_data *us, u8 lun,
-				 u8 *cmd, int cmd_len, u8 *buf, int buf_len,
+				 u8 *cmd, int cmd_len, u8 *buf,
+				 unsigned int buf_len,
 				 enum dma_data_direction dir, int *act_len)
 {
 	struct bulk_cb_wrap *bcb = (struct bulk_cb_wrap *)us->iobuf;
@@ -417,7 +418,7 @@ static int rts51x_write_mem(struct us_data *us, u16 addr, u8 *data, u16 len)
 }
 
 static int rts51x_read_status(struct us_data *us,
-			      u8 lun, u8 *status, int len, int *actlen)
+			      u8 lun, u8 *status, unsigned int len, int *actlen)
 {
 	int retval;
 	u8 cmnd[12] = { 0 };
-- 
2.50.1

[PATCH 2/3] usb: storage: realtek_cr: Simplify residue calculation in rts51x_bulk_transport()

[Thorsten Blum]

Simplify the calculation of 'residue' in rts51x_bulk_transport() and
avoid unnecessarily reassigning 'residue' to itself.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/usb/storage/realtek_cr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index d2c3e3a39693..8a4d7c0f2662 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -261,8 +261,8 @@ static int rts51x_bulk_transport(struct us_data *us, u8 lun,
 	 * try to compute the actual residue, based on how much data
 	 * was really transferred and what the device tells us
 	 */
-	if (residue)
-		residue = residue < buf_len ? residue : buf_len;
+	if (residue > buf_len)
+		residue = buf_len;
 
 	if (act_len)
 		*act_len = buf_len - residue;
-- 
2.50.1

[PATCH 3/3] usb: storage: realtek_cr: Use correct byte order for bcs->Residue

[Thorsten Blum]

Since 'bcs->Residue' has the data type '__le32', we must convert it to
the correct byte order of the CPU using this driver when assigning it to
the local variable 'residue'.

Cc: stable@vger.kernel.org
Fixes: 50a6cb932d5c ("USB: usb_storage: add ums-realtek driver")
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/usb/storage/realtek_cr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index 8a4d7c0f2662..758258a569a6 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -253,7 +253,7 @@ static int rts51x_bulk_transport(struct us_data *us, u8 lun,
 		return USB_STOR_TRANSPORT_ERROR;
 	}
 
-	residue = bcs->Residue;
+	residue = le32_to_cpu(bcs->Residue);
 	if (bcs->Tag != us->tag)
 		return USB_STOR_TRANSPORT_ERROR;
 
-- 
2.50.1

Re: [usb-storage] [PATCH 1/3] usb: storage: realtek_cr: Improve function parameter data types

[Alan Stern]

On Wed, Aug 13, 2025 at 12:12:47PM +0200, Thorsten Blum wrote:
> In rts51x_bulk_transport() and rts51x_read_status(), change the function
> parameters 'buf_len' and 'len' from 'int' to 'unsigned int' because
> their values cannot be negative.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/usb/storage/realtek_cr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> index 7dea28c2b8ee..d2c3e3a39693 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -199,7 +199,8 @@ static const struct us_unusual_dev realtek_cr_unusual_dev_list[] = {
>  #undef UNUSUAL_DEV
>  
>  static int rts51x_bulk_transport(struct us_data *us, u8 lun,
> -				 u8 *cmd, int cmd_len, u8 *buf, int buf_len,
> +				 u8 *cmd, int cmd_len, u8 *buf,
> +				 unsigned int buf_len,
>  				 enum dma_data_direction dir, int *act_len)
>  {
>  	struct bulk_cb_wrap *bcb = (struct bulk_cb_wrap *)us->iobuf;
> @@ -417,7 +418,7 @@ static int rts51x_write_mem(struct us_data *us, u16 addr, u8 *data, u16 len)
>  }
>  
>  static int rts51x_read_status(struct us_data *us,
> -			      u8 lun, u8 *status, int len, int *actlen)
> +			      u8 lun, u8 *status, unsigned int len, int *actlen)
>  {
>  	int retval;
>  	u8 cmnd[12] = { 0 };

I just looked through the original source file.  What about 
rts51x_bulk_transport_special()?  Shouldn't its buf_len parameter also 
be unsigned?

For that matter, what about cmd_len in both routines?

And have you checked the corresponding values in all the other 
usb-storage subdrivers?

As you can see, worrying about the difference between signed and 
unsigned values, when it doesn't really matter, quickly leads to a 
morass.

Alan Stern

Re: [PATCH 2/3] usb: storage: realtek_cr: Simplify residue calculation in rts51x_bulk_transport()

[Alan Stern]

On Wed, Aug 13, 2025 at 12:12:49PM +0200, Thorsten Blum wrote:
> Simplify the calculation of 'residue' in rts51x_bulk_transport() and
> avoid unnecessarily reassigning 'residue' to itself.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/usb/storage/realtek_cr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> index d2c3e3a39693..8a4d7c0f2662 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -261,8 +261,8 @@ static int rts51x_bulk_transport(struct us_data *us, u8 lun,
>  	 * try to compute the actual residue, based on how much data
>  	 * was really transferred and what the device tells us
>  	 */
> -	if (residue)
> -		residue = residue < buf_len ? residue : buf_len;
> +	if (residue > buf_len)
> +		residue = buf_len;
>  
>  	if (act_len)
>  		*act_len = buf_len - residue;

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 3/3] usb: storage: realtek_cr: Use correct byte order for bcs->Residue

[Alan Stern]

On Wed, Aug 13, 2025 at 12:12:51PM +0200, Thorsten Blum wrote:
> Since 'bcs->Residue' has the data type '__le32', we must convert it to
> the correct byte order of the CPU using this driver when assigning it to
> the local variable 'residue'.
> 
> Cc: stable@vger.kernel.org
> Fixes: 50a6cb932d5c ("USB: usb_storage: add ums-realtek driver")
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/usb/storage/realtek_cr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> index 8a4d7c0f2662..758258a569a6 100644
> --- a/drivers/usb/storage/realtek_cr.c
> +++ b/drivers/usb/storage/realtek_cr.c
> @@ -253,7 +253,7 @@ static int rts51x_bulk_transport(struct us_data *us, u8 lun,
>  		return USB_STOR_TRANSPORT_ERROR;
>  	}
>  
> -	residue = bcs->Residue;
> +	residue = le32_to_cpu(bcs->Residue);
>  	if (bcs->Tag != us->tag)
>  		return USB_STOR_TRANSPORT_ERROR;
>  

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 3/3] usb: storage: realtek_cr: Use correct byte order for bcs->Residue

[Greg Kroah-Hartman]

On Wed, Aug 13, 2025 at 12:12:51PM +0200, Thorsten Blum wrote:
> Since 'bcs->Residue' has the data type '__le32', we must convert it to
> the correct byte order of the CPU using this driver when assigning it to
> the local variable 'residue'.
> 
> Cc: stable@vger.kernel.org

When you have a bugfix, don't put it last in the patch series, as that
doesn't make much sense if you want to backport it anywhere, like you
are asking to do here.

Please just send this as a separate patch, and do the cleanups in a
different series.

thanks,

greg k-h

Re: [PATCH 3/3] usb: storage: realtek_cr: Use correct byte order for bcs->Residue

[Thorsten Blum]

On 13. Aug 2025, at 16:39, Greg Kroah-Hartman wrote:
> On Wed, Aug 13, 2025 at 12:12:51PM +0200, Thorsten Blum wrote:
>> Since 'bcs->Residue' has the data type '__le32', we must convert it to
>> the correct byte order of the CPU using this driver when assigning it to
>> the local variable 'residue'.
>> 
>> Cc: stable@vger.kernel.org
> 
> When you have a bugfix, don't put it last in the patch series, as that
> doesn't make much sense if you want to backport it anywhere, like you
> are asking to do here.
> 
> Please just send this as a separate patch, and do the cleanups in a
> different series.

Ok, you can find it here:

https://lore.kernel.org/lkml/20250813145247.184717-3-thorsten.blum@linux.dev/

Thanks,
Thorsten

Re: [usb-storage] [PATCH 1/3] usb: storage: realtek_cr: Improve function parameter data types

[Thorsten Blum]

On 13. Aug 2025, at 15:59, Alan Stern wrote:
> I just looked through the original source file.  What about 
> rts51x_bulk_transport_special()?  Shouldn't its buf_len parameter also 
> be unsigned?
> 
> For that matter, what about cmd_len in both routines?
> 
> And have you checked the corresponding values in all the other 
> usb-storage subdrivers?
> 
> As you can see, worrying about the difference between signed and 
> unsigned values, when it doesn't really matter, quickly leads to a 
> morass.

There are many other instances throughout the kernel where types could
be improved, which is why I originally combined this with the if check
change and limited the data type changes to that scope. Feel free to
skip this one, as it might not be worthwhile as a standalone patch.

Thanks,
Thorsten