RE: [PATCH 1/3] usb: dwc2: host: allocate qh before atomic enqueue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: John Youn [mailto:John.Youn@xxxxxxxxxxxx]
> Sent: Wednesday, June 10, 2015 7:33 PM
> To: Kaukab, Yousaf; John Youn; linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx
> Cc: Herrero, Gregory; heiko@xxxxxxxxx; Holmberg, Hans
> Subject: Re: [PATCH 1/3] usb: dwc2: host: allocate qh before atomic enqueue
> 
> On 6/10/2015 6:27 AM, Kaukab, Yousaf wrote:
> >> -----Original Message-----
> >> From: John Youn [mailto:John.Youn@xxxxxxxxxxxx]
> >> Sent: Wednesday, June 10, 2015 1:06 AM
> >> To: Kaukab, Yousaf; linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx;
> >> john.youn@xxxxxxxxxxxx
> >> Cc: Herrero, Gregory; heiko@xxxxxxxxx; Holmberg, Hans
> >> Subject: Re: [PATCH 1/3] usb: dwc2: host: allocate qh before atomic
> >> enqueue
> >>
> >> On 6/5/2015 7:23 AM, Mian Yousaf Kaukab wrote:
> >>> To avoid sleep while atomic bugs, allocate qh before calling
> >>> dwc2_hcd_urb_enqueue. qh pointer can be used directly now instead of
> >>> passing ep->hcpriv as double pointer.
> >>>
> >>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@xxxxxxxxx>
> >>> ---
> >>>  drivers/usb/dwc2/hcd.c       | 31 ++++++++++++++++++++++++++----
> >>>  drivers/usb/dwc2/hcd.h       |  5 ++++-
> >>>  drivers/usb/dwc2/hcd_queue.c | 45
> >>> ++++++++++++--------------------------------
> >>>  3 files changed, 43 insertions(+), 38 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index
> >>> b10377c..80bce71 100644
> >>> --- a/drivers/usb/dwc2/hcd.c
> >>> +++ b/drivers/usb/dwc2/hcd.c
> >>> @@ -359,7 +359,7 @@ void dwc2_hcd_stop(struct dwc2_hsotg *hsotg)
> >>>
> >>>  /* Caller must hold driver lock */
> >>>  static int dwc2_hcd_urb_enqueue(struct dwc2_hsotg *hsotg,
> >>> -				struct
> >> dwc2_hcd_urb *urb, void **ep_handle,
> >>> +				struct
> >> dwc2_hcd_urb *urb, struct dwc2_qh *qh,
> >>>  				gfp_t mem_flags)
> >>>  {
> >>>  	struct dwc2_qtd *qtd;
> >>> @@ -391,8 +391,7 @@ static int dwc2_hcd_urb_enqueue(struct
> >>> dwc2_hsotg
> >> *hsotg,
> >>>  		return -ENOMEM;
> >>>
> >>>  	dwc2_hcd_qtd_init(qtd, urb);
> >>> -	retval = dwc2_hcd_qtd_add(hsotg, qtd, (struct dwc2_qh
> >> **)ep_handle,
> >>> -				  mem_flags);
> >>> +	retval = dwc2_hcd_qtd_add(hsotg, qtd, qh);
> >>>  	if (retval) {
> >>>  		dev_err(hsotg->dev,
> >>>  			"DWC OTG HCD URB Enqueue
> >> failed adding QTD. Error status %d\n", @@
> >>> -2445,6 +2444,8 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd
> >>> *hcd,
> >> struct urb *urb,
> >>>  	u32 tflags = 0;
> >>>  	void *buf;
> >>>  	unsigned long flags;
> >>> +	struct dwc2_qh *qh;
> >>> +	bool qh_allocated = false;
> >>>
> >>>  	if (dbg_urb(urb)) {
> >>>  		dev_vdbg(hsotg->dev, "DWC OTG HCD URB
> >> Enqueue\n"); @@ -2523,13
> >>> +2524,24 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd,
> >>> +struct
> >> urb *urb,
> >>>
> >> 	 urb->iso_frame_desc[i].length);
> >>>
> >>>  	urb->hcpriv = dwc2_urb;
> >>> +	qh = (struct dwc2_qh *) ep->hcpriv;
> >>> +	/* Create QH for the endpoint if it doesn't exist */
> >>> +	if (!qh) {
> >>> +		qh = dwc2_hcd_qh_create(hsotg, dwc2_urb,
> >> mem_flags);
> >>> +		if (!qh) {
> >>> +			retval = -ENOMEM;
> >>> +			goto fail0;
> >>> +		}
> >>> +		ep->hcpriv = qh;
> >>> +		qh_allocated = true;
> >>> +	}
> >>>
> >>>  	spin_lock_irqsave(&hsotg->lock, flags);
> >>>  	retval = usb_hcd_link_urb_to_ep(hcd, urb);
> >>>  	if (retval)
> >>>  		goto fail1;
> >>>
> >>> -	retval = dwc2_hcd_urb_enqueue(hsotg, dwc2_urb, &ep->hcpriv,
> >> mem_flags);
> >>> +	retval = dwc2_hcd_urb_enqueue(hsotg, dwc2_urb, qh,
> >> mem_flags);
> >>>  	if (retval)
> >>>  		goto fail2;
> >>>
> >>> @@ -2549,6 +2561,17 @@ fail2:
> >>>  fail1:
> >>>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> >>>  	urb->hcpriv = NULL;
> >>> +	if (qh_allocated) {
> >>> +		struct dwc2_qtd *qtd2, *qtd2_tmp;
> >>> +
> >>> +		ep->hcpriv = NULL;
> >>> +		dwc2_hcd_qh_unlink(hsotg, qh);
> >>> +		/* Free each QTD in the QH's QTD list */
> >>> +		list_for_each_entry_safe(qtd2, qtd2_tmp, &qh-
> >>> qtd_list,
> >>> +
> >> 		 qtd_list_entry)
> >>> +
> >> 	dwc2_hcd_qtd_unlink_and_free(hsotg, qtd2, qh);
> >>> +		dwc2_hcd_qh_free(hsotg, qh);
> >>> +	}
> >>>  fail0:
> >>>  	kfree(dwc2_urb);
> >>>
> >>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h index
> >>> 7b5841c..fc10549 100644
> >>> --- a/drivers/usb/dwc2/hcd.h
> >>> +++ b/drivers/usb/dwc2/hcd.h
> >>> @@ -463,6 +463,9 @@ extern void dwc2_hcd_queue_transactions(struct
> >>> dwc2_hsotg *hsotg,
> >>>  /* Schedule Queue Functions */
> >>>  /* Implemented in hcd_queue.c */
> >>>  extern void dwc2_hcd_init_usecs(struct dwc2_hsotg *hsotg);
> >>> +extern struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
> >>> +
> >> struct dwc2_hcd_urb *urb,
> >>> +
> >> gfp_t mem_flags);
> >>>  extern void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct
> >>> dwc2_qh *qh);  extern int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg,
> >>> struct dwc2_qh *qh);  extern void dwc2_hcd_qh_unlink(struct
> >>> dwc2_hsotg *hsotg, struct dwc2_qh *qh); @@ -471,7 +474,7 @@ extern
> >>> void dwc2_hcd_qh_deactivate(struct dwc2_hsotg *hsotg, struct dwc2_qh
> >>> *qh,
> >>>
> >>>  extern void dwc2_hcd_qtd_init(struct dwc2_qtd *qtd, struct
> >>> dwc2_hcd_urb *urb);  extern int dwc2_hcd_qtd_add(struct dwc2_hsotg
> >> *hsotg, struct dwc2_qtd *qtd,
> >>> -			    struct dwc2_qh **qh, gfp_t
> >> mem_flags);
> >>> +			    struct dwc2_qh *qh);
> >>>
> >>>  /* Unlinks and frees a QTD */
> >>>  static inline void dwc2_hcd_qtd_unlink_and_free(struct dwc2_hsotg
> >>> *hsotg, diff --git a/drivers/usb/dwc2/hcd_queue.c
> >>> b/drivers/usb/dwc2/hcd_queue.c index 9b5c362..e573a4f 100644
> >>> --- a/drivers/usb/dwc2/hcd_queue.c
> >>> +++ b/drivers/usb/dwc2/hcd_queue.c
> >>> @@ -191,7 +191,7 @@ static void dwc2_qh_init(struct dwc2_hsotg
> >>> *hsotg,
> >> struct dwc2_qh *qh,
> >>>   *
> >>>   * Return: Pointer to the newly allocated QH, or NULL on error
> >>>   */
> >>> -static struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
> >>> +struct dwc2_qh *dwc2_hcd_qh_create(struct dwc2_hsotg *hsotg,
> >>>
> >> struct dwc2_hcd_urb *urb,
> >>>
> >> gfp_t mem_flags)
> >>>  {
> >>> @@ -767,57 +767,36 @@ void dwc2_hcd_qtd_init(struct dwc2_qtd *qtd,
> >> struct dwc2_hcd_urb *urb)
> >>>   *
> >>>   * @hsotg:        The DWC HCD structure
> >>>   * @qtd:          The QTD to add
> >>> - * @qh:           Out parameter to return queue head
> >>> - * @atomic_alloc: Flag to do atomic alloc if needed
> >>> + * @qh:           Queue head to add qtd to
> >>>   *
> >>>   * Return: 0 if successful, negative error code otherwise
> >>>   *
> >>> - * Finds the correct QH to place the QTD into. If it does not find
> >>> a QH, it
> >>> - * will create a new QH. If the QH to which the QTD is added is not
> >>> currently
> >>> - * scheduled, it is placed into the proper schedule based on its EP type.
> >>> + * If the QH to which the QTD is added is not currently scheduled,
> >>> + it is placed
> >>> + * into the proper schedule based on its EP type.
> >>>   */
> >>>  int dwc2_hcd_qtd_add(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
> >>> -		     struct dwc2_qh **qh, gfp_t mem_flags)
> >>> +		     struct dwc2_qh *qh)
> >>>  {
> >>> -	struct dwc2_hcd_urb *urb = qtd->urb;
> >>> -	int allocated = 0;
> >>>  	int retval;
> >>>
> >>>  	/*
> >>>  	 * Get the QH which holds the QTD-list to insert to. Create QH if
> >> it
> >>>  	 * doesn't exist.
> >>>  	 */
> >>
> >> This comment doesn't apply anymore. Otherwise the series looks ok.
> >
> > Applies cleanly to testing/next at my end. I just noticed that the patch is
> truncated on gmane
> (http://permalink.gmane.org/gmane.linux.usb.general/126493). Looks OK on
> other sites. I will resend the whole series anyway.
> >
> >>
> >> Acked-by: John Youn <johnyoun@xxxxxxxxxxxx>
> >>
> >>
> >> John
> >>
> > /Yousaf
> >
> 
> Sorry, I meant that the comment is no longer valid.
My bad. I will fix it.

> 
> John
> 

/Yousaf
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux