[PATCH v2] devres: Really align data field to unsigned long long
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.
That's because even on CPUs capable of non-aligned data access LL/SC
instructions require strict alignment.
Signed-off-by: Alexey Brodkin
Cc: Thomas Gleixner
Cc: sta...@vger.kernel.org
---
Changes v1 -> v2:
* Reworded commit message
* Inserted comment right in source [Thomas]
drivers/base/devres.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index f98a097e73f2..466fa59c866a 100644
--- 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.
+*/
+ unsigned long long data[] __aligned(sizeof(unsigned long
long));
};
struct devres_group {
--
2.17.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2] devres: Really align data field to unsigned long long
On Sun, Jul 08, 2018 at 08:56:21PM +0300, Alexey Brodkin wrote: > 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. > > That's because even on CPUs capable of non-aligned data access LL/SC > instructions require strict alignment. > > Signed-off-by: Alexey Brodkin > Cc: Thomas Gleixner > Cc: sta...@vger.kernel.org > --- > > Changes v1 -> v2: > > * Reworded commit message > * Inserted comment right in source [Thomas] > > drivers/base/devres.c | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Always use scripts/get_maintainer.pl to properly cc: the needed developer/maintainer. As it is, this patch is going to get dropped on the floor, sorry... greg k-h ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2] devres: Really align data field to unsigned long long
Hi Greg, вс, 8 июл. 2018 г. в 21:40, Greg KH : > > On Sun, Jul 08, 2018 at 08:56:21PM +0300, Alexey Brodkin wrote: > > 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. > > > > That's because even on CPUs capable of non-aligned data access LL/SC > > instructions require strict alignment. > > > > Signed-off-by: Alexey Brodkin > > Cc: Thomas Gleixner > > Cc: sta...@vger.kernel.org > > --- > > > > Changes v1 -> v2: > > > > * Reworded commit message > > * Inserted comment right in source [Thomas] > > > > drivers/base/devres.c | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > Always use scripts/get_maintainer.pl to properly cc: the needed > developer/maintainer. As it is, this patch is going to get dropped on > the floor, sorry... Right I was way too relaxed dealing with very generic stuff which might get not that much of attention as more narrow topics or subsystems. But anyways get_maintainer.pl says you're the guy so do I need to do anything extra still or it's OK for this time? -Alexey ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2] devres: Really align data field to unsigned long long
On Sun, Jul 08, 2018 at 09:45:31PM +0300, Алексей Бродкин wrote: > Hi Greg, > > вс, 8 июл. 2018 г. в 21:40, Greg KH : > > > > On Sun, Jul 08, 2018 at 08:56:21PM +0300, Alexey Brodkin wrote: > > > 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. > > > > > > That's because even on CPUs capable of non-aligned data access LL/SC > > > instructions require strict alignment. > > > > > > Signed-off-by: Alexey Brodkin > > > Cc: Thomas Gleixner > > > Cc: sta...@vger.kernel.org > > > --- > > > > > > Changes v1 -> v2: > > > > > > * Reworded commit message > > > * Inserted comment right in source [Thomas] > > > > > > drivers/base/devres.c | 8 ++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > Always use scripts/get_maintainer.pl to properly cc: the needed > > developer/maintainer. As it is, this patch is going to get dropped on > > the floor, sorry... > > Right I was way too relaxed dealing with very generic stuff which might get > not that much of attention as more narrow topics or subsystems. But anyways > get_maintainer.pl says you're the guy so do I need to do anything extra still > or > it's OK for this time? Please resend it properly, it is not in my patch queue anywhere... greg k-h ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[RESEND PATCH v2] devres: Really align data field to unsigned long long
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.
That's because even on CPUs capable of non-aligned data access LL/SC
instructions require strict alignment.
Signed-off-by: Alexey Brodkin
Cc: Greg Kroah-Hartman
Cc: Thomas Gleixner
Cc: sta...@vger.kernel.org
---
Changes v1 -> v2:
* Reworded commit message
* Inserted comment right in source [Thomas]
drivers/base/devres.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index f98a097e73f2..466fa59c866a 100644
--- 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.
+*/
+ unsigned long long data[] __aligned(sizeof(unsigned long
long));
};
struct devres_group {
--
2.17.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> 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.
So is this something you hit today? If not, why is this needed for
stable kernels?
> That's because even on CPUs capable of non-aligned data access LL/SC
> instructions require strict alignment.
Are you going to hit this code with all types of structures? What
happens when you do have an unaligned access?
>
> Signed-off-by: Alexey Brodkin
> Cc: Greg Kroah-Hartman
You didn't cc: this address :(
> Cc: Thomas Gleixner
> Cc: sta...@vger.kernel.org
> ---
>
> Changes v1 -> v2:
>
> * Reworded commit message
> * Inserted comment right in source [Thomas]
>
> drivers/base/devres.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index f98a097e73f2..466fa59c866a 100644
> --- 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.
> + */
> + unsigned long long data[] __aligned(sizeof(unsigned long
> long));
> };
Does this change the padding today for any other arches? If so, does
the increased memory usage cause any noticable issues?
thanks,
greg k-h
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long
Hi Greg,
On Mon, 2018-07-09 at 07:48 +0200, Greg KH wrote:
> On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > 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.
>
> So is this something you hit today? If not, why is this needed for
> stable kernels?
Indeed we hit that problem recently when Etnaviv driver was switched to
DRM GPU scheduler, see
commit e93b6deeb45a ("drm/etnaviv: hook up DRM GPU scheduler").
The most important part of DRM GPU scheduler is "job_id_count" member of
"drm_gpu_scheduler" structure of type "atomic64_t". This structure is put
in a buffer allocated by devm_kzalloc() and if "job_id_count" is not 64-bit
aligned atomic instruction fails with an exception.
As for stable requirements - mentioned commit was a part of 4.17 kernel
which broke GPU driver for one of our HSDK board so I guess back-porting
to 4.17 is a no-brainer.
Now given a nature of the problem at hand I may expect other breakages
whenever another driver/fs etc that use "atomic64_t" gets enabled.
> > That's because even on CPUs capable of non-aligned data access LL/SC
> > instructions require strict alignment.
>
> Are you going to hit this code with all types of structures?
If there're other cases which lead to 4-byte aligned "atomic64_t" variables
there will be a problem as well but it's quite hard to predict those cases.
That said if we manage to reproduce more similar issues there will be more
patches with fixes :)
> What happens when you do have an unaligned access?
Atomic instructions are a bit special as compared to normal loads and stores.
Even if normal loads and stores may deal with unaligned data atomic instructions
still require data to be aligned because it's hard to manage atomic value that
spans through multiple cache lines or even MMU pages. And hardware just
raises an alignment fault exception.
And that's not something special for ARC, I guess all CPUs are the same in
that regard, see here's an extract from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition: https://lkml.org/lkml/2017/12/5/440
>From "Table A3-1 Alignment requirements of load/store instructions"
it's seen that LDREXD, STREXD instructions will cause alignment fault
even if SCTLR.A=0 (strict alignment fault checking disabled) for non
double-word-aligned data.
> >
> > Signed-off-by: Alexey Brodkin
> > Cc: Greg Kroah-Hartman
>
> You didn't cc: this address :(
Sorry, which one?
That what I have in .mbox, see https://patchwork.ozlabs.org/patch/941092/mbox/:
-->8--
Cc: linux-a...@vger.kernel.org, Greg KH ,
Alexey Brodkin , sta...@vger.kernel.org,
Greg Kroah-Hartman ,
Thomas Gleixner , linux-snps-arc@lists.infradead.org
-->8--
-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
