RE: [PATCH v2 03/30] usb: dwc2: Make the DMA descriptor structure packed

[David Laight]

From: Felipe Balbi
> Sent: 11 November 2016 10:45
> Hi,
> 
> John Youn  writes:
> > On 11/10/2016 4:28 AM, David Laight wrote:
> >> From: John Youn
> >>> Sent: 10 November 2016 03:28
> >>> From: Vahram Aharonyan 
> >>>
> >>> Make the DMA descriptor structure packed to guarantee alignment and size
> >>> in memory.
> >>>
> >>> Signed-off-by: John Youn 
> >>> ---
> >>>  drivers/usb/dwc2/hw.h | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
> >>> index 6031efe..ee827e8 100644
> >>> --- a/drivers/usb/dwc2/hw.h
> >>> +++ b/drivers/usb/dwc2/hw.h
> >>> @@ -802,7 +802,7 @@
> >>>  struct dwc2_dma_desc {
> >>>   u32 status;
> >>>   u32 buf;
> >>> -};
> >>> +} __packed;
> >>
> >> Nack (not that my nacks have much force).
> >>
> >> You clearly do not understand what __packed means.
> >>
> >> The dma descriptor almost certainly has to be 32bit aligned (if not 64).
> >>
> >> By adding __packed you are telling the compiler that the structure can be
> >> aligned to any boundary (eg 4n+1) so that it must use byte accesses and
> >> shifts to access the members on systems that can't do misaligned accesses.
> >> I can't help feeling this isn't what you want.
> >>
> >> I suspect you are trying to tell the compiler not to add hidden padding.
> >> While the C language allows arbitrary padding after structure elements,
> >> the implementations for specific architectures define when such padding
> >> is added.
> >> Padding will not be added if an item is already on its natural boundary
> >> in any of the sane architecture specific definitions.
> >>
> >> Typical variations are whether 64bit items only need 32bit alignment.
> >> Some odd architectures specify 32bit alignment for all structs (don't
> >> think linux has any like that).
> >>
> >> By using __packed you are relying on a compiler extension - and so
> >> can assume the compiler is used a sane structure layout.
> >> If you are being really picky you could add a compile-time assert
> >> that the structure is the correct size - but that isn't worth it
> >> for 8 bytes.
> >>
> >
> > Ok, maybe I did a poor job of wording the commit message and should
> > not have used "alignment" to describe what we are doing.
> >
> > We are only using __packed in the sense that the elements should be
> > "packed" together, i.e. not padded, so that we can provide it to the
> > hardware as-is, without any compiler muckery.
> >
> > Whether or not that's actually necessary for such a trivial
> > struct... probably not, but I don't know.
> >
> > Do you think it is still wrong in this context?
> 
> yeah, I don't think that __packed is necessary. However, __packed is a
> very good indication of "we're giving this to HW as is", so in that
> case, I don't see any harm in keeping it.

No, that is a completely wrong statement.
Only structures that can be misaligned in memory should ever be marked 
'__packed'.
Even then a 'realignment copy' could be a lot more efficient.

David



--
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 v2 03/30] usb: dwc2: Make the DMA descriptor structure packed

[Felipe Balbi]

Hi,

John Youn  writes:
> On 11/10/2016 4:28 AM, David Laight wrote:
>> From: John Youn
>>> Sent: 10 November 2016 03:28
>>> From: Vahram Aharonyan 
>>>
>>> Make the DMA descriptor structure packed to guarantee alignment and size
>>> in memory.
>>>
>>> Signed-off-by: John Youn 
>>> ---
>>>  drivers/usb/dwc2/hw.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
>>> index 6031efe..ee827e8 100644
>>> --- a/drivers/usb/dwc2/hw.h
>>> +++ b/drivers/usb/dwc2/hw.h
>>> @@ -802,7 +802,7 @@
>>>  struct dwc2_dma_desc {
>>> u32 status;
>>> u32 buf;
>>> -};
>>> +} __packed;
>> 
>> Nack (not that my nacks have much force).
>> 
>> You clearly do not understand what __packed means.
>> 
>> The dma descriptor almost certainly has to be 32bit aligned (if not 64).
>> 
>> By adding __packed you are telling the compiler that the structure can be
>> aligned to any boundary (eg 4n+1) so that it must use byte accesses and
>> shifts to access the members on systems that can't do misaligned accesses.
>> I can't help feeling this isn't what you want.
>> 
>> I suspect you are trying to tell the compiler not to add hidden padding.
>> While the C language allows arbitrary padding after structure elements,
>> the implementations for specific architectures define when such padding
>> is added.
>> Padding will not be added if an item is already on its natural boundary
>> in any of the sane architecture specific definitions.
>> 
>> Typical variations are whether 64bit items only need 32bit alignment.
>> Some odd architectures specify 32bit alignment for all structs (don't
>> think linux has any like that).
>> 
>> By using __packed you are relying on a compiler extension - and so
>> can assume the compiler is used a sane structure layout.
>> If you are being really picky you could add a compile-time assert
>> that the structure is the correct size - but that isn't worth it
>> for 8 bytes.
>> 
>
> Ok, maybe I did a poor job of wording the commit message and should
> not have used "alignment" to describe what we are doing.
>
> We are only using __packed in the sense that the elements should be
> "packed" together, i.e. not padded, so that we can provide it to the
> hardware as-is, without any compiler muckery.
>
> Whether or not that's actually necessary for such a trivial
> struct... probably not, but I don't know.
>
> Do you think it is still wrong in this context?

