[PATCH] usb: fixing some clang warnings inside usb host drivers

[PATCH] usb: fixing some clang warnings inside usb host drivers

[Julio Faracco]

Clang is reporting some issues related variable values not used and
other issues inside some USB host drivers. This commit removes some
trashes and adds some strategies to mitigate those warnings.

The most important is the maxpacket not checking for zeros inside both
functions qtd_fill(). Even if this variable is always higher than zero,
it should be checked to avoid this kind of verbosity.

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 drivers/usb/host/ehci-dbg.c     | 1 -
 drivers/usb/host/ehci-q.c       | 2 +-
 drivers/usb/host/ohci-dbg.c     | 1 -
 drivers/usb/host/oxu210hp-hcd.c | 2 +-
 4 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
index 0b7f1edd9eec..70b4ff65295a 100644
--- a/drivers/usb/host/ehci-dbg.c
+++ b/drivers/usb/host/ehci-dbg.c
@@ -903,7 +903,6 @@ static ssize_t fill_registers_buffer(struct debug_buffer *buf)
 	temp = scnprintf(next, size, "complete %ld unlink %ld\n",
 		ehci->stats.complete, ehci->stats.unlink);
 	size -= temp;
-	next += temp;
 #endif
 
 done:
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 2cbf4f85bff3..98cb44414e78 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -64,7 +64,7 @@ qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
 		}
 
 		/* short packets may only terminate transfers */
-		if (count != len)
+		if (count != len && maxpacket > 0)
 			count -= (count % maxpacket);
 	}
 	qtd->hw_token = cpu_to_hc32(ehci, (count << 16) | token);
diff --git a/drivers/usb/host/ohci-dbg.c b/drivers/usb/host/ohci-dbg.c
index 4f267dc93882..6fc9c46ffe3c 100644
--- a/drivers/usb/host/ohci-dbg.c
+++ b/drivers/usb/host/ohci-dbg.c
@@ -561,7 +561,6 @@ static ssize_t fill_periodic_buffer(struct debug_buffer *buf)
 
 			} else {
 				/* we've seen it and what's after */
-				temp = 0;
 				ed = NULL;
 			}
 
diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
index e82ff2a49672..8a20d9d3c377 100644
--- a/drivers/usb/host/oxu210hp-hcd.c
+++ b/drivers/usb/host/oxu210hp-hcd.c
@@ -1232,7 +1232,7 @@ static int qtd_fill(struct ehci_qtd *qtd, dma_addr_t buf, size_t len,
 		}
 
 		/* short packets may only terminate transfers */
-		if (count != len)
+		if (count != len && maxpacket > 0)
 			count -= (count % maxpacket);
 	}
 	qtd->hw_token = cpu_to_le32((count << 16) | token);
-- 
2.31.1

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Greg KH]

On Sat, Dec 18, 2021 at 01:24:20AM -0300, Julio Faracco wrote:
> Clang is reporting some issues related variable values not used and
> other issues inside some USB host drivers. This commit removes some
> trashes and adds some strategies to mitigate those warnings.
> 
> The most important is the maxpacket not checking for zeros inside both
> functions qtd_fill(). Even if this variable is always higher than zero,
> it should be checked to avoid this kind of verbosity.
> 
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>

Please break this up into "one patch per warning" and show the warning
that is happening as well, as it is not obvious why you are changing
these files in the way you are doing it.

thanks,

greg k-h

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Joe Perches]

On Sat, 2021-12-18 at 01:24 -0300, Julio Faracco wrote:
> Clang is reporting some issues related variable values not used and
> other issues inside some USB host drivers. This commit removes some
> trashes and adds some strategies to mitigate those warnings.
[]
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
[]
> @@ -903,7 +903,6 @@ static ssize_t fill_registers_buffer(struct debug_buffer *buf)
>  	temp = scnprintf(next, size, "complete %ld unlink %ld\n",
>  		ehci->stats.complete, ehci->stats.unlink);
>  	size -= temp;
> -	next += temp;
>  #endif

I think this should line not be removed as it's a code pattern
repeated multiple times above and another entry could be added
below.

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Alan Stern]

