[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 <thorst...@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

[Thorsten Blum]

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

Signed-off-by: Thorsten Blum <thorst...@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

[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: sta...@vger.kernel.org
Fixes: 50a6cb932d5c ("USB: usb_storage: add ums-realtek driver")
Suggested-by: Alan Stern <st...@rowland.harvard.edu>

Signed-off-by: Thorsten Blum <thorst...@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

[Alan Stern]

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

[Alan Stern]

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

[Alan Stern]

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

[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: sta...@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

[Thorsten Blum]

Since 'bcs->Residue' has the data type '__le32', convert it to the

correct byte order of the CPU using this driver when assigning it to
the local variable 'residue'.

Cc: sta...@vger.kernel.org

Fixes: 50a6cb932d5c ("USB: usb_storage: add ums-realtek driver")
Suggested-by: Alan Stern <st...@rowland.harvard.edu>

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

Signed-off-by: Thorsten Blum <thorst...@linux.dev>
---

Resending this as a separate patch for backporting as requested by Greg.
Link to previous patch: https://lore.kernel.org/lkml/20250813101249.158...@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

[Thorsten Blum]

Ok, you can find it here:

https://lore.kernel.org/lkml/20250813145247.184...@linux.dev/

Thanks,
Thorsten

[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