drivers/edac/sb_edac.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
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
> 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>
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>
+ 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
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
> > + 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
> 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.
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
>
>
© 2016 - 2026 Red Hat, Inc.