netdev - RE: [PATCH v5] can: add Renesas R-Car CAN driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <063D6719AE5E284EB5DD2968C1650D6D460082@AcuExch.aculab.com>
Date:	Mon, 20 Jan 2014 12:13:23 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Ben Dooks' <ben.dooks@...ethink.co.uk>,
	Marc Kleine-Budde <mkl@...gutronix.de>
CC:	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"wg@...ndegger.com" <wg@...ndegger.com>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
	Linux-sh list <linux-sh@...r.kernel.org>,
	"vksavl@...il.com" <vksavl@...il.com>
Subject: RE: [PATCH v5] can: add Renesas R-Car CAN driver

From: 
> On 20/01/14 11:58, Marc Kleine-Budde wrote:
> > On 01/20/2014 12:52 PM, Geert Uytterhoeven wrote:
> >> On Mon, Jan 20, 2014 at 12:47 PM, Marc Kleine-Budde <mkl@...gutronix.de> wrote:
> >>> On 01/20/2014 12:43 PM, Geert Uytterhoeven wrote:
> >>>> On Thu, Dec 26, 2013 at 10:37 PM, Sergei Shtylyov
> >>>> <sergei.shtylyov@...entembedded.com> wrote:
> >>>>> Changes in version 3:
> >>>>
> >>>>> - added '__packed' to 'struct rcar_can_mbox_regs' and 'struct rcar_can_regs';
> >>>>> - removed unneeded type cast in the probe() method.
> >>>>
> >>>>> +/* 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.

> >> 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.

> > Yes, as this are registers they must not be accessed directly. However
> > we can use "__attribute__ ((packed, aligned(4)))" to tell the compiler
> > that the base address of this struct is always aligned to 4 bytes.
> 
> I thought we'd gotten past the stage of ever writing register
> definitions as structures?

Any other way is likely to be more error prone since nothing
ties the offsets with the correct base address.

IMHO it is best to only specify packed if the structure is expected
to be misaligned. Misaligned members can be 'fixed' by marking them
aligned().

Even then you might want to define the structure itself without
the 'packed' and the define a second packed structure containing
a single member.

eg: you might have:

struct a {
	u32	a_1;
	u64	a_2 __attribute__((aligned(4)));
	u32	a_3;
};

struct a_packed {
	struct a p;
} __attribute__((packed));

You might even need a union of the two types!

	David