On Sat, Dec 18, 2021 at 01:24:20AM -0300, Julio Faracco wrote:
> Clang is reporting some issues related variable values not used and
> other issues inside some USB host drivers. This commit removes some
> trashes and adds some strategies to mitigate those warnings.
> 
> The most important is the maxpacket not checking for zeros inside both
> functions qtd_fill(). Even if this variable is always higher than zero,
> it should be checked to avoid this kind of verbosity.
> 
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  drivers/usb/host/ehci-dbg.c     | 1 -
>  drivers/usb/host/ehci-q.c       | 2 +-
>  drivers/usb/host/ohci-dbg.c     | 1 -
>  drivers/usb/host/oxu210hp-hcd.c | 2 +-
>  4 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index 0b7f1edd9eec..70b4ff65295a 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -903,7 +903,6 @@ static ssize_t fill_registers_buffer(struct debug_buffer *buf)
>  	temp = scnprintf(next, size, "complete %ld unlink %ld\n",
>  		ehci->stats.complete, ehci->stats.unlink);
>  	size -= temp;
> -	next += temp;

Like Joe said, this is a standard pattern (an idiom) and I don't think 
it should be changed.  Besides, the compiler ought to remove the 
unnecessary store automatically.

> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 2cbf4f85bff3..98cb44414e78 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -64,7 +64,7 @@ qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
>  		}
>  
>  		/* short packets may only terminate transfers */
> -		if (count != len)
> +		if (count != len && maxpacket > 0)
>  			count -= (count % maxpacket);

This is different.  But again, I do not think the extra check should be 
added.  If maxpacket is 0, we _want_ the code to fail in a highly 
visible manner -- it would mean there is a bug somewhere else in the 
kernel.

> diff --git a/drivers/usb/host/ohci-dbg.c b/drivers/usb/host/ohci-dbg.c
> index 4f267dc93882..6fc9c46ffe3c 100644
> --- a/drivers/usb/host/ohci-dbg.c
> +++ b/drivers/usb/host/ohci-dbg.c
> @@ -561,7 +561,6 @@ static ssize_t fill_periodic_buffer(struct debug_buffer *buf)
>  
>  			} else {
>  				/* we've seen it and what's after */
> -				temp = 0;
>  				ed = NULL;
>  			}

This one is a good subject for removal.  If you separate this out into 
its own patch, I will Ack it.

> diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
> index e82ff2a49672..8a20d9d3c377 100644
> --- a/drivers/usb/host/oxu210hp-hcd.c
> +++ b/drivers/usb/host/oxu210hp-hcd.c
> @@ -1232,7 +1232,7 @@ static int qtd_fill(struct ehci_qtd *qtd, dma_addr_t buf, size_t len,
>  		}
>  
>  		/* short packets may only terminate transfers */
> -		if (count != len)
> +		if (count != len && maxpacket > 0)
>  			count -= (count % maxpacket);

This is essentially the same as the change to ehci-q.c.

In short, if the compiler produces warnings that are inappropriate, it's 
an indication that the warnings need to be disabled -- not that the 
code needs to be changed.

Alan Stern

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Tetsuo Handa]

On 2021/12/19 3:05, Alan Stern wrote:
>> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
>> index 2cbf4f85bff3..98cb44414e78 100644
>> --- a/drivers/usb/host/ehci-q.c
>> +++ b/drivers/usb/host/ehci-q.c
>> @@ -64,7 +64,7 @@ qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
>>  		}
>>  
>>  		/* short packets may only terminate transfers */
>> -		if (count != len)
>> +		if (count != len && maxpacket > 0)
>>  			count -= (count % maxpacket);
> 
> This is different.  But again, I do not think the extra check should be 
> added.  If maxpacket is 0, we _want_ the code to fail in a highly 
> visible manner -- it would mean there is a bug somewhere else in the 
> kernel.

Some of the callers are passing the return value from usb_maxpacket(), and
usb_maxpacket() can return 0. But division by 0 bug here becomes visible
only when len < count in

	count = 0x1000 - (buf & 0x0fff);	/* rest of that page */
	if (likely (len < count))		/* ... iff needed */
		count = len;

is false and count != len in

		if (count != len)
			count -= (count % maxpacket);

is true, which may be quite difficult to trigger.

Maybe we should make sure that maxpacket > 0 on the caller side, for e.g.

	/* qh makes control packets use qtd toggle; maybe switch it */
	if ((maxpacket & (this_qtd_len + (maxpacket - 1))) == 0)
		token ^= QTD_TOGGLE;

