[PATCH v2] usb: host: ehci-q: make qtd_fill() return *unsigned int*

[PATCH v2] usb: host: ehci-q: make qtd_fill() return *unsigned int*

[Sergey Shtylyov]

At the end of qtd_fill(), we assign the 'int count' variable to the 'size_t
length' field of 'struct ehci_qtd' -- which implies a problematic type cast.
Let's make that variable and the function's result *unsigned int* instead...

Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo.

Changes in version 2:
- changed 'u16' to *unsigned int* everywhere, repordering the declarations;
- updated the type of the 'this_qtd_len' local variable qh_urb_transaction();
- rewrote the patch description.

 drivers/usb/host/ehci-q.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: usb/drivers/usb/host/ehci-q.c
===================================================================
--- usb.orig/drivers/usb/host/ehci-q.c
+++ usb/drivers/usb/host/ehci-q.c
@@ -33,12 +33,13 @@
 
 /* fill a qtd, returning how much of the buffer we were able to queue up */
 
-static int
+static unsigned int
 qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
 		  size_t len, int token, int maxpacket)
 {
-	int	i, count;
+	unsigned int count;
 	u64	addr = buf;
+	int	i;
 
 	/* one buffer entry per 4K ... first might be short or unaligned */
 	qtd->hw_buf[0] = cpu_to_hc32(ehci, (u32)addr);
@@ -652,7 +653,7 @@ qh_urb_transaction (
 	 * and may serve as a control status ack
 	 */
 	for (;;) {
-		int this_qtd_len;
+		unsigned int this_qtd_len;
 
 		this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
 				maxpacket);

Re: [PATCH v2] usb: host: ehci-q: make qtd_fill() return *unsigned int*

[Alan Stern]

On Thu, Feb 17, 2022 at 08:30:13PM +0300, Sergey Shtylyov wrote:
> At the end of qtd_fill(), we assign the 'int count' variable to the 'size_t
> length' field of 'struct ehci_qtd' -- which implies a problematic type cast.
> Let's make that variable and the function's result *unsigned int* instead...
> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> ---

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

> This patch is against the 'usb-next' branch of Greg KH's 'usb.git' repo.
> 
> Changes in version 2:
> - changed 'u16' to *unsigned int* everywhere, repordering the declarations;
> - updated the type of the 'this_qtd_len' local variable qh_urb_transaction();
> - rewrote the patch description.
> 
>  drivers/usb/host/ehci-q.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Index: usb/drivers/usb/host/ehci-q.c
> ===================================================================
> --- usb.orig/drivers/usb/host/ehci-q.c
> +++ usb/drivers/usb/host/ehci-q.c
> @@ -33,12 +33,13 @@
>  
>  /* fill a qtd, returning how much of the buffer we were able to queue up */
>  
> -static int
> +static unsigned int
>  qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
>  		  size_t len, int token, int maxpacket)
>  {
> -	int	i, count;
> +	unsigned int count;
>  	u64	addr = buf;
> +	int	i;
>  
>  	/* one buffer entry per 4K ... first might be short or unaligned */
>  	qtd->hw_buf[0] = cpu_to_hc32(ehci, (u32)addr);
> @@ -652,7 +653,7 @@ qh_urb_transaction (
>  	 * and may serve as a control status ack
>  	 */
>  	for (;;) {
> -		int this_qtd_len;
> +		unsigned int this_qtd_len;
>  
>  		this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
>  				maxpacket);