Re: [PATCH] usb: dwc2: host: Fix wMaxPacketSize handling (fix webcam regression)

[Minas Harutyunyan <Minas.Harutyunyan@xxxxxxxxxxxx>]

On 6/1/2019 12:05 AM, Douglas Anderson wrote:
> In commit abb621844f6a ("usb: ch9: make usb_endpoint_maxp() return
> only packet size") the API to usb_endpoint_maxp() changed.  It used to
> just return wMaxPacketSize but after that commit it returned
> wMaxPacketSize with the high bits (the multiplier) masked off.  If you
> wanted to get the multiplier it was now up to your code to call the
> new usb_endpoint_maxp_mult() which was introduced in
> commit 541b6fe63023 ("usb: add helper to extract bits 12:11 of
> wMaxPacketSize").
>
> Prior to the API change most host drivers were updated, but no update
> was made to dwc2.  Presumably it was assumed that dwc2 was too
> simplistic to use the multiplier and thus just didn't support a
> certain class of USB devices.  However, it turns out that dwc2 did use
> the multiplier and many devices using it were working quite nicely.
> That means that many USB devices have been broken since the API
> change.  One such device is a Logitech HD Pro Webcam C920.
>
> Specifically, though dwc2 didn't directly call usb_endpoint_maxp(), it
> did call usb_maxpacket() which in turn called usb_endpoint_maxp().
>
> Let's update dwc2 to work properly with the new API.
>
> Fixes: abb621844f6a ("usb: ch9: make usb_endpoint_maxp() return only packet size")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---

Acked-by: Minas Harutyunyan <hminas@xxxxxxxxxxxx>

>   drivers/usb/dwc2/hcd.c       | 29 +++++++++++++++++------------
>   drivers/usb/dwc2/hcd.h       | 20 +++++++++++---------
>   drivers/usb/dwc2/hcd_intr.c  |  5 +++--
>   drivers/usb/dwc2/hcd_queue.c | 10 ++++++----
>   4 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index b50ec3714fd8..5c51bf5506d1 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -2608,7 +2608,7 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>   	chan->dev_addr = dwc2_hcd_get_dev_addr(&urb->pipe_info);
>   	chan->ep_num = dwc2_hcd_get_ep_num(&urb->pipe_info);
>   	chan->speed = qh->dev_speed;
> -	chan->max_packet = dwc2_max_packet(qh->maxp);
> +	chan->max_packet = qh->maxp;
>   
>   	chan->xfer_started = 0;
>   	chan->halt_status = DWC2_HC_XFER_NO_HALT_STATUS;
> @@ -2686,7 +2686,7 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>   		 * This value may be modified when the transfer is started
>   		 * to reflect the actual transfer length
>   		 */
> -		chan->multi_count = dwc2_hb_mult(qh->maxp);
> +		chan->multi_count = qh->maxp_mult;
>   
>   	if (hsotg->params.dma_desc_enable) {
>   		chan->desc_list_addr = qh->desc_list_dma;
> @@ -3806,19 +3806,21 @@ static struct dwc2_hcd_urb *dwc2_hcd_urb_alloc(struct dwc2_hsotg *hsotg,
>   
>   static void dwc2_hcd_urb_set_pipeinfo(struct dwc2_hsotg *hsotg,
>   				      struct dwc2_hcd_urb *urb, u8 dev_addr,
> -				      u8 ep_num, u8 ep_type, u8 ep_dir, u16 mps)
> +				      u8 ep_num, u8 ep_type, u8 ep_dir,
> +				      u16 maxp, u16 maxp_mult)
>   {
>   	if (dbg_perio() ||
>   	    ep_type == USB_ENDPOINT_XFER_BULK ||
>   	    ep_type == USB_ENDPOINT_XFER_CONTROL)
>   		dev_vdbg(hsotg->dev,
> -			 "addr=%d, ep_num=%d, ep_dir=%1x, ep_type=%1x, mps=%d\n",
> -			 dev_addr, ep_num, ep_dir, ep_type, mps);
> +			 "addr=%d, ep_num=%d, ep_dir=%1x, ep_type=%1x, maxp=%d (%d mult)\n",
> +			 dev_addr, ep_num, ep_dir, ep_type, maxp, maxp_mult);
>   	urb->pipe_info.dev_addr = dev_addr;
>   	urb->pipe_info.ep_num = ep_num;
>   	urb->pipe_info.pipe_type = ep_type;
>   	urb->pipe_info.pipe_dir = ep_dir;
> -	urb->pipe_info.mps = mps;
> +	urb->pipe_info.maxp = maxp;
> +	urb->pipe_info.maxp_mult = maxp_mult;
>   }
>   
>   /*
> @@ -3909,8 +3911,9 @@ void dwc2_hcd_dump_state(struct dwc2_hsotg *hsotg)
>   					dwc2_hcd_is_pipe_in(&urb->pipe_info) ?
>   					"IN" : "OUT");
>   				dev_dbg(hsotg->dev,
> -					"      Max packet size: %d\n",
> -					dwc2_hcd_get_mps(&urb->pipe_info));
> +					"      Max packet size: %d (%d mult)\n",
> +					dwc2_hcd_get_maxp(&urb->pipe_info),
> +					dwc2_hcd_get_maxp_mult(&urb->pipe_info));
>   				dev_dbg(hsotg->dev,
>   					"      transfer_buffer: %p\n",
>   					urb->buf);
> @@ -4510,8 +4513,10 @@ static void dwc2_dump_urb_info(struct usb_hcd *hcd, struct urb *urb,
>   	}
>   
>   	dev_vdbg(hsotg->dev, "  Speed: %s\n", speed);
> -	dev_vdbg(hsotg->dev, "  Max packet size: %d\n",
> -		 usb_maxpacket(urb->dev, urb->pipe, usb_pipeout(urb->pipe)));
> +	dev_vdbg(hsotg->dev, "  Max packet size: %d (%d mult)\n",
> +		 usb_endpoint_maxp(&urb->ep->desc),
> +		 usb_endpoint_maxp_mult(&urb->ep->desc));
> +
>   	dev_vdbg(hsotg->dev, "  Data buffer length: %d\n",
>   		 urb->transfer_buffer_length);
>   	dev_vdbg(hsotg->dev, "  Transfer buffer: %p, Transfer DMA: %08lx\n",
> @@ -4594,8 +4599,8 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
>   	dwc2_hcd_urb_set_pipeinfo(hsotg, dwc2_urb, usb_pipedevice(urb->pipe),
>   				  usb_pipeendpoint(urb->pipe), ep_type,
>   				  usb_pipein(urb->pipe),
> -				  usb_maxpacket(urb->dev, urb->pipe,
> -						!(usb_pipein(urb->pipe))));
> +				  usb_endpoint_maxp(&ep->desc),
> +				  usb_endpoint_maxp_mult(&ep->desc));
>   
>   	buf = urb->transfer_buffer;
>   
> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
> index c089ffa1f0a8..ce6445a06588 100644
> --- a/drivers/usb/dwc2/hcd.h
> +++ b/drivers/usb/dwc2/hcd.h
> @@ -171,7 +171,8 @@ struct dwc2_hcd_pipe_info {
>   	u8 ep_num;
>   	u8 pipe_type;
>   	u8 pipe_dir;
> -	u16 mps;
> +	u16 maxp;
> +	u16 maxp_mult;
>   };
>   
>   struct dwc2_hcd_iso_packet_desc {
> @@ -264,6 +265,7 @@ struct dwc2_hs_transfer_time {
>    *                       - USB_ENDPOINT_XFER_ISOC
>    * @ep_is_in:           Endpoint direction
>    * @maxp:               Value from wMaxPacketSize field of Endpoint Descriptor
> + * @maxp_mult:          Multiplier for maxp
>    * @dev_speed:          Device speed. One of the following values:
>    *                       - USB_SPEED_LOW
>    *                       - USB_SPEED_FULL
> @@ -340,6 +342,7 @@ struct dwc2_qh {
>   	u8 ep_type;
>   	u8 ep_is_in;
>   	u16 maxp;
> +	u16 maxp_mult;
>   	u8 dev_speed;
>   	u8 data_toggle;
>   	u8 ping_state;
> @@ -503,9 +506,14 @@ static inline u8 dwc2_hcd_get_pipe_type(struct dwc2_hcd_pipe_info *pipe)
>   	return pipe->pipe_type;
>   }
>   
> -static inline u16 dwc2_hcd_get_mps(struct dwc2_hcd_pipe_info *pipe)
> +static inline u16 dwc2_hcd_get_maxp(struct dwc2_hcd_pipe_info *pipe)
> +{
> +	return pipe->maxp;
> +}
> +
> +static inline u16 dwc2_hcd_get_maxp_mult(struct dwc2_hcd_pipe_info *pipe)
>   {
> -	return pipe->mps;
> +	return pipe->maxp_mult;
>   }
>   
>   static inline u8 dwc2_hcd_get_dev_addr(struct dwc2_hcd_pipe_info *pipe)
> @@ -620,12 +628,6 @@ static inline bool dbg_urb(struct urb *urb)
>   static inline bool dbg_perio(void) { return false; }
>   #endif
>   
> -/* High bandwidth multiplier as encoded in highspeed endpoint descriptors */
> -#define dwc2_hb_mult(wmaxpacketsize) (1 + (((wmaxpacketsize) >> 11) & 0x03))
> -
> -/* Packet size for any kind of endpoint descriptor */
> -#define dwc2_max_packet(wmaxpacketsize) ((wmaxpacketsize) & 0x07ff)
> -
>   /*
>    * Returns true if frame1 index is greater than frame2 index. The comparison
>    * is done modulo FRLISTEN_64_SIZE. This accounts for the rollover of the
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 88b5dcf3aefc..a052d39b4375 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -1617,8 +1617,9 @@ static void dwc2_hc_ahberr_intr(struct dwc2_hsotg *hsotg,
>   
>   	dev_err(hsotg->dev, "  Speed: %s\n", speed);
>   
> -	dev_err(hsotg->dev, "  Max packet size: %d\n",
> -		dwc2_hcd_get_mps(&urb->pipe_info));
> +	dev_err(hsotg->dev, "  Max packet size: %d (mult %d)\n",
> +		dwc2_hcd_get_maxp(&urb->pipe_info),
> +		dwc2_hcd_get_maxp_mult(&urb->pipe_info));
>   	dev_err(hsotg->dev, "  Data buffer length: %d\n", urb->length);
>   	dev_err(hsotg->dev, "  Transfer buffer: %p, Transfer DMA: %08lx\n",
>   		urb->buf, (unsigned long)urb->dma);
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index ea3aa640c15c..68bbac64b753 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -708,7 +708,7 @@ static void dwc2_hs_pmap_unschedule(struct dwc2_hsotg *hsotg,
>   static int dwc2_uframe_schedule_split(struct dwc2_hsotg *hsotg,
>   				      struct dwc2_qh *qh)
>   {
> -	int bytecount = dwc2_hb_mult(qh->maxp) * dwc2_max_packet(qh->maxp);
> +	int bytecount = qh->maxp_mult * qh->maxp;
>   	int ls_search_slice;
>   	int err = 0;
>   	int host_interval_in_sched;
> @@ -1332,7 +1332,7 @@ static int dwc2_check_max_xfer_size(struct dwc2_hsotg *hsotg,
>   	u32 max_channel_xfer_size;
>   	int status = 0;
>   
> -	max_xfer_size = dwc2_max_packet(qh->maxp) * dwc2_hb_mult(qh->maxp);
> +	max_xfer_size = qh->maxp * qh->maxp_mult;
>   	max_channel_xfer_size = hsotg->params.max_transfer_size;
>   
>   	if (max_xfer_size > max_channel_xfer_size) {
> @@ -1517,8 +1517,9 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
>   	u32 prtspd = (hprt & HPRT0_SPD_MASK) >> HPRT0_SPD_SHIFT;
>   	bool do_split = (prtspd == HPRT0_SPD_HIGH_SPEED &&
>   			 dev_speed != USB_SPEED_HIGH);
> -	int maxp = dwc2_hcd_get_mps(&urb->pipe_info);
> -	int bytecount = dwc2_hb_mult(maxp) * dwc2_max_packet(maxp);
> +	int maxp = dwc2_hcd_get_maxp(&urb->pipe_info);
> +	int maxp_mult = dwc2_hcd_get_maxp_mult(&urb->pipe_info);
> +	int bytecount = maxp_mult * maxp;
>   	char *speed, *type;
>   
>   	/* Initialize QH */
> @@ -1531,6 +1532,7 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
>   
>   	qh->data_toggle = DWC2_HC_PID_DATA0;
>   	qh->maxp = maxp;
> +	qh->maxp_mult = maxp_mult;
>   	INIT_LIST_HEAD(&qh->qtd_list);
>   	INIT_LIST_HEAD(&qh->qh_list_entry);
>