Message-ID: <20240717120027.7168536a@foz.lan>
Date: Wed, 17 Jul 2024 12:00:27 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: <shiju.jose@...wei.com>
Cc: <linux-edac@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
 <linux-acpi@...r.kernel.org>, <linux-mm@...ck.org>,
 <linux-kernel@...r.kernel.org>, <bp@...en8.de>, <tony.luck@...el.com>,
 <rafael@...nel.org>, <lenb@...nel.org>, <mchehab@...nel.org>,
 <dan.j.williams@...el.com>, <dave@...olabs.net>,
 <jonathan.cameron@...wei.com>, <dave.jiang@...el.com>,
 <alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
 <ira.weiny@...el.com>, <david@...hat.com>, <Vilas.Sridharan@....com>,
 <leo.duran@....com>, <Yazen.Ghannam@....com>, <rientjes@...gle.com>,
 <jiaqiyan@...gle.com>, <Jon.Grimm@....com>, <dave.hansen@...ux.intel.com>,
 <naoya.horiguchi@....com>, <james.morse@....com>, <jthoughton@...gle.com>,
 <somasundaram.a@....com>, <erdemaktas@...gle.com>, <pgonda@...gle.com>,
 <duenwen@...gle.com>, <mike.malvestuto@...el.com>, <gthelen@...gle.com>,
 <wschwartz@...erecomputing.com>, <dferguson@...erecomputing.com>,
 <wbs@...amperecomputing.com>, <nifan.cxl@...il.com>,
 <tanxiaofei@...wei.com>, <prime.zeng@...ilicon.com>,
 <roberto.sassu@...wei.com>, <kangkang.shen@...urewei.com>,
 <wanghuiqiang@...wei.com>, <linuxarm@...wei.com>
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@...wei.com> escreveu:

> From: Shiju Jose <shiju.jose@...wei.com>
> 
> 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@...wei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
> ---
>  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.

> +
> +#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.

I got confused when reviewed the first function and saw there an
unconditional:

	return 1;

Now, I guess the goal is to return the number of initialized
features, right?

> +
> +/**
> + * 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.

> +		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".

> +
> +	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).

> +	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.

> +			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.

> +
> +			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.

> +
> +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.

> +	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

Powered by blists - more mailing lists