From: Felipe Balbi
> Sent: 11 November 2016 10:45
> 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.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
