> -----Original Message----- > From: jwerner@xxxxxxxxxx [mailto:jwerner@xxxxxxxxxx] On Behalf Of Julius > Werner > Sent: Friday, March 20, 2015 7:23 PM > To: Kaukab, Yousaf > Cc: Julius Werner; linux-usb@xxxxxxxxxxxxxxx; Felipe Balbi; > john.youn@xxxxxxxxxxxx; Herrero, Gregory; r.baldyga@xxxxxxxxxxx; Dinh > Nguyen; zhangfei.gao@xxxxxxxxxx > 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��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
