> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Julius
> Werner
> Sent: Wednesday, March 18, 2015 11:43 PM
> To: Kaukab, Yousaf
> Cc: [email protected]; Felipe Balbi; [email protected]; Herrero,
> Gregory; [email protected]; Dinh Nguyen; [email protected]
> Subject: Re: [PATCH v1 18/20] usb: dwc2: host: don't use dma_alloc_coherent
> with irqs disabled
>
> [once more, without Gmail being stupid]
>
> Nice! This also solves problems with exhausting coherent DMA memory when
> you plug too many devices in.
>
> On Tue, Mar 17, 2015 at 2:54 AM, Mian Yousaf Kaukab
> <[email protected]> wrote:
> > From: Gregory Herrero <[email protected]>
> >
> > Align buffer must be allocated using kmalloc since irqs are disabled.
> > Coherency is handled through dma_map_single which can be used with
> > irqs disabled.
> >
> > Signed-off-by: Gregory Herrero <[email protected]>
> > ---
> > drivers/usb/dwc2/hcd.c | 7 ++++---
> > drivers/usb/dwc2/hcd_intr.c | 10 ++++++++++
> > drivers/usb/dwc2/hcd_queue.c | 7 ++++---
> > 3 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index
> > fd2ad25..54f58c1 100644
> > --- a/drivers/usb/dwc2/hcd.c
> > +++ b/drivers/usb/dwc2/hcd.c
> > @@ -710,9 +710,7 @@ static int dwc2_hc_setup_align_buf(struct
> dwc2_hsotg *hsotg, struct dwc2_qh *qh,
> > /* 3072 = 3 max-size Isoc packets */
> > buf_size = 3072;
> >
> > - qh->dw_align_buf = dma_alloc_coherent(hsotg->dev, buf_size,
> > - &qh->dw_align_buf_dma,
> > - GFP_ATOMIC);
> > + qh->dw_align_buf = kzalloc(buf_size, GFP_ATOMIC);
>
> Shouldn't this also set GFP_DMA? [...]
No, it should be GFP_ATOMIC, as this can be reached from interrupt handler.
> [...] (And does it need to be kzalloc()? I think
> kmalloc() should be enough and might avoid another memory
> transfer.)
kmalloc() is better.
>
> > if (!qh->dw_align_buf)
> > return -ENOMEM;
> > qh->dw_align_buf_size = buf_size; @@ -737,6 +735,9 @@
> > static int dwc2_hc_setup_align_buf(struct dwc2_hsotg *hsotg, struct dwc2_qh
> *qh,
> > }
> > }
> >
> > + qh->dw_align_buf_dma = dma_map_single(hsotg->dev,
> > + qh->dw_align_buf, qh->dw_align_buf_size,
> > + DMA_TO_DEVICE);
>
> Documentation/DMA-API.txt says that you must always use the same
> arguments for matching dma_map_single() and dma_unmap_single()... so I
> think this and all the unmaps should use DMA_BIDIRECTIONAL.
The mapping is wrong. It should consider endpoint direction. Something like
chan->ep_is_in ? DMA_FROM_DEVICE : DMA_TO_DEVICE
>
> > +
> > chan->align_buf = qh->dw_align_buf_dma;
> > return 0;
> > }
> > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> > index 6927bba..22f1476 100644
> > --- a/drivers/usb/dwc2/hcd_intr.c
> > +++ b/drivers/usb/dwc2/hcd_intr.c
> > @@ -468,6 +468,8 @@ static int dwc2_update_urb_state(struct dwc2_hsotg
> *hsotg,
> > /* Non DWORD-aligned buffer case handling */
> > if (chan->align_buf && xfer_length && chan->ep_is_in) {
> > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> > __func__);
> > + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> > + chan->qh->dw_align_buf_size,
> > + DMA_FROM_DEVICE);
> > memcpy(urb->buf + urb->actual_length,
> > chan->qh->dw_align_buf,
> > xfer_length);
> > }
> > @@ -559,6 +561,8 @@ static enum dwc2_halt_status
> dwc2_update_isoc_urb_state(
> > chan->ep_is_in) {
> > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> > __func__);
> > + dma_unmap_single(hsotg->dev, chan->qh-
> >dw_align_buf_dma,
> > + chan->qh->dw_align_buf_size,
> > + DMA_FROM_DEVICE);
> > memcpy(urb->buf + frame_desc->offset +
> > qtd->isoc_split_offset,
> > chan->qh->dw_align_buf,
> > frame_desc->actual_length); @@ -588,6
> > +592,8 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
> > chan->ep_is_in) {
> > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> > __func__);
> > + dma_unmap_single(hsotg->dev, chan->qh-
> >dw_align_buf_dma,
> > + chan->qh->dw_align_buf_size,
> > + DMA_FROM_DEVICE);
> > memcpy(urb->buf + frame_desc->offset +
> > qtd->isoc_split_offset,
> > chan->qh->dw_align_buf,
> > frame_desc->actual_length); @@ -926,6
> > +932,8 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg
> > *hsotg,
> >
> > if (chan->align_buf) {
> > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> > __func__);
> > + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> > + chan->qh->dw_align_buf_size,
> > + DMA_FROM_DEVICE);
> > memcpy(qtd->urb->buf + frame_desc->offset +
> > qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> > }
> > @@ -1155,6 +1163,8 @@ static void dwc2_update_urb_state_abn(struct
> dwc2_hsotg *hsotg,
> > /* Non DWORD-aligned buffer case handling */
> > if (chan->align_buf && xfer_length && chan->ep_is_in) {
> > dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n",
> > __func__);
> > + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> > + chan->qh->dw_align_buf_size,
> > + DMA_FROM_DEVICE);
> > memcpy(urb->buf + urb->actual_length,
> > chan->qh->dw_align_buf,
> > xfer_length);
> > }
> > diff --git a/drivers/usb/dwc2/hcd_queue.c
> > b/drivers/usb/dwc2/hcd_queue.c index 02103b66..5f206ab 100644
> > --- a/drivers/usb/dwc2/hcd_queue.c
> > +++ b/drivers/usb/dwc2/hcd_queue.c
> > @@ -231,9 +231,10 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg,
> > struct dwc2_qh *qh) {
> > if (hsotg->core_params->dma_desc_enable > 0)
> > dwc2_hcd_qh_free_ddma(hsotg, qh);
> > - else if (qh->dw_align_buf)
> > - dma_free_coherent(hsotg->dev, qh->dw_align_buf_size,
> > - qh->dw_align_buf, qh->dw_align_buf_dma);
> > + else {
>
> Why did you drop the 'if (qh->dw_align_buf)' check? I think you still need it
> since not all endpoints use dwc2_hc_setup_align_buf() (isochronous ones whose
> buffer is already aligned skip it).
>
Yes, the check should be there.
> > + kfree(qh->dw_align_buf);
> > + qh->dw_align_buf_dma = (dma_addr_t)0;
> > + }
> > kfree(qh);
> > }
BR,
Yousaf
N�����r��y����b�X��ǧv�^�){.n�+����{������^n�r���z���h�����&�� �G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�
