From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> To: David Miller <davem@davemloft.net>, mkl@pengutronix.de Cc: geert@linux-m68k.org, netdev@vger.kernel.org, wg@grandegger.com, linux-can@vger.kernel.org, linux-sh@vger.kernel.org, vksavl@gmail.com Subject: Re: [PATCH v5] can: add Renesas R-Car CAN driver Date: Mon, 20 Jan 2014 21:12:19 +0000 [thread overview] Message-ID: <52DD9F3C.7000408@cogentembedded.com> (raw) In-Reply-To: <20140120.111614.394228612374217980.davem@davemloft.net> Hello. On 01/20/2014 10:16 PM, David Miller wrote: >>>>>> - 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. >>> 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. >> 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 truly think using packed here is rediculous, please remove it unless > you can prove that things won't work without it. They will. But how about the following 'struct rcar_can_regs'? > Thanks. WBR, Sergei
WARNING: multiple messages have this Message-ID (diff)
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> To: David Miller <davem@davemloft.net>, mkl@pengutronix.de Cc: geert@linux-m68k.org, netdev@vger.kernel.org, wg@grandegger.com, linux-can@vger.kernel.org, linux-sh@vger.kernel.org, vksavl@gmail.com Subject: Re: [PATCH v5] can: add Renesas R-Car CAN driver Date: Tue, 21 Jan 2014 01:12:12 +0300 [thread overview] Message-ID: <52DD9F3C.7000408@cogentembedded.com> (raw) In-Reply-To: <20140120.111614.394228612374217980.davem@davemloft.net> Hello. On 01/20/2014 10:16 PM, David Miller wrote: >>>>>> - 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. >>> 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. >> 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 truly think using packed here is rediculous, please remove it unless > you can prove that things won't work without it. They will. But how about the following 'struct rcar_can_regs'? > Thanks. WBR, Sergei
next prev parent reply other threads:[~2014-01-20 21:12 UTC|newest] Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-12-26 20:37 [PATCH v5] can: add Renesas R-Car CAN driver Sergei Shtylyov 2013-12-26 21:37 ` Sergei Shtylyov 2014-01-13 13:46 ` Sergei Shtylyov 2014-01-13 13:46 ` Sergei Shtylyov 2014-01-20 9:18 ` Marc Kleine-Budde 2014-01-20 9:18 ` Marc Kleine-Budde 2014-01-25 0:34 ` Sergei Shtylyov 2014-01-25 1:34 ` Sergei Shtylyov 2014-02-13 12:12 ` Marc Kleine-Budde 2014-02-13 12:12 ` Marc Kleine-Budde 2014-02-20 22:48 ` Sergei Shtylyov 2014-02-20 23:48 ` Sergei Shtylyov 2014-02-28 9:08 ` Marc Kleine-Budde 2014-02-28 9:08 ` Marc Kleine-Budde 2014-02-28 11:16 ` Sergei Shtylyov 2014-02-28 11:16 ` Sergei Shtylyov 2014-02-28 11:37 ` Marc Kleine-Budde 2014-02-28 11:37 ` Marc Kleine-Budde 2014-02-28 11:41 ` Geert Uytterhoeven 2014-02-28 11:41 ` Geert Uytterhoeven 2014-02-28 11:47 ` David Laight 2014-02-28 11:47 ` David Laight 2014-02-28 11:50 ` Marc Kleine-Budde 2014-02-28 11:50 ` Marc Kleine-Budde 2014-02-28 12:02 ` David Laight 2014-02-28 12:02 ` David Laight 2014-02-28 11:49 ` Marc Kleine-Budde 2014-02-28 11:49 ` Marc Kleine-Budde 2014-02-28 12:05 ` Sergei Shtylyov 2014-02-28 12:05 ` Sergei Shtylyov 2014-02-28 12:17 ` David Laight 2014-02-28 12:17 ` David Laight 2014-02-28 12:34 ` Sergei Shtylyov 2014-02-28 12:34 ` Sergei Shtylyov 2014-01-20 11:43 ` Geert Uytterhoeven 2014-01-20 11:43 ` Geert Uytterhoeven 2014-01-20 11:47 ` Marc Kleine-Budde 2014-01-20 11:47 ` Marc Kleine-Budde 2014-01-20 11:52 ` Geert Uytterhoeven 2014-01-20 11:52 ` Geert Uytterhoeven 2014-01-20 11:58 ` Marc Kleine-Budde 2014-01-20 11:58 ` Marc Kleine-Budde 2014-01-20 12:02 ` Ben Dooks 2014-01-20 12:05 ` Geert Uytterhoeven 2014-01-20 12:05 ` Geert Uytterhoeven 2014-01-20 12:08 ` Marc Kleine-Budde 2014-01-20 12:08 ` Marc Kleine-Budde 2014-01-20 12:05 ` Marc Kleine-Budde 2014-01-20 12:05 ` Marc Kleine-Budde 2014-01-20 12:13 ` David Laight 2014-01-20 12:13 ` David Laight 2014-01-20 12:35 ` Marc Kleine-Budde 2014-01-20 12:35 ` Marc Kleine-Budde 2014-01-20 19:16 ` David Miller 2014-01-20 19:16 ` David Miller 2014-01-20 21:12 ` Sergei Shtylyov [this message] 2014-01-20 22:12 ` Sergei Shtylyov 2014-01-20 21:17 ` Marc Kleine-Budde 2014-01-20 21:17 ` Marc Kleine-Budde 2014-01-22 11:52 ` Ben Dooks 2014-01-22 11:54 ` Geert Uytterhoeven 2014-01-22 11:54 ` Geert Uytterhoeven 2014-01-22 11:58 ` David Laight 2014-01-22 11:58 ` David Laight 2014-01-20 12:12 ` Sergei Shtylyov 2014-01-20 12:12 ` Sergei Shtylyov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=52DD9F3C.7000408@cogentembedded.com \ --to=sergei.shtylyov@cogentembedded.com \ --cc=davem@davemloft.net \ --cc=geert@linux-m68k.org \ --cc=linux-can@vger.kernel.org \ --cc=linux-sh@vger.kernel.org \ --cc=mkl@pengutronix.de \ --cc=netdev@vger.kernel.org \ --cc=vksavl@gmail.com \ --cc=wg@grandegger.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.