RE: [PATCH 1/2] EDAC/device: simplify info allocation

Zhuo, Qiuxu
2026-05-01T14:47:15Z

> From: Rosen Penev <rosenp@gmail.com>
> Sent: Friday, May 1, 2026 6:01 AM
> To: linux-edac@vger.kernel.org
> Cc: Borislav Petkov <bp@alien8.de>; Luck, Tony <tony.luck@intel.com>; Kees
> Cook <kees@kernel.org>; Gustavo A. R. Silva <gustavoars@kernel.org>; open
> list <linux-kernel@vger.kernel.org>; open list:KERNEL HARDENING (not
> covered by other areas):Keyword:\b__counted_by(_le|_be)?\b <linux-
> hardening@vger.kernel.org>
> Subject: [PATCH 1/2] EDAC/device: simplify info allocation
>
> Use a flexible array member to combine info and instance allocations.
> Simplifies allocation slightly.
>
> Add __counted_by for extra runtime analysis. Move counting variable
> assignment to after allocation as kzalloc_flex already does with GCC >= 15.

I'm confused - I don't see kzalloc_flex()used in this patch.

>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
> drivers/edac/edac_device.c | 23 ++++++++---------------
> drivers/edac/edac_device.h | 4 +---
> 2 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c index
> cf0d3c2dfc04..9c21997a50e0 100644
> --- a/drivers/edac/edac_device.c
> +++ b/drivers/edac/edac_device.c
> @@ -58,31 +58,25 @@ edac_device_alloc_ctl_info(unsigned pvt_sz, char
> *dev_name, unsigned nr_instance
> char *blk_name, unsigned nr_blocks, unsigned
> off_val,
> int device_index)
> {
> - struct edac_device_block *dev_blk, *blk_p, *blk;
> + struct edac_device_block *blk_p, *blk;
> struct edac_device_instance *dev_inst, *inst;
> struct edac_device_ctl_info *dev_ctl;
> unsigned instance, block;
> + size_t alloc_size;
> void *pvt;
> int err;
>
> edac_dbg(4, "instances=%d blocks=%d\n", nr_instances, nr_blocks);
>
> - dev_ctl = kzalloc_obj(struct edac_device_ctl_info);
> + alloc_size = struct_size(dev_ctl, instances, nr_instances);
> + alloc_size += sizeof(*dev_ctl->blocks) * nr_instances * nr_blocks;
> + dev_ctl = kzalloc(alloc_size, GFP_KERNEL);
> if (!dev_ctl)
> return NULL;
>
> - dev_inst = kzalloc_objs(struct edac_device_instance, nr_instances);
> - if (!dev_inst)
> - goto free;
> -
> - dev_ctl->instances = dev_inst;
> -
> - dev_blk = kzalloc_objs(struct edac_device_block,
> - nr_instances * nr_blocks);
> - if (!dev_blk)
> - goto free;
> + dev_ctl->nr_instances = nr_instances;
>
> - dev_ctl->blocks = dev_blk;
> + dev_ctl->blocks = (struct edac_device_block *)(dev_ctl->instances +
> +nr_instances);
>

1. This assumes that the address
'dev_ctl->instances +nr_instances' is properly aligned for struct edac_device_block,
but that is not guaranteed.

2. Using a flexible array member for a single structure member can be reasonable and easy to read.
But this patch effectively turns two members into flexible arrays and requires manual address calculation for
one of them (and there is alignment risk). This makes code less readable to me.
I'd prefer old code, as it's easier to follow ownership and no pointer alignment risk.

> if (pvt_sz) {
> pvt = kzalloc(pvt_sz, GFP_KERNEL);
> @@ -93,7 +87,6 @@ edac_device_alloc_ctl_info(unsigned pvt_sz, char
> *dev_name, unsigned nr_instance
> }
>
> dev_ctl->dev_idx = device_index;
> - dev_ctl->nr_instances = nr_instances;
>
> /* Default logging of CEs and UEs */
> dev_ctl->log_ce = 1;
> @@ -107,7 +100,7 @@ edac_device_alloc_ctl_info(unsigned pvt_sz, char
> *dev_name, unsigned nr_instance
> inst = &dev_inst[instance];

BUG: 'dev_inst' is used w/o initialization. …