[PATCH] EDAC: sb_edac: use kzalloc_flex

[PATCH] EDAC: sb_edac: use kzalloc_flex

[Rosen Penev]

Simplifies allocations by using a flexible array member in this struct.

Add __counted_by to get extra runtime analysis.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/edac/sb_edac.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 09d4e816404b..7b282dfd093f 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -364,11 +364,11 @@ struct sbridge_dev {
 	int			seg;
 	u8			bus, mc;
 	u8			node_id, source_id;
-	struct pci_dev		**pdev;
 	enum domain		dom;
 	int			n_devs;
 	int			i_devs;
 	struct mem_ctl_info	*mci;
+	struct pci_dev		*pdev[] __counted_by(n_devs);
 };
 
 struct knl_pvt {
@@ -771,21 +771,14 @@ static struct sbridge_dev *alloc_sbridge_dev(int seg, u8 bus, enum domain dom,
 {
 	struct sbridge_dev *sbridge_dev;
 
-	sbridge_dev = kzalloc_obj(*sbridge_dev);
+	sbridge_dev = kzalloc_flex(*sbridge_dev, pdev, table->n_devs_per_imc);
 	if (!sbridge_dev)
 		return NULL;
 
-	sbridge_dev->pdev = kzalloc_objs(*sbridge_dev->pdev,
-					 table->n_devs_per_imc);
-	if (!sbridge_dev->pdev) {
-		kfree(sbridge_dev);
-		return NULL;
-	}
-
+	sbridge_dev->n_devs = table->n_devs_per_imc;
 	sbridge_dev->seg = seg;
 	sbridge_dev->bus = bus;
 	sbridge_dev->dom = dom;
-	sbridge_dev->n_devs = table->n_devs_per_imc;
 	list_add_tail(&sbridge_dev->list, &sbridge_edac_list);
 
 	return sbridge_dev;
@@ -794,7 +787,6 @@ static struct sbridge_dev *alloc_sbridge_dev(int seg, u8 bus, enum domain dom,
 static void free_sbridge_dev(struct sbridge_dev *sbridge_dev)
 {
 	list_del(&sbridge_dev->list);
-	kfree(sbridge_dev->pdev);
 	kfree(sbridge_dev);
 }
 
-- 
2.53.0

RE: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Luck, Tony]

+	sbridge_dev->n_devs = table->n_devs_per_imc;

Do you need this? I thought that kzalloc_flex() filled in the __counted_by field of the structure for you.

-Tony

Re: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Rosen Penev]

On Wed, Mar 11, 2026 at 4:39 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> +       sbridge_dev->n_devs = table->n_devs_per_imc;
>
> Do you need this? I thought that kzalloc_flex() filled in the __counted_by field of the structure for you.
kzalloc_flex is just a macro over kzalloc(struct_size()). It does not
do automatic __counted_by, which makes no sense.
> -Tony

RE: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Luck, Tony]

> > +       sbridge_dev->n_devs = table->n_devs_per_imc;
> >
> > Do you need this? I thought that kzalloc_flex() filled in the __counted_by field of the structure for you.
> kzalloc_flex is just a macro over kzalloc(struct_size()). It does not
> do automatic __counted_by, which makes no sense.

It is a fancy macro. But may be complier version dependent whether the count is filled in.

See include/linux/slab.h where kzalloc_flex() uses __alloc_flex() which looks like this:


#define __alloc_flex(KMALLOC, GFP, TYPE, FAM, COUNT)                    \
({                                                                      \
        const size_t __count = (COUNT);                                 \
        const size_t __obj_size = struct_size_t(TYPE, FAM, __count);    \
        TYPE *__obj_ptr = KMALLOC(__obj_size, GFP);                     \
        if (__obj_ptr)                                                  \
                __set_flex_counter(__obj_ptr->FAM, __count);            \
        __obj_ptr;                                                      \
})


See the __set_flex_counter() ... looks like it is trying to set the counted_by field.

-Tony

Re: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Rosen Penev]

