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?
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
