Em Wed, 17 Jul 2024 11:01:58 +0000
Shiju Jose <shiju.jose@xxxxxxxxxx> escreveu:
> Hi Mauro,
>
> Thanks for the feedbacks.
>
> >-----Original Message-----
> >From: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> >Sent: 17 July 2024 11:00
> >To: Shiju Jose <shiju.jose@xxxxxxxxxx>
> >Cc: linux-edac@xxxxxxxxxxxxxxx; linux-cxl@xxxxxxxxxxxxxxx; linux-
> >acpi@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >bp@xxxxxxxxx; tony.luck@xxxxxxxxx; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
> >mchehab@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; dave@xxxxxxxxxxxx; Jonathan
> >Cameron <jonathan.cameron@xxxxxxxxxx>; dave.jiang@xxxxxxxxx;
> >alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx; ira.weiny@xxxxxxxxx;
> >david@xxxxxxxxxx; Vilas.Sridharan@xxxxxxx; leo.duran@xxxxxxx;
> >Yazen.Ghannam@xxxxxxx; rientjes@xxxxxxxxxx; jiaqiyan@xxxxxxxxxx;
> >Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
> >naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx;
> >somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx;
> >duenwen@xxxxxxxxxx; mike.malvestuto@xxxxxxxxx; gthelen@xxxxxxxxxx;
> >wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx;
> >wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; tanxiaofei
> ><tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; Roberto
> >Sassu <roberto.sassu@xxxxxxxxxx>; kangkang.shen@xxxxxxxxxxxxx;
> >wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; Linuxarm
> ><linuxarm@xxxxxxxxxx>
> >Subject: Re: [RFC PATCH v9 01/11] EDAC: Add generic EDAC RAS feature driver
> >
> >Em Tue, 16 Jul 2024 16:03:25 +0100
> ><shiju.jose@xxxxxxxxxx> escreveu:
> >
> >> From: Shiju Jose <shiju.jose@xxxxxxxxxx>
> >>
> >> Add generic EDAC driver supports registering RAS features supported in
> >> the system. The driver exposes feature's control attributes to the
> >> 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>
> >> ---
> >> drivers/edac/Makefile | 1 +
> >> drivers/edac/edac_ras_feature.c | 155
> >> +++++++++++++++++++++++++++++++ include/linux/edac_ras_feature.h |
> >> 66 +++++++++++++
> >> 3 files changed, 222 insertions(+)
> >> create mode 100755 drivers/edac/edac_ras_feature.c create mode
> >> 100755 include/linux/edac_ras_feature.h
> >>
> >> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index
> >> 9c09893695b7..c532b57a6d8a 100644
> >> --- a/drivers/edac/Makefile
> >> +++ b/drivers/edac/Makefile
> >> @@ -10,6 +10,7 @@ obj-$(CONFIG_EDAC) := edac_core.o
> >>
> >> edac_core-y := edac_mc.o edac_device.o edac_mc_sysfs.o
> >> edac_core-y += edac_module.o edac_device_sysfs.o wq.o
> >> +edac_core-y += edac_ras_feature.o
> >>
> >> edac_core-$(CONFIG_EDAC_DEBUG) += debugfs.o
> >>
> >> diff --git a/drivers/edac/edac_ras_feature.c
> >> b/drivers/edac/edac_ras_feature.c new file mode 100755 index
> >> 000000000000..24a729fea66f
> >> --- /dev/null
> >> +++ b/drivers/edac/edac_ras_feature.c
> >> @@ -0,0 +1,155 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * EDAC RAS control feature driver supports registering RAS
> >> + * features with the EDAC and exposes the feature's control
> >> + * attributes to the userspace in sysfs.
> >> + *
> >> + * Copyright (c) 2024 HiSilicon Limited.
> >> + */
> >> +
> >
> >> +#define pr_fmt(fmt) "EDAC RAS CONTROL FEAT: " fmt
> >
> >Sounds a too long prefix for my taste.
> Will do. Previously it was "EDAC RAS FEAT"
>
> >
> >> +
> >> +#include <linux/edac_ras_feature.h>
> >> +
> >> +static void edac_ras_dev_release(struct device *dev) {
> >> + struct edac_ras_feat_ctx *ctx =
> >> + container_of(dev, struct edac_ras_feat_ctx, dev);
> >> +
> >> + kfree(ctx);
> >> +}
> >> +
> >> +const struct device_type edac_ras_dev_type = {
> >> + .name = "edac_ras_dev",
> >> + .release = edac_ras_dev_release,
> >> +};
> >> +
> >> +static void edac_ras_dev_unreg(void *data) {
> >> + device_unregister(data);
> >> +}
> >> +
> >> +static int edac_ras_feat_scrub_init(struct device *parent,
> >> + struct edac_scrub_data *sdata,
> >> + const struct edac_ras_feature *sfeat,
> >> + const struct attribute_group **attr_groups) {
> >> + sdata->ops = sfeat->scrub_ops;
> >> + sdata->private = sfeat->scrub_ctx;
> >> +
> >> + return 1;
> >> +}
> >> +
> >> +static int edac_ras_feat_ecs_init(struct device *parent,
> >> + struct edac_ecs_data *edata,
> >> + const struct edac_ras_feature *efeat,
> >> + const struct attribute_group **attr_groups) {
> >> + int num = efeat->ecs_info.num_media_frus;
> >> +
> >> + edata->ops = efeat->ecs_ops;
> >> + edata->private = efeat->ecs_ctx;
> >> +
> >> + return num;
> >> +}
> >
> >I would place this function earlier and/or add some documentation for the above
> >two functions.
> Will do. I guess you want place these functions above edac_ras_dev_release() right?
I mean placing edac_ras_feat_ecs_ini() before edac_ras_feat_scrub_init(),
as it helps reviewers to understand that the return code is the number
of attr groups. Another option would be to document the arguments and
the return value for such functions.
> >
> >I got confused when reviewed the first function and saw there an
> >unconditional:
> The call for the feature specific init functions are added here in the next feature specific patches
> of this series.
> >
> > return 1;
> >
> >Now, I guess the goal is to return the number of initialized features, right?
> Return the number of attr groups added for a feature as the instances for a feature is dynamic,
> for e.g. the number of FRUs in ECS feature.
>
> >
> >> +
> >> +/**
> >> + * edac_ras_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.
> >> + *
> >> + * Returns 0 on success, error otherwise.
> >> + * The new edac_ras_feat_ctx would be freed automatically.
> >> + */
> >> +int edac_ras_dev_register(struct device *parent, char *name,
> >> + void *private, int num_features,
> >> + const struct edac_ras_feature *ras_features) {
> >> + const struct attribute_group **ras_attr_groups;
> >> + struct edac_ras_feat_ctx *ctx;
> >> + int attr_gcnt = 0;
> >> + int ret, feat;
> >> +
> >> + if (!parent || !name || !num_features || !ras_features)
> >> + return -EINVAL;
> >> +
> >> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> >> + if (!ctx)
> >> + return -ENOMEM;
> >> +
> >> + ctx->dev.parent = parent;
> >> + ctx->private = private;
> >> +
> >> + /* Double parse so we can make space for attributes */
> >> + for (feat = 0; feat < num_features; feat++) {
> >> + switch (ras_features[feat].feat) {
> >> + case ras_feat_scrub:
> >> + attr_gcnt++;
> >> + break;
> >> + case ras_feat_ecs:
> >> + attr_gcnt +=
> >ras_features[feat].ecs_info.num_media_frus;
> >> + break;
> >
> >As already suggested, the enum names shall be in uppercase.
> >Having a lowercase one here looks really weird.
> Agree.
> >
> >> + default:
> >> + ret = -EINVAL;
> >> + goto ctx_free;
> >> + }
> >> + }
> >
> >I would place this logic earlier, before allocating ctx, as, in case of errors, the
> >function can just call "return -EINVAL".
> Ok.
>
> >
> >> +
> >> + ras_attr_groups = devm_kzalloc(parent,
> >> + (attr_gcnt + 1) * sizeof(*ras_attr_groups),
> >> + GFP_KERNEL);
> >
> >Hmm... why are you using devm variant here, and non-devm one for cxt?
> >
> >My personal preference is to avoid devm variants, as memory is only freed
> >when the device refcount becomes zero (which, depending on the driver, may
> >never happen in practice, as driver core may keep a refcount, depending on how
> >the device was probed).
> Can use Kzalloc and need to add free for ras_attr_groups on error etc.
Ok. While here, please also use the kcalloc/kmalloc_array variants, as
doing num * sizeof(foo) should be avoided.
Btw, there are some checks inside checkpatch meant to identify it like
ALLOC_WITH_MULTIPLY. Not sure why it didn't trigger it here.
Hint: while the number of false positive hits increase, I'm always running
checkpatch with --strict, as it detects some additional potential problems.
> >> + if (!ras_attr_groups) {
> >> + ret = -ENOMEM;
> >> + goto ctx_free;
> >> + }
> >> +
> >> + attr_gcnt = 0;
> >> + for (feat = 0; feat < num_features; feat++, ras_features++) {
> >> + if (ras_features->feat == ras_feat_scrub) {
> >
> >I would use a switch here as well, just like the previous feature type check.
> Will do.
> >
> >> + if (!ras_features->scrub_ops)
> >> + continue;
> >> + ret = edac_ras_feat_scrub_init(parent, &ctx->scrub,
> >> + ras_features,
> >&ras_attr_groups[attr_gcnt]);
> >
> >I don't think it is worth having those ancillary functions here...
> >
> >> + if (ret < 0)
> >> + goto ctx_free;
> >> +
> >> + attr_gcnt += ret;
> >> + } else if (ras_features->feat == ras_feat_ecs) {
> >> + if (!ras_features->ecs_ops)
> >> + continue;
> >> + ret = edac_ras_feat_ecs_init(parent, &ctx->ecs,
> >> + ras_features,
> >&ras_attr_groups[attr_gcnt]);
> >
> >and here, as most of the current functions are very simple:
> >
> >both just sets two arguments:
> >
> > edata->ops
> > edata->private
> >
> >and returned vaules are always a positive counter...
> >
> >> + if (ret < 0)
> >> + goto ctx_free;
> >
> >So, this check for instance, doesn't make sense.
> The call for the feature specific init functions are added in the next feature specific patches
> of this series and which could return error.
Ok.
> >
> >> +
> >> + attr_gcnt += ret;
> >> + } else {
> >> + ret = -EINVAL;
> >> + goto ctx_free;
> >> + }
> >> + }
> >> + ras_attr_groups[attr_gcnt] = NULL;
> >> + ctx->dev.bus = edac_get_sysfs_subsys();
> >> + ctx->dev.type = &edac_ras_dev_type;
> >> + ctx->dev.groups = ras_attr_groups;
> >> + dev_set_drvdata(&ctx->dev, ctx);
> >> + ret = dev_set_name(&ctx->dev, name);
> >> + if (ret)
> >> + goto ctx_free;
> >> +
> >> + ret = device_register(&ctx->dev);
> >> + if (ret) {
> >> + put_device(&ctx->dev);
> >> + return ret;
> >> + }
> >> +
> >> + return devm_add_action_or_reset(parent, edac_ras_dev_unreg,
> >> +&ctx->dev);
> >> +
> >> +ctx_free:
> >> + kfree(ctx);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(edac_ras_dev_register);
> >> diff --git a/include/linux/edac_ras_feature.h
> >> b/include/linux/edac_ras_feature.h
> >> new file mode 100755
> >> index 000000000000..000e99141023
> >> --- /dev/null
> >> +++ b/include/linux/edac_ras_feature.h
> >> @@ -0,0 +1,66 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * EDAC RAS control features.
> >> + *
> >> + * Copyright (c) 2024 HiSilicon Limited.
> >> + */
> >> +
> >> +#ifndef __EDAC_RAS_FEAT_H
> >> +#define __EDAC_RAS_FEAT_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/edac.h>
> >> +
> >> +#define EDAC_RAS_NAME_LEN 128
> >> +
> >> +enum edac_ras_feat {
> >> + ras_feat_scrub,
> >> + ras_feat_ecs,
> >> + ras_feat_max
> >> +};
> >
> >Enum values in uppercase, please.
> Will do.
> >
> >> +
> >> +struct edac_ecs_ex_info {
> >> + u16 num_media_frus;
> >> +};
> >> +
> >> +/*
> >> + * EDAC RAS feature information structure */ struct edac_scrub_data
> >> +{
> >> + const struct edac_scrub_ops *ops;
> >> + void *private;
> >> +};
> >> +
> >> +struct edac_ecs_data {
> >> + const struct edac_ecs_ops *ops;
> >> + void *private;
> >> +};
> >> +
> >> +struct device;
> >> +
> >> +struct edac_ras_feat_ctx {
> >> + struct device dev;
> >> + void *private;
> >> + struct edac_scrub_data scrub;
> >> + struct edac_ecs_data ecs;
> >> +};
> >> +
> >> +struct edac_ras_feature {
> >> + enum edac_ras_feat feat;
> >> + union {
> >> + const struct edac_scrub_ops *scrub_ops;
> >> + const struct edac_ecs_ops *ecs_ops;
> >> + };
> >> + union {
> >> + struct edac_ecs_ex_info ecs_info;
> >> + };
> >
> >I would place the variable structs union at the end. This may help with
> >alignments, if you place the pointers earlier.
> Will do.
>
> >
> >> + union {
> >> + void *scrub_ctx;
> >> + void *ecs_ctx;
> >> + };
> >> +};
> >> +
> >> +int edac_ras_dev_register(struct device *parent, char *dev_name,
> >> + void *parent_pvt_data, int num_features,
> >> + const struct edac_ras_feature *ras_features); #endif
> >/*
> >> +__EDAC_RAS_FEAT_H */
> >
> >
> >
> >Thanks,
> >Mauro
> >
>
> Thanks,
> Shiju
Thanks,
Mauro
