Hi,
John Youn <John.Youn@xxxxxxxxxxxx> writes:
> On 11/10/2016 4:28 AM, David Laight wrote:
>> From: John Youn
>>> Sent: 10 November 2016 03:28
>>> From: Vahram Aharonyan <vahrama@xxxxxxxxxxxx>
>>>
>>> Make the DMA descriptor structure packed to guarantee alignment and size
>>> in memory.
>>>
>>> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
>>> ---
>>> 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
Attachment:
signature.asc
Description: PGP signature
