| Message ID | 20200516162115.16545-1-wata2ki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/mc: Fix memory alignment calculation formula | expand |
On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote: > From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp> > > During the development of the off-tree driver, Wait, what? Am I reading this correctly that you have an out-of-tree EDAC driver? If so, why? Why not submit it upstream? Thx.
Hi Out of tree driver (edac_injection) is under developing now by Gabriele Paoloni. This driver will upstream future. When I was porting this driver to aarch64 environment, I found this bug. This bug is also common bug for other edac me drivers. My opinion, this bug should be fixed instead of waiting for the driver to be developed. Because alignment miss may only cause performance degradation in case of Intel, but it cause CPU exceptions (Oops/kernel panic) in case of aarch64 and other risc like architecture. This bug was supposed to be fixed in commit(a). But this fix conflict with the commit(b) fix and consequently the bug was not fixed. commit(a): https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/edac/edac_mc.c?h=linux-3.7.y&id=8447c4d15e357a458c9051ddc84aa6c8b9c27000 commit(b): https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/edac/edac_mc.c?h=linux-3.7.y&id=93e4fe64ece4eccf0ff4ac69bceb389290b8ab7c Thanks 2020年6月3日(水) 20:28 Borislav Petkov <bp@alien8.de>: > > On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote: > > From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp> > > > > During the development of the off-tree driver, > > Wait, what? > > Am I reading this correctly that you have an out-of-tree EDAC driver? > > If so, why? Why not submit it upstream? > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Jun 03, 2020 at 10:00:01PM +0900, Naoto YAMAGUCHI wrote: > Out of tree driver (edac_injection) is under developing now by Gabriele > Paoloni. This driver will upstream future. Ah ok. I thought someone is running an out-of-tree EDAC driver for no good reason. > When I was porting this driver to aarch64 environment, I found this bug. I'll have a look after the merge window, thanks. Btw, please do not top-post. Thx.
On Sun, May 17, 2020 at 01:21:15AM +0900, wata2ki wrote: > From: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp> > > During the development of the off-tree driver, we found a bug that > causes alignment fault exception in mutex_lock. > > Line of the code: > ffffffc010536ce4: c85ffe62 ldaxr x2, [x19] > > Register value: > x19: ffffff800e90f6c4 > > This problem was caused by the alignment error of pvt_info > in struct mem_ctl_info. It is caused by a calculation formula > error in edac_align_ptr. > > Existing calculation formula is using variable p, but this > variable is address of the pointer variable not memory offset. > In this calculation formula should use *p. > > Signed-off-by: Naoto Yamaguchi <i33399_YAMAGUCHI@aisin-aw.co.jp> > --- > drivers/edac/edac_mc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > mode change 100644 => 100755 drivers/edac/edac_mc.c > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > old mode 100644 > new mode 100755 > index 75ede27bdf6a..70929f136dd7 > --- a/drivers/edac/edac_mc.c > +++ b/drivers/edac/edac_mc.c > @@ -271,7 +271,7 @@ void *edac_align_ptr(void **p, unsigned int size, int n_elems) > else > return (char *)ptr; > > - r = (unsigned long)p % align; > + r = (unsigned long)(*p) % align; Looks about right to me. Btw, you don't need the () around *p - that's evaluated right-to-left so the dereference happens first and *then* the typecast, i.e., what you want here. In any case, this line comes from 8447c4d15e35 ("edac: Do alignment logic properly in edac_align_ptr()") and I believe it was wrong to use 'p' as that function works with the memory offsets - not with the pointer to the pointer. It's a whole different story whether I think this whole thing makes sense and it is ugly... Anyway, adding the gentlemen from that commit to Cc.
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c old mode 100644 new mode 100755 index 75ede27bdf6a..70929f136dd7 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -271,7 +271,7 @@ void *edac_align_ptr(void **p, unsigned int size, int n_elems) else return (char *)ptr; - r = (unsigned long)p % align; + r = (unsigned long)(*p) % align; if (r == 0) return (char *)ptr;