[PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Thorsten Blum]

Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
only update the local variable 'residue' if needed.

Update the rts51x_read_status() function signature accordingly.

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

diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index 7dea28c2b8ee..8a4d7c0f2662 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;
@@ -260,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;
@@ -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

Re: [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Greg Kroah-Hartman]

On Tue, Aug 12, 2025 at 04:43:58PM +0200, Thorsten Blum wrote:
> Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
> only update the local variable 'residue' if needed.

But why?

> Update the rts51x_read_status() function signature accordingly.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/usb/storage/realtek_cr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Have you tested this change?  What caused this to be needed?

thanks,

greg k-h

Re: [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Thorsten Blum]

Hi Greg,

On 12. Aug 2025, at 17:00, Greg Kroah-Hartman wrote:
> On Tue, Aug 12, 2025 at 04:43:58PM +0200, Thorsten Blum wrote:
>> Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
>> only update the local variable 'residue' if needed.
> 
> But why?

The parameter 'buf_len' is never negative, so using 'unsigned int' is
semantically better. Since both 'buf_len' and 'residue' are now unsigned
integers, we can directly compare them without the additional
'if (residue)' check.

Unnecessarily reassigning 'residue' to itself has also been removed.

> Update the rts51x_read_status() function signature accordingly.
>> 
>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>> ---
>> drivers/usb/storage/realtek_cr.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> Have you tested this change?  What caused this to be needed?

I've only compile-tested it due to lack of hardware.

I came across this because Coccinelle/coccicheck suggested using min()
instead of the ternary operator, but I realized it could be simplified
in a cleaner way.

Thanks,
Thorsten

Re: [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Alan Stern]

On Tue, Aug 12, 2025 at 04:43:58PM +0200, Thorsten Blum wrote:
> Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
> only update the local variable 'residue' if needed.
> 
> Update the rts51x_read_status() function signature accordingly.

That last part isn't really necessary, is it?  It doesn't make the code 
any clearer, less buggy, or quicker to execute.

> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/usb/storage/realtek_cr.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
> index 7dea28c2b8ee..8a4d7c0f2662 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;
> @@ -260,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;

This really has nothing at all to do with whether buf_len is a signed 
quantity -- it should never be negative.  (And I have no idea why the 
original code includes that test for residue being nonzero.)

Much more serious is something you didn't change: Just above these lines 
it says:

	residue = bcs->Residue;

It should say:

	residue = le32_to_cpu(bcs->Residue);

Alan Stern

Re: [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Thorsten Blum]

Hi Alan,

On 12. Aug 2025, at 22:06, Alan Stern wrote:
> On Tue, Aug 12, 2025 at 04:43:58PM +0200, Thorsten Blum wrote:
>> Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
>> only update the local variable 'residue' if needed.
>> 
>> Update the rts51x_read_status() function signature accordingly.
> 
> That last part isn't really necessary, is it?  It doesn't make the code 
> any clearer, less buggy, or quicker to execute.

It's mostly for consistency because the parameter 'len' is used to call
rts51x_bulk_transport() which now expects an unsigned integer. I'd still
argue that it makes the code and the function signature a bit clearer
because now the type communicates that 'len' cannot be negative.

>> 	- if (residue)
>> 		- residue = residue < buf_len ? residue : buf_len;
>> 	+ if (residue > buf_len)
>> 		+ residue = buf_len;
> 
> This really has nothing at all to do with whether buf_len is a signed 
> quantity -- it should never be negative.  (And I have no idea why the 
> original code includes that test for residue being nonzero.)

I agree with "it should never be negative" and ideally the type should
reflect this if possible.

It's also easier to reason about the code when comparing two unsigned
integers than having to think about implicit type conversion.

> Much more serious is something you didn't change: Just above these lines 
> it says:
> 
> 	residue = bcs->Residue;
> 
> It should say:
> 
> 	residue = le32_to_cpu(bcs->Residue);

That should probably be another patch, no?

Thanks,
Thorsten

Re: [usb-storage] Re: [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Alan Stern]

On Tue, Aug 12, 2025 at 11:28:56PM +0200, Thorsten Blum wrote:
> Hi Alan,
> 
> On 12. Aug 2025, at 22:06, Alan Stern wrote:
> > On Tue, Aug 12, 2025 at 04:43:58PM +0200, Thorsten Blum wrote:
> >> Change the function parameter 'buf_len' from 'int' to 'unsigned int' and
> >> only update the local variable 'residue' if needed.
> >> 
> >> Update the rts51x_read_status() function signature accordingly.
> > 
> > That last part isn't really necessary, is it?  It doesn't make the code 
> > any clearer, less buggy, or quicker to execute.
> 
> It's mostly for consistency because the parameter 'len' is used to call
> rts51x_bulk_transport() which now expects an unsigned integer. I'd still
> argue that it makes the code and the function signature a bit clearer
> because now the type communicates that 'len' cannot be negative.
> 
> >> 	- if (residue)
> >> 		- residue = residue < buf_len ? residue : buf_len;
> >> 	+ if (residue > buf_len)
> >> 		+ residue = buf_len;
> > 
> > This really has nothing at all to do with whether buf_len is a signed 
> > quantity -- it should never be negative.  (And I have no idea why the 
> > original code includes that test for residue being nonzero.)
> 
> I agree with "it should never be negative" and ideally the type should
> reflect this if possible.
> 
> It's also easier to reason about the code when comparing two unsigned
> integers than having to think about implicit type conversion.
> 
> > Much more serious is something you didn't change: Just above these lines 
> > it says:
> > 
> > 	residue = bcs->Residue;
> > 
> > It should say:
> > 
> > 	residue = le32_to_cpu(bcs->Residue);
> 
> That should probably be another patch, no?

So we're really talking about three separate things:

	Making buf_len and len unsigned;

	Simplifying the calculation of residue;

	Using the correct byte order for bcs->Residue.

The last one fixes a real bug; the other two are very minor by 
comparison.  Regardless, they should be in three separate patches.

If you would like to submit three new patches, please do.

Alan Stern

Re: [usb-storage] [PATCH] usb: storage: realtek_cr: Simplify rts51x_bulk_transport()

[Thorsten Blum]

On 13. Aug 2025, at 03:38, Alan Stern wrote:
> If you would like to submit three new patches, please do.

I just submitted this as three separate patches:

https://lore.kernel.org/lkml/20250813101249.158270-2-thorsten.blum@linux.dev/

Thanks,
Thorsten