On Thu, Mar 12, 2026 at 10:09 AM Luck, Tony <tony.luck@intel.com> wrote:
>
> > > +       sbridge_dev->n_devs = table->n_devs_per_imc;
> > >
> > > Do you need this? I thought that kzalloc_flex() filled in the __counted_by field of the structure for you.
> > kzalloc_flex is just a macro over kzalloc(struct_size()). It does not
> > do automatic __counted_by, which makes no sense.
>
> It is a fancy macro. But may be complier version dependent whether the count is filled in.
>
> See include/linux/slab.h where kzalloc_flex() uses __alloc_flex() which looks like this:
>
>
> #define __alloc_flex(KMALLOC, GFP, TYPE, FAM, COUNT)                    \
> ({                                                                      \
>         const size_t __count = (COUNT);                                 \
>         const size_t __obj_size = struct_size_t(TYPE, FAM, __count);    \
>         TYPE *__obj_ptr = KMALLOC(__obj_size, GFP);                     \
>         if (__obj_ptr)                                                  \
>                 __set_flex_counter(__obj_ptr->FAM, __count);            \
>         __obj_ptr;                                                      \
> })
>
>
> See the __set_flex_counter() ... looks like it is trying to set the counted_by field.
/**
 * __set_flex_counter() - Set the counter associated with the given flexible
 *                        array member that has been annoated by __counted_by().
 * @FAM: Instance of flexible array member within a given struct.
 * @COUNT: Value to store to the __counted_by annotated @FAM_PTR's counter.
 *
 * This is a no-op if no annotation exists. Count needs to be checked with
 * overflows_flex_counter_type() before using this function.
 */
>
> -Tony
>
>

RE: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Zhuo, Qiuxu]

> From: Luck, Tony <tony.luck@intel.com>
> [...]
> Subject: RE: [PATCH] EDAC: sb_edac: use kzalloc_flex
> 
> > > +       sbridge_dev->n_devs = table->n_devs_per_imc;
> > >
> > > Do you need this? I thought that kzalloc_flex() filled in the __counted_by
> field of the structure for you.
> > kzalloc_flex is just a macro over kzalloc(struct_size()). It does not
> > do automatic __counted_by, which makes no sense.
> 
> It is a fancy macro. But may be complier version dependent whether the
> count is filled in.

Yes, " __builtin_counted_by_ref" needs gcc >= 15. See comments [1].
My gcc is v13.3 on Ubuntu 24.04, and I verified that "sbridge_dev->n_devs" was NOT filled in automatically [2].

For safety, we still need to fill in the count field explicitly. 

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler_types.h#n550

[2] --- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -778,7 +778,8 @@ static struct sbridge_dev *alloc_sbridge_dev(int seg, u8 bus, enum domain dom,
        sbridge_dev->seg = seg;
        sbridge_dev->bus = bus;
        sbridge_dev->dom = dom;
-       sbridge_dev->n_devs = table->n_devs_per_imc;
+       //sbridge_dev->n_devs = table->n_devs_per_imc;
+       pr_info("sbridge_dev->n_devs = %d, table->n_devs_per_imc = %d\n", sbridge_dev->n_devs, table->n_devs_per_imc);

Testing result log:

[ 1640.072460] EDAC DEBUG: sbridge_init:
[ 1640.072474] EDAC sbridge: Seeking for: PCI ID 8086:3ca0
[ 1640.072505] sbridge_dev->n_devs = 0, table->n_devs_per_imc = 11     <------------------
[ 1640.072511] EDAC DEBUG: sbridge_get_onedevice: Detected 8086:3ca0
[ 1640.072515] EDAC sbridge: Seeking for: PCI ID 8086:3ca0
[ 1640.072530] sbridge_dev->n_devs = 0, table->n_devs_per_imc = 11     <------------------
...
Failed to load the sb_edac driver.

RE: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Zhuo, Qiuxu]

> From: Rosen Penev <rosenp@gmail.com>
> [...]
> Subject: [PATCH] EDAC: sb_edac: use kzalloc_flex

Could you use this style of subject:

  EDAC/sb: Use kzalloc_flex()

