All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <David.Laight@ACULAB.COM>
To: 'Ben Dooks' <ben.dooks@codethink.co.uk>,
	Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
	David Miller <davem@davemloft.net>,
	"geert@linux-m68k.org" <geert@linux-m68k.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"wg@grandegger.com" <wg@grandegger.com>,
	"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
	"linux-sh@vger.kernel.org" <linux-sh@vger.kernel.org>,
	"vksavl@gmail.com" <vksavl@gmail.com>
Subject: RE: [PATCH v5] can: add Renesas R-Car CAN driver
Date: Wed, 22 Jan 2014 11:58:50 +0000	[thread overview]
Message-ID: <063D6719AE5E284EB5DD2968C1650D6D463994@AcuExch.aculab.com> (raw)
In-Reply-To: <52DFB101.8040907@codethink.co.uk>

RnJvbTogQmVuIERvb2tzDQo+IE9uIDIwLzAxLzE0IDIxOjE3LCBNYXJjIEtsZWluZS1CdWRkZSB3
cm90ZToNCj4gPiBPbiAwMS8yMC8yMDE0IDExOjEyIFBNLCBTZXJnZWkgU2h0eWx5b3Ygd3JvdGU6
DQo+ID4gWy4uLl0NCj4gPg0KPiA+Pj4gSSB0cnVseSB0aGluayB1c2luZyBwYWNrZWQgaGVyZSBp
cyByZWRpY3Vsb3VzLCBwbGVhc2UgcmVtb3ZlIGl0IHVubGVzcw0KPiA+Pj4geW91IGNhbiBwcm92
ZSB0aGF0IHRoaW5ncyB3b24ndCB3b3JrIHdpdGhvdXQgaXQuDQo+ID4+DQo+ID4+ICAgICBUaGV5
IHdpbGwuIEJ1dCBob3cgYWJvdXQgdGhlIGZvbGxvd2luZyAnc3RydWN0IHJjYXJfY2FuX3JlZ3Mn
Pw0KPiA+DQo+ID4gVGhlIHN0cmN1dHMgaW4gcXVlc3Rpb24gYXJlOg0KPiA+DQo+ID4+ICsvKiBN
YWlsYm94IHJlZ2lzdGVycyBzdHJ1Y3R1cmUgKi8NCj4gPj4gK3N0cnVjdCByY2FyX2Nhbl9tYm94
X3JlZ3Mgew0KPiA+PiArCXUzMiBpZDsJCS8qIElERSBhbmQgUlRSIGJpdHMsIFNJRCBhbmQgRUlE
ICovDQo+ID4+ICsJdTggc3R1YjsJLyogTm90IHVzZWQgKi8NCj4gPj4gKwl1OCBkbGM7CQkvKiBE
YXRhIExlbmd0aCBDb2RlIC0gYml0cyBbMC4uM10gKi8NCj4gPj4gKwl1OCBkYXRhWzhdOwkvKiBE
YXRhIEJ5dGVzICovDQo+ID4+ICsJdTggdHNoOwkJLyogVGltZSBTdGFtcCBIaWdoZXIgQnl0ZSAq
Lw0KPiA+PiArCXU4IHRzbDsJCS8qIFRpbWUgU3RhbXAgTG93ZXIgQnl0ZSAqLw0KPiA+PiArfSBf
X3BhY2tlZDsNCj4gPj4gKw0KPiA+PiArc3RydWN0IHJjYXJfY2FuX3JlZ3Mgew0KPiA+PiArCXN0
cnVjdCByY2FyX2Nhbl9tYm94X3JlZ3MgbWJbUkNBUl9DQU5fTl9NQlhdOyAvKiBNYWlsYm94IHJl
Z2lzdGVycyAqLw0KPiA+PiArCXUzMiBta3JfMl85WzhdOwkvKiBNYXNrIFJlZ2lzdGVycyAyLTkg
Ki8NCj4gPj4gKwl1MzIgZmlkY3JbMl07CS8qIEZJRk8gUmVjZWl2ZWQgSUQgQ29tcGFyZSBSZWdp
c3RlciAqLw0KPiA+PiArCXUzMiBta2l2bHIxOwkvKiBNYXNrIEludmFsaWQgUmVnaXN0ZXIgMSAq
Lw0KPiA+PiArCXUzMiBtaWVyMTsJLyogTWFpbGJveCBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVy
IDEgKi8NCj4gPj4gKwl1MzIgbWtyXzBfMVsyXTsJLyogTWFzayBSZWdpc3RlcnMgMC0xICovDQo+
ID4+ICsJdTMyIG1raXZscjA7ICAgIC8qIE1hc2sgSW52YWxpZCBSZWdpc3RlciAwKi8NCj4gPj4g
Kwl1MzIgbWllcjA7ICAgICAgLyogTWFpbGJveCBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVyIDAg
Ki8NCj4gPj4gKwl1OCBwYWRfNDQwWzB4M2MwXTsNCj4gPj4gKwl1OCBtY3RsWzY0XTsJLyogTWVz
c2FnZSBDb250cm9sIFJlZ2lzdGVycyAqLw0KPiA+PiArCXUxNiBjdGxyOwkvKiBDb250cm9sIFJl
Z2lzdGVyICovDQo+ID4+ICsJdTE2IHN0cjsJLyogU3RhdHVzIHJlZ2lzdGVyICovDQo+ID4+ICsJ
dTggYmNyWzNdOwkvKiBCaXQgQ29uZmlndXJhdGlvbiBSZWdpc3RlciAqLw0KPiA+PiArCXU4IGNs
a3I7CS8qIENsb2NrIFNlbGVjdCBSZWdpc3RlciAqLw0KPiA+PiArCXU4IHJmY3I7CS8qIFJlY2Vp
dmUgRklGTyBDb250cm9sIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggcmZwY3I7CS8qIFJlY2VpdmUg
RklGTyBQb2ludGVyIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCB0ZmNyOwkvKiBUcmFu
c21pdCBGSUZPIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCB0ZnBjcjsgICAgICAgLyog
VHJhbnNtaXQgRklGTyBQb2ludGVyIENvbnRyb2wgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCBlaWVy
OwkvKiBFcnJvciBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggZWlmcjsJ
LyogRXJyb3IgSW50ZXJydXB0IEZhY3RvciBKdWRnZSBSZWdpc3RlciAqLw0KPiA+PiArCXU4IHJl
Y3I7CS8qIFJlY2VpdmUgRXJyb3IgQ291bnQgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCB0ZWNyOyAg
ICAgICAgLyogVHJhbnNtaXQgRXJyb3IgQ291bnQgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1OCBlY3Ny
OwkvKiBFcnJvciBDb2RlIFN0b3JlIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggY3NzcjsJLyogQ2hh
bm5lbCBTZWFyY2ggU3VwcG9ydCBSZWdpc3RlciAqLw0KPiA+PiArCXU4IG1zc3I7CS8qIE1haWxi
b3ggU2VhcmNoIFN0YXR1cyBSZWdpc3RlciAqLw0KPiA+PiArCXU4IG1zbXI7CS8qIE1haWxib3gg
U2VhcmNoIE1vZGUgUmVnaXN0ZXIgKi8NCj4gPj4gKwl1MTYgdHNyOwkvKiBUaW1lIFN0YW1wIFJl
Z2lzdGVyICovDQo+ID4+ICsJdTggYWZzcjsJLyogQWNjZXB0YW5jZSBGaWx0ZXIgU3VwcG9ydCBS
ZWdpc3RlciAqLw0KPiA+PiArCXU4IHBhZF84NTc7DQo+ID4+ICsJdTggdGNyOwkJLyogVGVzdCBD
b250cm9sIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggcGFkXzg1OVs3XTsNCj4gPj4gKwl1OCBpZXI7
CQkvKiBJbnRlcnJ1cHQgRW5hYmxlIFJlZ2lzdGVyICovDQo+ID4+ICsJdTggaXNyOwkJLyogSW50
ZXJydXB0IFN0YXR1cyBSZWdpc3RlciAqLw0KPiA+PiArCXU4IHBhZF84NjI7DQo+ID4+ICsJdTgg
bWJzbXI7CS8qIE1haWxib3ggU2VhcmNoIE1hc2sgUmVnaXN0ZXIgKi8NCj4gPj4gK30gX19wYWNr
ZWQ7DQo+ID4NCj4gPiBJIHRoaW5rIHRoZXkgc2hvdWxkIHdvcmsgd2l0aG91dCBwYWNrZWQsIHRv
by4NCj4gDQo+IEkgdGhpbmsgdGhpcyBkaXNjdXNzaW9uIHByb3ZlcyB3aHkgdGhpcyBpcyBub3Qg
YSBnb29kIGlkZWEuDQoNCllvdSBuZWVkIHRoZSBzZWNvbmQgc3RydWN0dXJlIHRvIGJlIHRoZSBj
b3JyZWN0IHNpemUsIHdpdGggb3Igd2l0aG91dA0KX19wYWNrZWQgdGhlcmUgY291bGQgZWFzaWx5
IGJlIGEgdHlwbyAtIHNvIGFkZCBhIGNvbXBpbGUtdHlwZSBhc3NlcnQNCm9uIHRoZSBzaXplLg0K
DQpJZiB0aGlzIG1hdGNoZXMgc29tZSBoYXJkd2FyZSBzcGVjLCBhbmQgdGhlIGhhcmR3YXJlIHNw
ZWMgZG9lc24ndA0KaGF2ZSBhbnl0aGluZyBtaXNhbGlnbmVkLCB0aGVuIHlvdSBkb24ndCB3YW50
IHRvIHNwZWNpZnkgX19wYWNrZWQuDQpXaXRob3V0IHRoZSBfX3BhY2tlZCB0aGUgc2l6ZSBjaGVj
ayB3aWxsIGRldGVjdCBtb3JlIHR5cG9zLg0KDQo+IElJUkMsIGEgY29tcGlsZXIgaGFzIHRoZSBy
aWdodCB0byBwYWQsIG9yIHJlLW9yZGVyIHN0cnVjdHVyZQ0KPiBlbGVtZW50cyBhcyBpdCBzZWVz
IGZpdCBkZXBlbmRpbmcgb24gdGhlIGFyY2hpdGVjdHVyZSBhbmQgb3B0aW9ucy4NCg0KVGhlIGxh
bmd1YWdlIG1pZ2h0IGFsbG93IG1hbnkgdGhpbmdzLCB0aGUgQUJJIGlzIG11Y2ggbW9yZSBleHBs
aWNpdC4NCkluIHRoaXMgY2FzZSB5b3UgcmVhbGx5IGNhcmUgYWJvdXQgdGhlIEFCSS4NCg0KCURh
dmlkDQoNCg=

