Re: [PATCH 02/30] usb: dwc2: Update DMA descriptor structure

2016-11-09 Thread John Youn 
On 11/9/2016 4:18 AM, David Laight wrote:
> From: John Youn
>> Sent: 08 November 2016 22:30
> ...
 Also make it __packed.
>>>
>>> why are you making it packed? Does it _have_ to be packed? If it must,
>>> why wasn't it before?
>>
>> We want to guarantee that it reflects the exact descriptor structure
>> in memory, so probably yes. It was overlooked before and worked
>> because the compiler likely packed it anyways. This is just to make
>> sure it does.
> ...
> 
> That isn't all that 'packed' means.
> 
> 'packed' also tells the compiler to access the structure with instructions 
> that
> will work if it isn't on the 'natural' boundary.

Good to know.

> 
> If you want to ensure the structure is the right size (pretty pointless here)
> then add a compile time assert against the structure size.

Doesn't 'packed' make that unnecessary as well?

Regards,
John
--
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

RE: [PATCH 02/30] usb: dwc2: Update DMA descriptor structure

2016-11-09 Thread David Laight 
From: John Youn
> Sent: 08 November 2016 22:30
...
> >> Also make it __packed.
> >
> > why are you making it packed? Does it _have_ to be packed? If it must,
> > why wasn't it before?
> 
> We want to guarantee that it reflects the exact descriptor structure
> in memory, so probably yes. It was overlooked before and worked
> because the compiler likely packed it anyways. This is just to make
> sure it does.
...

That isn't all that 'packed' means.

'packed' also tells the compiler to access the structure with instructions that
will work if it isn't on the 'natural' boundary.

If you want to ensure the structure is the right size (pretty pointless here)
then add a compile time assert against the structure size.

David

Re: [PATCH 02/30] usb: dwc2: Update DMA descriptor structure

2016-11-08 Thread John Youn 
On 11/8/2016 1:14 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> From: Vahram Aharonyan 
>>
>> Rename DMA descriptor structure from dwc2_hcd_dma_desc to dwc2_dma_desc
>> as it is applies to both host and gadget.
>>
>> Also make it __packed.
> 
> why are you making it packed? Does it _have_ to be packed? If it must,
> why wasn't it before?

We want to guarantee that it reflects the exact descriptor structure
in memory, so probably yes. It was overlooked before and worked
because the compiler likely packed it anyways. This is just to make
sure it does.

> 
> Also, it seems to me that adding __packed should be a separate patch.
> 

Ok.

Regards,
John
--
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

Re: [PATCH 02/30] usb: dwc2: Update DMA descriptor structure

2016-11-08 Thread Felipe Balbi 

Hi,

John Youn  writes:
> From: Vahram Aharonyan 
>
> Rename DMA descriptor structure from dwc2_hcd_dma_desc to dwc2_dma_desc
> as it is applies to both host and gadget.
>
> Also make it __packed.

why are you making it packed? Does it _have_ to be packed? If it must,
why wasn't it before?

Also, it seems to me that adding __packed should be a separate patch.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 02/30] usb: dwc2: Update DMA descriptor structure

2016-11-07 Thread John Youn 
From: Vahram Aharonyan 

Rename DMA descriptor structure from dwc2_hcd_dma_desc to dwc2_dma_desc
as it is applies to both host and gadget.

Also make it __packed.

Signed-off-by: Vahram Aharonyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/hcd.c  |  4 ++--
 drivers/usb/dwc2/hcd.h  |  2 +-
 drivers/usb/dwc2/hcd_ddma.c | 48 ++---
 drivers/usb/dwc2/hw.h   |  7 ---
 4 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 3ac0085..393a962 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -5110,7 +5110,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
