From: Felipe Balbi > Sent: 11 November 2016 10:45 > Hi, > > John Youn <john.y...@synopsys.com> writes: > > On 11/10/2016 4:28 AM, David Laight wrote: > >> From: John Youn > >>> Sent: 10 November 2016 03:28 > >>> From: Vahram Aharonyan <vahr...@synopsys.com> > >>> > >>> Make the DMA descriptor structure packed to guarantee alignment and size > >>> in memory. > >>> > >>> Signed-off-by: John Youn <johny...@synopsys.com> > >>> --- > >>> 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