Fix address alignment and return value casting, modify data types
and refactor the flow to be more clear and readable.
Eliav Farber (4):
EDAC: Fix calculation of returned address and next offset in
edac_align_ptr()
EDAC: Remove unnecessary cast to char* in edac_align_ptr() function
EDAC: Refactor edac_align_ptr() to use u8/u16/u32/u64 data types
EDAC: Refactor edac_align_ptr() flow
drivers/edac/edac_mc.c | 41 ++++++++++++++++++++---------------------
1 file changed, 20 insertions(+), 21 deletions(-)
--
2.32.0
Modify flow to be more clear:
- Calculate required alignment based on size.
- Check if *p is aligned and fix if not.
- Set return ptr to to be *p.
- Increase *p by new size for the next call.
Signed-off-by: Eliav Farber <[email protected]>
---
drivers/edac/edac_mc.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 3367bf997b73..a3ff5a019fc7 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -241,9 +241,7 @@ EXPORT_SYMBOL_GPL(edac_mem_types);
void *edac_align_ptr(void **p, unsigned size, int n_elems)
{
unsigned align, r;
- void *ptr = *p;
-
- *p += size * n_elems;
+ void *ptr;
/*
* 'p' can possibly be an unaligned item X such that sizeof(X) is
@@ -258,16 +256,22 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
else if (size > sizeof(u8))
align = sizeof(u16);
else
- return ptr;
-
- r = (unsigned long)ptr % align;
+ goto out;
- if (r == 0)
- return ptr;
+ /* Calculate alignment, and fix *p if not aligned. */
+ r = (unsigned long)*p % align;
+ if (r)
+ *p += align - r;
- *p += align - r;
+out:
+ /*
+ * Set return ptr to to be *p (after alignment if it was needed),
+ * and increase *p by new size for the next call.
+ */
+ ptr = *p;
+ *p += size * n_elems;
- return (void *)(((unsigned long)ptr) + align - r);
+ return ptr;
}
static void _edac_mc_free(struct mem_ctl_info *mci)
--
2.32.0
Prefer well defined size variables, that are same in size across all
systems.
Signed-off-by: Eliav Farber <[email protected]>
---
drivers/edac/edac_mc.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 8b9b86a7720a..3367bf997b73 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -250,18 +250,13 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
* 'size'. Adjust 'p' so that its alignment is at least as
* stringent as what the compiler would provide for X and return
* the aligned result.
- * Here we assume that the alignment of a "long long" is the most
- * stringent alignment that the compiler will ever provide by default.
- * As far as I know, this is a reasonable assumption.
*/
- if (size > sizeof(long))
- align = sizeof(long long);
- else if (size > sizeof(int))
- align = sizeof(long);
- else if (size > sizeof(short))
- align = sizeof(int);
- else if (size > sizeof(char))
- align = sizeof(short);
+ if (size > sizeof(u32))
+ align = sizeof(u64);
+ else if (size > sizeof(u16))
+ align = sizeof(u32);
+ else if (size > sizeof(u8))
+ align = sizeof(u16);
else
return ptr;
--
2.32.0
On Thu, Jan 13, 2022 at 10:06:21AM +0000, Eliav Farber wrote:
> Prefer well defined size variables, that are same in size across all
> systems.
>
> Signed-off-by: Eliav Farber <[email protected]>
> ---
> drivers/edac/edac_mc.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> index 8b9b86a7720a..3367bf997b73 100644
> --- a/drivers/edac/edac_mc.c
> +++ b/drivers/edac/edac_mc.c
> @@ -250,18 +250,13 @@ void *edac_align_ptr(void **p, unsigned size, int n_elems)
> * 'size'. Adjust 'p' so that its alignment is at least as
> * stringent as what the compiler would provide for X and return
> * the aligned result.
> - * Here we assume that the alignment of a "long long" is the most
> - * stringent alignment that the compiler will ever provide by default.
> - * As far as I know, this is a reasonable assumption.
> */
> - if (size > sizeof(long))
> - align = sizeof(long long);
> - else if (size > sizeof(int))
> - align = sizeof(long);
> - else if (size > sizeof(short))
> - align = sizeof(int);
> - else if (size > sizeof(char))
> - align = sizeof(short);
> + if (size > sizeof(u32))
> + align = sizeof(u64);
> + else if (size > sizeof(u16))
> + align = sizeof(u32);
> + else if (size > sizeof(u8))
> + align = sizeof(u16);
> else
> return ptr;
This is just silly. I think you should simply align on 8 and kill all
that bla.
This whole pointer alignment, then picking out the actual pointers of
the embedded struct members is just a bunch of unneeded complexity. I'd
like to get rid of it completely one day...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Jan 13, 2022 at 10:06:22AM +0000, Eliav Farber wrote:
> Modify flow to be more clear:
> - Calculate required alignment based on size.
> - Check if *p is aligned and fix if not.
> - Set return ptr to to be *p.
> - Increase *p by new size for the next call.
Right, as I said earlier, piling more on this crap design is not the
right thing to do. Looking at the call sites, what I think you should do
instead is simply compute the whole allocation size by making sure the
alignment of those substruct element pointers we're assigning to, is 8
or so, for simplicity.
You can store the offsets in those helper variables, like it is done
now.
Then do the allocation and add the offsets to the pointer kzalloc has
returned.
And then get rid of that edac_align_ptr() crap. This thing is silly
beyond repair and passing in that **p offset back'n'forth is making
stuff more complicated than it is - one can simply do that computation
before calling kzalloc and doesn't need a "helper" which ain't helping.
I'd say.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette