On Wed, 9 Oct 2024 13:41:02 +0100 <shiju.jose@xxxxxxxxxx> wrote: > From: Shiju Jose <shiju.jose@xxxxxxxxxx> > > Add generic EDAC device features control supports registering > RAS features supported in the system. Driver exposes features > control attributes to userspace in > /sys/bus/edac/devices/<dev-name>/<ras-feature>/ > > Co-developed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Signed-off-by: Shiju Jose <shiju.jose@xxxxxxxxxx> Hi Shiju, Spotted a few minor bugs in here that I'd missed in internal review :( See below. Jonathan > --- > drivers/edac/edac_device.c | 105 +++++++++++++++++++++++++++++++++++++ > include/linux/edac.h | 32 +++++++++++ > 2 files changed, 137 insertions(+) > > diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c > index 621dc2a5d034..0b8aa8150239 100644 > --- a/drivers/edac/edac_device.c > +++ b/drivers/edac/edac_device.c > + > +/** > + * edac_dev_register - register device for RAS features with EDAC > + * @parent: client device. > + * @name: client device's name. > + * @private: parent driver's data to store in the context if any. > + * @num_features: number of RAS features to register. > + * @ras_features: list of RAS features to register. > + * > + * Return: > + * * %0 - Success. > + * * %-EINVAL - Invalid parameters passed. > + * * %-ENOMEM - Dynamic memory allocation failed. > + * > + * The new edac_dev_feat_ctx would be freed automatically. > + */ > +int edac_dev_register(struct device *parent, char *name, > + void *private, int num_features, > + const struct edac_dev_feature *ras_features) > +{ ... > + ret = device_register(&ctx->dev); > + if (ret) { > + put_device(&ctx->dev); > + goto groups_free; > + return ret; Unreachable line. However, shouldn't have the goto here as put_device() should result in the release being called in which case this is a double free. So drop the goto and keep the return. > + } > + > + return devm_add_action_or_reset(parent, edac_dev_unreg, &ctx->dev); > + > +groups_free: > + kfree(ras_attr_groups); > +ctx_free: > + kfree(ctx); > + return ret; > +} > +EXPORT_SYMBOL_GPL(edac_dev_register); > diff --git a/include/linux/edac.h b/include/linux/edac.h > index b4ee8961e623..1db008a82690 100644 > --- a/include/linux/edac.h > +++ b/include/linux/edac.h > @@ -661,4 +661,36 @@ static inline struct dimm_info *edac_get_dimm(struct mem_ctl_info *mci, > +/* EDAC device feature information structure */ > +struct edac_dev_data { > + u8 instance; > + void *private; > +}; > + > +struct device; That forwards def doesn't work as this header needs to include enough information to establish layout of struct edac_dev_feat_ctx. Header already includes linux/device.h so just drop this. > + > +struct edac_dev_feat_ctx { > + struct device dev; > + void *private; > +}; > + > +struct edac_dev_feature { > + enum edac_dev_feat ft_type; > + u8 instance; > + void *ctx; > +}; > + > +int edac_dev_register(struct device *parent, char *dev_name, > + void *parent_pvt_data, int num_features, > + const struct edac_dev_feature *ras_features); > #endif /* _LINUX_EDAC_H_ */