On 01/20/2014 01:13 PM, David Laight wrote:

From: 

quoted

On 20/01/14 11:58, Marc Kleine-Budde wrote:

quoted

On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:

quoted

On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde [off-list ref] wrote:

quoted

On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:

quoted

On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
[off-list ref] wrote:

quoted

Changes in version 3:

quoted

- added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
- removed unneeded type cast in the probe() method.

quoted

+/* Mailbox registers structure */
+struct rcar_can_mbox_regs {
+       u32 id;         /* IDE and RTR bits, SID and EID */
+       u8 stub;        /* Not used */
+       u8 dlc;         /* Data Length Code - bits [0..3] */
+       u8 data[8];     /* Data Bytes */
+       u8 tsh;         /* Time Stamp Higher Byte */
+       u8 tsl;         /* Time Stamp Lower Byte */
+} __packed;

Sorry, I missed the earlier discussion, but why the _packed?
One u32 and 12 bytes makes a nice multiple of 4.

Better safe than sorry, it's the layout of the registers in hardware,
don't let the compiler optimize here.

Why not just add a compile time check against the size of the
structure - that will ensure that no padding is accidentally added.

And what do when the check fails? Add an the __packed (again)? :D

quoted

quoted

quoted

Actually __packed makes it less safe, as the compiler now assumes
the u32 id member is unaligned, and thus may turn 32-bit accesses into 4
byte accesses.

Fortunately it won't happen in this case as the code uses writel/readl to
acces the id member.

Which means that it will be aligned (and must be aligned).
So the packed is completely useless and pointless.

Then we'll remove the packed.

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |