On Mon, Apr 01, 2013 at 10:49:26AM +0800, Peter Chen wrote:
> On Fri, Mar 29, 2013 at 12:32:00AM +0100, Michael Grzeschik wrote:

[snip]

> What's the value of sizeof(struct ci13xxx_qh) after you pack it
> as 64 bytes aligned?
> 

With the struct attribute aligned(64) the result
of sizeof(struct ci13xxx_qh) is 64 bytes.

> QH is 48-bytes structure, but needs 64-bytes aligned, it can't
> avoid wasting 16 bytes. 
Correct.

> So, please make sure the memory layout is correct for qh arrays.

I tested the code, it did not work without the aligned attribute.

> >     /* alloc resources */
> > -   ci->qh_pool = dma_pool_create("ci13xxx_qh", dev,
> > -                                  sizeof(struct ci13xxx_qh),
> > -                                  64, CI13XXX_PAGE_SIZE);
> > -   if (ci->qh_pool == NULL)
> > +   ci->qh.ptr = dma_alloc_coherent(dev, ci->qh.size,
> > +                                   &ci->qh.dma, GFP_ATOMIC);
> > +
> 
> GFP_ATMOIC isn't needed.

Correct, will fix.

> 
> > +   if (ci->qh.ptr == NULL)
> >             return -ENOMEM;
> >  
> >     ci->td_pool = dma_pool_create("ci13xxx_td", dev,
> > @@ -1814,7 +1814,6 @@ destroy_eps:
> >  free_pools:
> >     dma_pool_destroy(ci->td_pool);
> >  free_qh_pool:
> > -   dma_pool_destroy(ci->qh_pool);
> 
> You may need dma_free_coherent.

Will add that here.

> 
> >     return retval;
> >  }
> >  
> > @@ -1836,7 +1835,6 @@ static void udc_stop(struct ci13xxx *ci)
> >     destroy_eps(ci);
> >  
> >     dma_pool_destroy(ci->td_pool);
> > -   dma_pool_destroy(ci->qh_pool);
> >  
> >     if (!IS_ERR_OR_NULL(ci->transceiver)) {
> >             otg_set_peripheral(ci->transceiver->otg, NULL);
> > diff --git a/drivers/usb/chipidea/udc.h b/drivers/usb/chipidea/udc.h
> > index 4b1315a..b7d3882 100644
> > --- a/drivers/usb/chipidea/udc.h
> > +++ b/drivers/usb/chipidea/udc.h
> > @@ -55,7 +55,7 @@ struct ci13xxx_qh {
> >     /* 9 */
> >     u32 RESERVED;
> >     struct usb_ctrlrequest   setup;
> > -} __attribute__ ((packed, aligned(4)));
> > +} __attribute__ ((packed, aligned(64)));
> >  
> >  /**
> >   * struct ci13xxx_req - usb request representation
> > -- 
> > 1.8.2.rc2

Thanks for the review,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to