and

	if (usb_pipecontrol (urb->pipe)) {
		one_more = 1;
		token ^= 0x0100;	/* "in" <--> "out"  */
		token |= QTD_TOGGLE;	/* force DATA1 */
	} else if (usb_pipeout(urb->pipe)
			&& (urb->transfer_flags & URB_ZERO_PACKET)
			&& !(urb->transfer_buffer_length % maxpacket)) {
		one_more = 1;
	}

are expecting that maxpacket > 0 ?

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Alan Stern]

On Sun, Dec 19, 2021 at 10:41:02AM +0900, Tetsuo Handa wrote:
> On 2021/12/19 3:05, Alan Stern wrote:
> >> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> >> index 2cbf4f85bff3..98cb44414e78 100644
> >> --- a/drivers/usb/host/ehci-q.c
> >> +++ b/drivers/usb/host/ehci-q.c
> >> @@ -64,7 +64,7 @@ qtd_fill(struct ehci_hcd *ehci, struct ehci_qtd *qtd, dma_addr_t buf,
> >>  		}
> >>  
> >>  		/* short packets may only terminate transfers */
> >> -		if (count != len)
> >> +		if (count != len && maxpacket > 0)
> >>  			count -= (count % maxpacket);
> > 
> > This is different.  But again, I do not think the extra check should be 
> > added.  If maxpacket is 0, we _want_ the code to fail in a highly 
> > visible manner -- it would mean there is a bug somewhere else in the 
> > kernel.
> 
> Some of the callers are passing the return value from usb_maxpacket(), and
> usb_maxpacket() can return 0. But division by 0 bug here becomes visible
> only when len < count in
> 
> 	count = 0x1000 - (buf & 0x0fff);	/* rest of that page */
> 	if (likely (len < count))		/* ... iff needed */
> 		count = len;
> 
> is false and count != len in
> 
> 		if (count != len)
> 			count -= (count % maxpacket);
> 
> is true, which may be quite difficult to trigger.
> 
> Maybe we should make sure that maxpacket > 0 on the caller side, for e.g.
> 
> 	/* qh makes control packets use qtd toggle; maybe switch it */
> 	if ((maxpacket & (this_qtd_len + (maxpacket - 1))) == 0)
> 		token ^= QTD_TOGGLE;
> 
> and
> 
> 	if (usb_pipecontrol (urb->pipe)) {
> 		one_more = 1;
> 		token ^= 0x0100;	/* "in" <--> "out"  */
> 		token |= QTD_TOGGLE;	/* force DATA1 */
> 	} else if (usb_pipeout(urb->pipe)
> 			&& (urb->transfer_flags & URB_ZERO_PACKET)
> 			&& !(urb->transfer_buffer_length % maxpacket)) {
> 		one_more = 1;
> 	}
> 
> are expecting that maxpacket > 0 ?

You should read this code in usb_submit_urb():

	max = usb_endpoint_maxp(&ep->desc);
	if (max <= 0) {
		dev_dbg(&dev->dev,
			"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
			usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
			__func__, max);
		return -EMSGSIZE;
	}

As far as I know, every code path leading to qtd_fill() has to pass this 
test.

Alan Stern

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Tetsuo Handa]

On 2021/12/19 11:50, Alan Stern wrote:
> You should read this code in usb_submit_urb():
> 
> 	max = usb_endpoint_maxp(&ep->desc);
> 	if (max <= 0) {
> 		dev_dbg(&dev->dev,
> 			"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
> 			usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
> 			__func__, max);
> 		return -EMSGSIZE;
> 	}
> 
> As far as I know, every code path leading to qtd_fill() has to pass this 
> test.

Excuse me, but surely qtd_fill() is using the result from usb_maxpacket()

----------------------------------------
static struct list_head *
qh_urb_transaction (
	struct ehci_hcd		*ehci,
	struct urb		*urb,
	struct list_head	*head,
	gfp_t			flags
) {
(...snipped...)
	maxpacket = usb_maxpacket(urb->dev, urb->pipe, !is_input);

	/*
	 * buffer gets wrapped in one or more qtds;
	 * last one may be "short" (including zero len)
	 * and may serve as a control status ack
	 */
	for (;;) {
		int this_qtd_len;

		this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
				maxpacket);
		this_sg_len -= this_qtd_len;
		len -= this_qtd_len;
		buf += this_qtd_len;
(...snipped...)
}
----------------------------------------

and usb_maxpacket() may return 0 ?