> 
> Simplifies allocations by using a flexible array member in this struct.
> 
> Add __counted_by to get extra runtime analysis.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/edac/sb_edac.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index
> 09d4e816404b..7b282dfd093f 100644
> --- a/drivers/edac/sb_edac.c
> +++ b/drivers/edac/sb_edac.c
> @@ -364,11 +364,11 @@ struct sbridge_dev {
>  	int			seg;
>  	u8			bus, mc;
>  	u8			node_id, source_id;
> -	struct pci_dev		**pdev;
>  	enum domain		dom;
>  	int			n_devs;
>  	int			i_devs;
>  	struct mem_ctl_info	*mci;
> +	struct pci_dev		*pdev[] __counted_by(n_devs);
>  };
> 
>  struct knl_pvt {
> @@ -771,21 +771,14 @@ static struct sbridge_dev *alloc_sbridge_dev(int
> seg, u8 bus, enum domain dom,  {
>  	struct sbridge_dev *sbridge_dev;
> 
> -	sbridge_dev = kzalloc_obj(*sbridge_dev);
> +	sbridge_dev = kzalloc_flex(*sbridge_dev, pdev, table-
> >n_devs_per_imc);
>  	if (!sbridge_dev)
>  		return NULL;
> 
> -	sbridge_dev->pdev = kzalloc_objs(*sbridge_dev->pdev,
> -					 table->n_devs_per_imc);
> -	if (!sbridge_dev->pdev) {
> -		kfree(sbridge_dev);
> -		return NULL;
> -	}
> -
> +	sbridge_dev->n_devs = table->n_devs_per_imc;

What's the reason for moving this line of code up here?

>  	sbridge_dev->seg = seg;
>  	sbridge_dev->bus = bus;
>  	sbridge_dev->dom = dom;
> -	sbridge_dev->n_devs = table->n_devs_per_imc;
>  	list_add_tail(&sbridge_dev->list, &sbridge_edac_list);
> [...]

Other than the comments above:

   Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Re: [PATCH] EDAC: sb_edac: use kzalloc_flex

[Rosen Penev]

On Wed, Mar 11, 2026 at 7:28 PM Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
>
>
> > From: Rosen Penev <rosenp@gmail.com>
> > [...]
> > Subject: [PATCH] EDAC: sb_edac: use kzalloc_flex
>
> Could you use this style of subject:
>
>   EDAC/sb: Use kzalloc_flex()
Will do.
>
> >
> > Simplifies allocations by using a flexible array member in this struct.
> >
> > Add __counted_by to get extra runtime analysis.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  drivers/edac/sb_edac.c | 14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c index
> > 09d4e816404b..7b282dfd093f 100644
> > --- a/drivers/edac/sb_edac.c
> > +++ b/drivers/edac/sb_edac.c
> > @@ -364,11 +364,11 @@ struct sbridge_dev {
> >       int                     seg;
> >       u8                      bus, mc;
> >       u8                      node_id, source_id;
> > -     struct pci_dev          **pdev;
> >       enum domain             dom;
> >       int                     n_devs;
> >       int                     i_devs;
> >       struct mem_ctl_info     *mci;
> > +     struct pci_dev          *pdev[] __counted_by(n_devs);
> >  };
> >
> >  struct knl_pvt {
> > @@ -771,21 +771,14 @@ static struct sbridge_dev *alloc_sbridge_dev(int
> > seg, u8 bus, enum domain dom,  {
> >       struct sbridge_dev *sbridge_dev;
> >
> > -     sbridge_dev = kzalloc_obj(*sbridge_dev);
> > +     sbridge_dev = kzalloc_flex(*sbridge_dev, pdev, table-
> > >n_devs_per_imc);
> >       if (!sbridge_dev)
> >               return NULL;
> >
> > -     sbridge_dev->pdev = kzalloc_objs(*sbridge_dev->pdev,
> > -                                      table->n_devs_per_imc);
> > -     if (!sbridge_dev->pdev) {
> > -             kfree(sbridge_dev);
> > -             return NULL;
> > -     }
> > -
> > +     sbridge_dev->n_devs = table->n_devs_per_imc;
>
> What's the reason for moving this line of code up here?
__counted_by requires setting the counting variable first.
>
> >       sbridge_dev->seg = seg;
> >       sbridge_dev->bus = bus;
> >       sbridge_dev->dom = dom;
> > -     sbridge_dev->n_devs = table->n_devs_per_imc;
> >       list_add_tail(&sbridge_dev->list, &sbridge_edac_list);
> > [...]
>
> Other than the comments above:
>
>    Reviewed-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>