yeah, I don't think that __packed is necessary. However, __packed is a
very good indication of "we're giving this to HW as is", so in that
case, I don't see any harm in keeping it.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 03/30] usb: dwc2: Make the DMA descriptor structure packed

[John Youn]

On 11/10/2016 4:28 AM, David Laight wrote:
> From: John Youn
>> Sent: 10 November 2016 03:28
>> From: Vahram Aharonyan 
>>
>> Make the DMA descriptor structure packed to guarantee alignment and size
>> in memory.
>>
>> Signed-off-by: John Youn 
>> ---
>>  drivers/usb/dwc2/hw.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
>> index 6031efe..ee827e8 100644
>> --- a/drivers/usb/dwc2/hw.h
>> +++ b/drivers/usb/dwc2/hw.h
>> @@ -802,7 +802,7 @@
>>  struct dwc2_dma_desc {
>>  u32 status;
>>  u32 buf;
>> -};
>> +} __packed;
> 
> Nack (not that my nacks have much force).
> 
> You clearly do not understand what __packed means.
> 
> The dma descriptor almost certainly has to be 32bit aligned (if not 64).
> 
> By adding __packed you are telling the compiler that the structure can be
> aligned to any boundary (eg 4n+1) so that it must use byte accesses and
> shifts to access the members on systems that can't do misaligned accesses.
> I can't help feeling this isn't what you want.
> 
> I suspect you are trying to tell the compiler not to add hidden padding.
> While the C language allows arbitrary padding after structure elements,
> the implementations for specific architectures define when such padding
> is added.
> Padding will not be added if an item is already on its natural boundary
> in any of the sane architecture specific definitions.
> 
> Typical variations are whether 64bit items only need 32bit alignment.
> Some odd architectures specify 32bit alignment for all structs (don't
> think linux has any like that).
> 
> By using __packed you are relying on a compiler extension - and so
> can assume the compiler is used a sane structure layout.
> If you are being really picky you could add a compile-time assert
> that the structure is the correct size - but that isn't worth it
> for 8 bytes.
> 

Ok, maybe I did a poor job of wording the commit message and should
not have used "alignment" to describe what we are doing.

We are only using __packed in the sense that the elements should be
"packed" together, i.e. not padded, so that we can provide it to the
hardware as-is, without any compiler muckery.

Whether or not that's actually necessary for such a trivial
struct... probably not, but I don't know.

Do you think it is still wrong in this context?

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 v2 03/30] usb: dwc2: Make the DMA descriptor structure packed

[David Laight]

From: John Youn
> Sent: 10 November 2016 03:28
> From: Vahram Aharonyan 
> 
> Make the DMA descriptor structure packed to guarantee alignment and size
> in memory.
> 
> Signed-off-by: John Youn 
> ---
>  drivers/usb/dwc2/hw.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
> index 6031efe..ee827e8 100644
> --- a/drivers/usb/dwc2/hw.h
> +++ b/drivers/usb/dwc2/hw.h
> @@ -802,7 +802,7 @@
>  struct dwc2_dma_desc {
>   u32 status;
>   u32 buf;
> -};
> +} __packed;

Nack (not that my nacks have much force).

You clearly do not understand what __packed means.

The dma descriptor almost certainly has to be 32bit aligned (if not 64).

By adding __packed you are telling the compiler that the structure can be
aligned to any boundary (eg 4n+1) so that it must use byte accesses and
shifts to access the members on systems that can't do misaligned accesses.
I can't help feeling this isn't what you want.

I suspect you are trying to tell the compiler not to add hidden padding.
While the C language allows arbitrary padding after structure elements,
the implementations for specific architectures define when such padding
is added.
Padding will not be added if an item is already on its natural boundary
in any of the sane architecture specific definitions.

Typical variations are whether 64bit items only need 32bit alignment.
Some odd architectures specify 32bit alignment for all structs (don't
think linux has any like that).

By using __packed you are relying on a compiler extension - and so
can assume the compiler is used a sane structure layout.
If you are being really picky you could add a compile-time assert
that the structure is the correct size - but that isn't worth it
for 8 bytes.

David

--
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

[PATCH v2 03/30] usb: dwc2: Make the DMA descriptor structure packed

[John Youn]

From: Vahram Aharonyan 

Make the DMA descriptor structure packed to guarantee alignment and size
in memory.

Signed-off-by: John Youn 
---
 drivers/usb/dwc2/hw.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
index 6031efe..ee827e8 100644
--- a/drivers/usb/dwc2/hw.h
+++ b/drivers/usb/dwc2/hw.h
@@ -802,7 +802,7 @@
 struct dwc2_dma_desc {
u32 status;
u32 buf;
-};
+} __packed;
 
 #define HOST_DMA_A (1 << 31)
 #define HOST_DMA_STS_MASK  (0x3 << 28)
-- 
2.10.0

--
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