if (hsotg->params.dma_desc_enable ||
hsotg->params.dma_desc_fs_enable) {
hsotg->desc_gen_cache = kmem_cache_create("dwc2-gen-desc",
-   sizeof(struct dwc2_hcd_dma_desc) *
+   sizeof(struct dwc2_dma_desc) *
MAX_DMA_DESC_NUM_GENERIC, 512, SLAB_CACHE_DMA,
NULL);
if (!hsotg->desc_gen_cache) {
@@ -5126,7 +5126,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
}
 
hsotg->desc_hsisoc_cache = kmem_cache_create("dwc2-hsisoc-desc",
-   sizeof(struct dwc2_hcd_dma_desc) *
+   sizeof(struct dwc2_dma_desc) *
MAX_DMA_DESC_NUM_HS_ISOC, 512, 0, NULL);
if (!hsotg->desc_hsisoc_cache) {
dev_err(hsotg->dev,
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 7758bfb..d92656d 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -348,7 +348,7 @@ struct dwc2_qh {
struct list_head qtd_list;
struct dwc2_host_chan *channel;
struct list_head qh_list_entry;
-   struct dwc2_hcd_dma_desc *desc_list;
+   struct dwc2_dma_desc *desc_list;
dma_addr_t desc_list_dma;
u32 desc_list_sz;
u32 *n_bytes;
diff --git a/drivers/usb/dwc2/hcd_ddma.c b/drivers/usb/dwc2/hcd_ddma.c
index 41e0a8d..70851b9 100644
--- a/drivers/usb/dwc2/hcd_ddma.c
+++ b/drivers/usb/dwc2/hcd_ddma.c
@@ -95,7 +95,7 @@ static int dwc2_desc_list_alloc(struct dwc2_hsotg *hsotg, 
struct dwc2_qh *qh,
else
desc_cache = hsotg->desc_gen_cache;
 
-   qh->desc_list_sz = sizeof(struct dwc2_hcd_dma_desc) *
+   qh->desc_list_sz = sizeof(struct dwc2_dma_desc) *
dwc2_max_desc_num(qh);
 
qh->desc_list = kmem_cache_zalloc(desc_cache, flags | GFP_DMA);
@@ -322,7 +322,7 @@ static void dwc2_release_channel_ddma(struct dwc2_hsotg 
*hsotg,
qh->ntd = 0;
 
if (qh->desc_list)
-   memset(qh->desc_list, 0, sizeof(struct dwc2_hcd_dma_desc) *
+   memset(qh->desc_list, 0, sizeof(struct dwc2_dma_desc) *
   dwc2_max_desc_num(qh));
 }
 
@@ -542,7 +542,7 @@ static void dwc2_fill_host_isoc_dma_desc(struct dwc2_hsotg 
*hsotg,
 struct dwc2_qh *qh, u32 max_xfer_size,
 u16 idx)
 {
-   struct dwc2_hcd_dma_desc *dma_desc = >desc_list[idx];
+   struct dwc2_dma_desc *dma_desc = >desc_list[idx];
struct dwc2_hcd_iso_packet_desc *frame_desc;
 
memset(dma_desc, 0, sizeof(*dma_desc));
@@ -571,8 +571,8 @@ static void dwc2_fill_host_isoc_dma_desc(struct dwc2_hsotg 
*hsotg,
 
dma_sync_single_for_device(hsotg->dev,
qh->desc_list_dma +
-   (idx * sizeof(struct dwc2_hcd_dma_desc)),
-   sizeof(struct dwc2_hcd_dma_desc),
+   (idx * sizeof(struct dwc2_dma_desc)),
+   sizeof(struct dwc2_dma_desc),
DMA_TO_DEVICE);
 }
 
@@ -645,8 +645,8 @@ static void dwc2_init_isoc_dma_desc(struct dwc2_hsotg 
*hsotg,
qh->desc_list[idx].status |= HOST_DMA_IOC;
dma_sync_single_for_device(hsotg->dev,
   qh->desc_list_dma + (idx *
-  sizeof(struct dwc2_hcd_dma_desc)),
-  sizeof(struct dwc2_hcd_dma_desc),
+  sizeof(struct dwc2_dma_desc)),
+  sizeof(struct dwc2_dma_desc),
   DMA_TO_DEVICE);
}
 #else
@@ -679,8 +679,8 @@ static void dwc2_init_isoc_dma_desc(struct dwc2_hsotg 
*hsotg,
qh->desc_list[idx].status |= HOST_DMA_IOC;
dma_sync_single_for_device(hsotg->dev,
   qh->desc_list_dma +
-  (idx * sizeof(struct dwc2_hcd_dma_desc)),
-