WARNING: multiple messages have this Message-ID (diff)
From: David Laight <David.Laight@ACULAB.COM>
To: 'Ben Dooks' <ben.dooks@codethink.co.uk>,
	Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
	David Miller <davem@davemloft.net>,
	"geert@linux-m68k.org" <geert@linux-m68k.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"wg@grandegger.com" <wg@grandegger.com>,
	"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
	"linux-sh@vger.kernel.org" <linux-sh@vger.kernel.org>,
	"vksavl@gmail.com" <vksavl@gmail.com>
Subject: RE: [PATCH v5] can: add Renesas R-Car CAN driver
Date: Wed, 22 Jan 2014 11:58:50 +0000	[thread overview]
Message-ID: <063D6719AE5E284EB5DD2968C1650D6D463994@AcuExch.aculab.com> (raw)
In-Reply-To: <52DFB101.8040907@codethink.co.uk>

From: Ben Dooks
> On 20/01/14 21:17, Marc Kleine-Budde wrote:
> > On 01/20/2014 11:12 PM, Sergei Shtylyov wrote:
> > [...]
> >
> >>> 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'?
> >
> > The strcuts in question are:
> >
> >> +/* 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;
> >> +
> >> +struct rcar_can_regs {
> >> +	struct rcar_can_mbox_regs mb[RCAR_CAN_N_MBX]; /* Mailbox registers */
> >> +	u32 mkr_2_9[8];	/* Mask Registers 2-9 */
> >> +	u32 fidcr[2];	/* FIFO Received ID Compare Register */
> >> +	u32 mkivlr1;	/* Mask Invalid Register 1 */
> >> +	u32 mier1;	/* Mailbox Interrupt Enable Register 1 */
> >> +	u32 mkr_0_1[2];	/* Mask Registers 0-1 */
> >> +	u32 mkivlr0;    /* Mask Invalid Register 0*/
> >> +	u32 mier0;      /* Mailbox Interrupt Enable Register 0 */
> >> +	u8 pad_440[0x3c0];
> >> +	u8 mctl[64];	/* Message Control Registers */
> >> +	u16 ctlr;	/* Control Register */
> >> +	u16 str;	/* Status register */
> >> +	u8 bcr[3];	/* Bit Configuration Register */
> >> +	u8 clkr;	/* Clock Select Register */
> >> +	u8 rfcr;	/* Receive FIFO Control Register */
> >> +	u8 rfpcr;	/* Receive FIFO Pointer Control Register */
> >> +	u8 tfcr;	/* Transmit FIFO Control Register */
> >> +	u8 tfpcr;       /* Transmit FIFO Pointer Control Register */
> >> +	u8 eier;	/* Error Interrupt Enable Register */
> >> +	u8 eifr;	/* Error Interrupt Factor Judge Register */
> >> +	u8 recr;	/* Receive Error Count Register */
> >> +	u8 tecr;        /* Transmit Error Count Register */
> >> +	u8 ecsr;	/* Error Code Store Register */
> >> +	u8 cssr;	/* Channel Search Support Register */
> >> +	u8 mssr;	/* Mailbox Search Status Register */
> >> +	u8 msmr;	/* Mailbox Search Mode Register */
> >> +	u16 tsr;	/* Time Stamp Register */
> >> +	u8 afsr;	/* Acceptance Filter Support Register */
> >> +	u8 pad_857;
> >> +	u8 tcr;		/* Test Control Register */
> >> +	u8 pad_859[7];
> >> +	u8 ier;		/* Interrupt Enable Register */
> >> +	u8 isr;		/* Interrupt Status Register */
> >> +	u8 pad_862;
> >> +	u8 mbsmr;	/* Mailbox Search Mask Register */
> >> +} __packed;
> >
> > I think they should work without packed, too.
> 
> I think this discussion proves why this is not a good idea.

You need the second structure to be the correct size, with or without
__packed there could easily be a typo - so add a compile-type assert
on the size.

If this matches some hardware spec, and the hardware spec doesn't
have anything misaligned, then you don't want to specify __packed.
Without the __packed the size check will detect more typos.

> IIRC, a compiler has the right to pad, or re-order structure
> elements as it sees fit depending on the architecture and options.

The language might allow many things, the ABI is much more explicit.
In this case you really care about the ABI.

	David


  parent reply	other threads:[~2014-01-22 11:58 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
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 [this message]
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=063D6719AE5E284EB5DD2968C1650D6D463994@AcuExch.aculab.com \
    --to=david.laight@aculab.com \
    --cc=ben.dooks@codethink.co.uk \
    --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=sergei.shtylyov@cogentembedded.com \
    --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: link
Be 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.