----------------------------------------
static inline __u16
usb_maxpacket(struct usb_device *udev, int pipe, int is_out)
{
	struct usb_host_endpoint	*ep;
	unsigned			epnum = usb_pipeendpoint(pipe);

	if (is_out) {
		WARN_ON(usb_pipein(pipe));
		ep = udev->ep_out[epnum];
	} else {
		WARN_ON(usb_pipeout(pipe));
		ep = udev->ep_in[epnum];
	}
	if (!ep)
		return 0;

	/* NOTE:  only 0x07ff bits are for packet size... */
	return usb_endpoint_maxp(&ep->desc);
}
----------------------------------------

If we don't need to care about the possibility of returning 0 (including
all possible race conditions taken into account), please explain it as a
comment block.

Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

[Alan Stern]

On Sun, Dec 19, 2021 at 04:59:44PM +0900, Tetsuo Handa wrote:
> On 2021/12/19 11:50, Alan Stern wrote:
> > You should read this code in usb_submit_urb():
> > 
> > 	max = usb_endpoint_maxp(&ep->desc);
> > 	if (max <= 0) {
> > 		dev_dbg(&dev->dev,
> > 			"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
> > 			usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
> > 			__func__, max);
> > 		return -EMSGSIZE;
> > 	}
> > 
> > As far as I know, every code path leading to qtd_fill() has to pass this 
> > test.
> 
> Excuse me, but surely qtd_fill() is using the result from usb_maxpacket()
> 
> ----------------------------------------
> static struct list_head *
> qh_urb_transaction (
> 	struct ehci_hcd		*ehci,
> 	struct urb		*urb,
> 	struct list_head	*head,
> 	gfp_t			flags
> ) {
> (...snipped...)
> 	maxpacket = usb_maxpacket(urb->dev, urb->pipe, !is_input);
> 
> 	/*
> 	 * buffer gets wrapped in one or more qtds;
> 	 * last one may be "short" (including zero len)
> 	 * and may serve as a control status ack
> 	 */
> 	for (;;) {
> 		int this_qtd_len;
> 
> 		this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
> 				maxpacket);
> 		this_sg_len -= this_qtd_len;
> 		len -= this_qtd_len;
> 		buf += this_qtd_len;
> (...snipped...)
> }
> ----------------------------------------
> 
> and usb_maxpacket() may return 0 ?
> 
> ----------------------------------------
> static inline __u16
> usb_maxpacket(struct usb_device *udev, int pipe, int is_out)
> {
> 	struct usb_host_endpoint	*ep;
> 	unsigned			epnum = usb_pipeendpoint(pipe);
> 
> 	if (is_out) {
> 		WARN_ON(usb_pipein(pipe));
> 		ep = udev->ep_out[epnum];
> 	} else {
> 		WARN_ON(usb_pipeout(pipe));
> 		ep = udev->ep_in[epnum];
> 	}
> 	if (!ep)
> 		return 0;
> 
> 	/* NOTE:  only 0x07ff bits are for packet size... */
> 	return usb_endpoint_maxp(&ep->desc);
> }
> ----------------------------------------

You should also read this code in usb_submit_urb():

	ep = usb_pipe_endpoint(dev, urb->pipe);
	if (!ep)
		return -ENOENT;

together with the definition of usb_pipe_endpoint():

static inline struct usb_host_endpoint *
usb_pipe_endpoint(struct usb_device *dev, unsigned int pipe)
{
	struct usb_host_endpoint **eps;
	eps = usb_pipein(pipe) ? dev->ep_in : dev->ep_out;
	return eps[usb_pipeendpoint(pipe)];
}

As you can see, this carries out the same calculation that 
usb_maxpacket() makes, but it fails with an error if ep would be NULL.

> If we don't need to care about the possibility of returning 0 (including
> all possible race conditions taken into account), please explain it as a
> comment block.

You may write such a comment and submit it as a patch, if you like.  But 
keep in mind that the USB subsystem is full of potential race conditions 
like this one, kept in check by appropriate locking and synchronization.  
Writing a comment for each and every possible occurrence would be 
daunting and counterproductive.

Also, if you like, you may submit a patch that changes 
qh_urb_transaction() so that it calls usb_endpoint_maxp() rather than 
usb_maxpacket() (using &urb->ep->desc as the argument rather than 
urb->pipe), so that it more closely imitates the calculation in 
usb_submit_urb().  You can even add a WARN_ON(maxpacket == 0), but I 
do not expect it will ever be triggered.

Alan Stern