> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf Of Julius
> Werner
> Sent: Friday, March 20, 2015 7:23 PM
> To: Kaukab, Yousaf
> Cc: Julius Werner; [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
>
> >> > - 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.
>
> Are the two mutually exclusive? I meant that I think this should be
> 'kzalloc(buf_size, GFP_DMA | GFP_ATOMIC)', because I wouldn't be sure that
> GFP_ATOMIC always returns DMA-able memory on systems that may have
> limitations there.
>
> >> > }
> >> > }
> >> >
> >> > + 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
>
> Yeah, I think this should work as well.
>
> However, looking at this again I think there are more problems on unmapping.
> Since some of those calls are guarded by chan->ep_is_in, you will not unmap
> the memory out OUT transfers. DMA map/unmap calls must always be
> balanced.
>
> I think in all the cases where the code previously had something like
>
> if (chan->align_buf && chan->ep_is_in) {
> memcpy(...);
> ...;
> }
>
> you'll need to change it into
>
> if (chan->align_buf) {
> if (chan->ep_is_in) {
> memcpy(...);
> dma_unmap_single(..., DMA_FROM_DEVICE);
> ...
> } else {
> dma_unmap_single(..., DMA_TO_DEVICE);
> }
> }
>
> You might also want to double-check all abnormal error paths to ensure you're
> not leaking a DMA mapping. The previous code might not have been so careful
> (since for a really bad error you usually don't care about memcpy()ing the
> receive buffer back), but if you use DMA mappings you have to.
Error paths looks ok to me. Let me know if you have any specific case in mind.
I will update the patch for the rest of the comments.
BR,
Yousaf
N�����r��y����b�X��ǧv�^�){.n�+����{������^n�r���z���h�����&�� �G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�
