From: Alexey Brodkin <Alexey.Brodkin@synopsys.com> To: "David.Laight@ACULAB.COM" <David.Laight@ACULAB.COM> Cc: "tglx@linutronix.de" <tglx@linutronix.de>, "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "greg@kroah.com" <greg@kroah.com>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "linux-snps-arc@lists.infradead.org" <linux-snps-arc@lists.infradead.org>, "stable@vger.kernel.org" <stable@vger.kernel.org> Subject: Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long Date: Mon, 9 Jul 2018 09:59:35 +0000 [thread overview] Message-ID: <c95ac7435736d93843c45628ba4d41a4a4988074.camel@synopsys.com> (raw) In-Reply-To: <f7499a811ba54c00a11f7525ae216213@AcuMS.aculab.com> Hi David, On Mon, 2018-07-09 at 09:16 +0000, David Laight wrote: > From: Alexey Brodkin > > Sent: 09 July 2018 05:45 > > Depending on ABI "long long" type of a particular 32-bit CPU > > might be aligned by either word (32-bits) or double word (64-bits). > > Make sure "data" is really 64-bit aligned for any 32-bit CPU. > > > > At least for 32-bit ARC cores ABI requires "long long" types > > to be aligned by normal 32-bit word. This makes "data" field aligned to > > 12 bytes. Which is still OK as long as we use 32-bit data only. > > > > But once we want to use native atomic64_t type (i.e. when we use special > > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit > > misaligned access exception. > > Shouldn't there be a typedef for the actual type. > Perhaps it is even atomic64_t ? > And have the __aligned(8) applied to that typedef ?? That's a good idea indeed but it doesn't solve the problem with struct devres_node. Consider the following snippet: -------------------------------->8------------------------------- struct mystruct { atomic64_t myvar; } struct mystruct *p; p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); -------------------------------->8------------------------------- Here myvar address will match address of "data" member of struct devres_node. So if "data" is has offset of 12 bytes from the beginning of a page then myvar won't be 64-bit aligned regardless of myvar's attribute, right? > > That's because even on CPUs capable of non-aligned data access LL/SC > > instructions require strict alignment. > > ... > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * Depending on ABI "long long" type of a particular 32-bit CPU > > + * might be aligned by either word (32-bits) or double word (64-bits). > > + * Make sure "data" is really 64-bit aligned for any 32-bit CPU. > > Just: > /* data[] must be 64 bit aligned even on 32 bit architectures > * because it might be accessed by instructions that require > * aligned memory arguments. > > > + */ > > + unsigned long long data[] __aligned(sizeof(unsigned long long)); > > One day assuming that 'unsigned long long' is exactly 64 bits will bite. > This probably ought to be u64 (or similar). I agree. Initially I wanted to keep as few changes as possible but IMHO switching to more predictable data type makes sense. -Alexey
WARNING: multiple messages have this Message-ID (diff)
From: Alexey.Brodkin@synopsys.com (Alexey Brodkin) To: linux-snps-arc@lists.infradead.org Subject: [RESEND PATCH v2] devres: Really align data field to unsigned long long Date: Mon, 9 Jul 2018 09:59:35 +0000 [thread overview] Message-ID: <c95ac7435736d93843c45628ba4d41a4a4988074.camel@synopsys.com> (raw) In-Reply-To: <f7499a811ba54c00a11f7525ae216213@AcuMS.aculab.com> Hi David, On Mon, 2018-07-09@09:16 +0000, David Laight wrote: > From: Alexey Brodkin > > Sent: 09 July 2018 05:45 > > Depending on ABI "long long" type of a particular 32-bit CPU > > might be aligned by either word (32-bits) or double word (64-bits). > > Make sure "data" is really 64-bit aligned for any 32-bit CPU. > > > > At least for 32-bit ARC cores ABI requires "long long" types > > to be aligned by normal 32-bit word. This makes "data" field aligned to > > 12 bytes. Which is still OK as long as we use 32-bit data only. > > > > But once we want to use native atomic64_t type (i.e. when we use special > > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit > > misaligned access exception. > > Shouldn't there be a typedef for the actual type. > Perhaps it is even atomic64_t ? > And have the __aligned(8) applied to that typedef ?? That's a good idea indeed but it doesn't solve the problem with struct devres_node. Consider the following snippet: -------------------------------->8------------------------------- struct mystruct { atomic64_t myvar; } struct mystruct *p; p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL); -------------------------------->8------------------------------- Here myvar address will match address of "data" member of struct devres_node. So if "data" is has offset of 12 bytes from the beginning of a page then myvar won't be 64-bit aligned regardless of myvar's attribute, right? > > That's because even on CPUs capable of non-aligned data access LL/SC > > instructions require strict alignment. > > ... > > --- a/drivers/base/devres.c > > +++ b/drivers/base/devres.c > > @@ -24,8 +24,12 @@ struct devres_node { > > > > struct devres { > > struct devres_node node; > > - /* -- 3 pointers */ > > - unsigned long long data[]; /* guarantee ull alignment */ > > + /* > > + * Depending on ABI "long long" type of a particular 32-bit CPU > > + * might be aligned by either word (32-bits) or double word (64-bits). > > + * Make sure "data" is really 64-bit aligned for any 32-bit CPU. > > Just: > /* data[] must be 64 bit aligned even on 32 bit architectures > * because it might be accessed by instructions that require > * aligned memory arguments. > > > + */ > > + unsigned long long data[] __aligned(sizeof(unsigned long long)); > > One day assuming that 'unsigned long long' is exactly 64 bits will bite. > This probably ought to be u64 (or similar). I agree. Initially I wanted to keep as few changes as possible but IMHO switching to more predictable data type makes sense. -Alexey
next prev parent reply other threads:[~2018-07-09 9:59 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-09 4:44 [RESEND PATCH v2] devres: Really align data field to unsigned long long Alexey Brodkin 2018-07-09 4:44 ` Alexey Brodkin 2018-07-09 5:48 ` Greg KH 2018-07-09 5:48 ` Greg KH 2018-07-09 6:46 ` Alexey Brodkin 2018-07-09 6:46 ` Alexey Brodkin 2018-07-09 6:46 ` Alexey Brodkin 2018-07-09 7:06 ` greg 2018-07-09 7:06 ` greg 2018-07-09 7:06 ` greg 2018-07-09 7:17 ` Alexey Brodkin 2018-07-09 7:17 ` Alexey Brodkin 2018-07-09 7:17 ` Alexey Brodkin 2018-07-09 7:33 ` greg 2018-07-09 7:33 ` greg 2018-07-09 7:33 ` greg 2018-07-09 7:07 ` Geert Uytterhoeven 2018-07-09 7:07 ` Geert Uytterhoeven 2018-07-09 7:22 ` Alexey Brodkin 2018-07-09 7:22 ` Alexey Brodkin 2018-07-09 7:22 ` Alexey Brodkin 2018-07-09 7:52 ` Geert Uytterhoeven 2018-07-09 7:52 ` Geert Uytterhoeven 2018-07-09 8:37 ` Alexey Brodkin 2018-07-09 8:37 ` Alexey Brodkin 2018-07-09 8:37 ` Alexey Brodkin 2018-07-09 8:37 ` Alexey Brodkin 2018-07-09 9:03 ` Geert Uytterhoeven 2018-07-09 9:03 ` Geert Uytterhoeven 2018-07-09 9:16 ` David Laight 2018-07-09 9:16 ` David Laight 2018-07-09 9:16 ` David Laight 2018-07-09 9:23 ` Geert Uytterhoeven 2018-07-09 9:23 ` Geert Uytterhoeven 2018-07-09 9:54 ` David Laight 2018-07-09 9:54 ` David Laight 2018-07-09 9:59 ` Alexey Brodkin [this message] 2018-07-09 9:59 ` Alexey Brodkin 2018-07-09 9:59 ` Alexey Brodkin 2018-07-09 10:18 ` David Laight 2018-07-09 10:18 ` David Laight 2018-07-09 10:18 ` David Laight 2018-07-09 10:23 ` Alexey Brodkin 2018-07-09 10:23 ` Alexey Brodkin 2018-07-09 10:23 ` Alexey Brodkin 2018-07-09 10:23 ` Alexey Brodkin 2018-07-09 18:27 ` Vineet Gupta 2018-07-09 18:27 ` Vineet Gupta 2018-07-09 18:27 ` Vineet Gupta 2018-07-10 6:42 ` Alexey Brodkin 2018-07-10 6:42 ` Alexey Brodkin 2018-07-10 6:42 ` Alexey Brodkin
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=c95ac7435736d93843c45628ba4d41a4a4988074.camel@synopsys.com \ --to=alexey.brodkin@synopsys.com \ --cc=David.Laight@ACULAB.COM \ --cc=greg@kroah.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-snps-arc@lists.infradead.org \ --cc=stable@vger.kernel.org \ --cc=tglx@linutronix.de \ /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.