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.