> -----Original Message----- > From: jwerner@xxxxxxxxxx [mailto:jwerner@xxxxxxxxxx] On Behalf Of Julius > Werner > Sent: Wednesday, March 18, 2015 11:43 PM > To: Kaukab, Yousaf > Cc: 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 > > [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 > <yousaf.kaukab@xxxxxxxxx> wrote: > > From: Gregory Herrero <gregory.herrero@xxxxxxxxx> > > > > 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 <gregory.herrero@xxxxxxxxx> > > --- > > 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��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
