From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3415C3DA4B for ; Wed, 17 Jul 2024 10:00:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 932756B009B; Wed, 17 Jul 2024 06:00:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8E24A6B009C; Wed, 17 Jul 2024 06:00:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7AA886B009D; Wed, 17 Jul 2024 06:00:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 5DB326B009B for ; Wed, 17 Jul 2024 06:00:49 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id D2EF0A4D6D for ; Wed, 17 Jul 2024 10:00:48 +0000 (UTC) X-FDA: 82348800576.08.A6784D0 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf24.hostedemail.com (Postfix) with ESMTP id 36E4C180042 for ; Wed, 17 Jul 2024 10:00:44 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=oIITv2Oz; spf=pass (imf24.hostedemail.com: domain of mchehab+huawei@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=mchehab+huawei@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721210406; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=cQ3PnIu2XD5RV0qxs8N61a1Yv8sur8aLTgb2e8qpBsY=; b=RLCeO8XSFdJ5lR/khxi1a27il7ZQOsGfXdaT1j2w7sn7QRlfJn0+XBDFubOwCTht3Ios8G he9rkF3syEjyIXxWZDVHTc0Oznlm8tZPJRlsn+5ugPWi3uKbIG/lNzsPg4jAuKUUJc4MKZ +AwCx3xKRWZQlhMXbhRPqGjQp+f2NQY= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721210406; a=rsa-sha256; cv=none; b=E9WtwRz4fkqeI00jTa53tUGHpwndP9vAUbhXQstOIm9Im9Nz1gHJOgWS0TfVNn1pcIf7EA fHf4UVo/BcC37VuUCsKOMNFQJOFKSBcbe/pXNJmXFeozsj0m63imCX+lcQUY43yWqkUX+f XbTuSwPJ8JhDcPJT4bFZDemqnOaJv9E= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=oIITv2Oz; spf=pass (imf24.hostedemail.com: domain of mchehab+huawei@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=mchehab+huawei@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 15EBCCE1715; Wed, 17 Jul 2024 10:00:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8B67CC4AF09; Wed, 17 Jul 2024 10:00:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721210439; bh=Y8fvJSiFbbffJ1tqg7DrvVvGrSXcWezFpGdYqavSyYE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=oIITv2Oz4LaYbx37sPrRhnS5Zy7L+dyPBRU/6MTbfM2Fn+OCv3YDkZ9S10+DWZnlM 0F4oIBxHi8wV6UOO0z+ZkKYdR2D5jvPyMhTRhzNrP3nqKMWENFGmm8ElKW2ZxgDZ6q vGuKfq/UCe4quA1e9K+R3suPSMDPSOGnXTCtWu6g3rlVLKwkZfK5n6VZ5GbWcdEurl 4GjxlObJgMZ0hagz+HhZ7Hb+6Lxf7uvvDC9Rzwayl+ufzQsEb9CBk6F1FDE/kyWvKs 91lpAII81wyYMAMOQJpjdjrcFX6R1PIegnV7PaLOHwe2Iogcd+OjVX2ZgXzxLni7gq YRFJyMt1Bx7+w== Date: Wed, 17 Jul 2024 12:00:27 +0200 From: Mauro Carvalho Chehab To: Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [RFC PATCH v9 01/11] EDAC: Add generic EDAC RAS feature driver Message-ID: <20240717120027.7168536a@foz.lan> In-Reply-To: <20240716150336.2042-2-shiju.jose@huawei.com> References: <20240716150336.2042-1-shiju.jose@huawei.com> <20240716150336.2042-2-shiju.jose@huawei.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 36E4C180042 X-Stat-Signature: z8zdgt7akaxpyetrzz6acoyx16wyjuuk X-HE-Tag: 1721210444-440018 X-HE-Meta: U2FsdGVkX18xSbGElW1+fGAUNV3jjpCLa4Q1FhktGnICVk9J/2cmWL8DoGCwUdjHmrKX1bHpoJwN/yihlo85+IBP8hdMX0OH2l0Bnde/p5wbNtQYO/MCuxfpruJWy1cJY65+kUwuZyRUMw45lP5DeuKQyZ2R6K5Z0YsalXldm2L7GFpW5VC+Mx/bIDhcsFYGveM2lnH7wSFM1R9eMhYHZr2JYGWenUTT4kz5YuPkcFZuj1MPo24FGxWppWaABVCvwWMuK+KYOO041bs823BjIzBgNZG16EkVaqx6YcoxCb83qv0p7sZc+vOJyBeq14LKedDj9t0sad49i0CFqIO9ypVVwjJphTmha1s7byl6kOspd7CMX5iCoH1mt2XKpLXUWsM5cCyLBne10AFzisG2KHIfW42gp6EdlAOQJbcvSHi8IPGojJmTQYvpfMO8JnO7KGfikzeulTvbU4jAXHjt8W4qXhaGc1oujFF4uG7K4t0lHH4C40Z3kqbyQlLu52kFJW7zYpDxHIQNh/xQIrbl3wdWr8g3mPewzqhFZtscqWsnloCnictCLb69VBcnw5jmUNEikpO8dP81Yhef00QRmmDJ5DBBqwReNwHuravbBStL1GPporPfrjU6HzwuDp+sVjal0nGzc52vhK9LDhjxAuQW5u0ZxeFGbbzgRIIlRT6f/mUUVeoun2ZpbuJjpD/B9xHm5gXGghXg0ixWr4lt9uxkH+TvTDRtI4/JCRhD9aLnobj8ogkj0uGJLXef7iCslnpFrVTKxz7+59xjGwvr0UcUX6bgfgAYKBJyYS5CRQS/4ioX1taXI30czBiKi/k7c/0le1Z6FDVMg4vh+nTLZPH7kiWyCQX+iBjgRsTeVna2i+zxMQyBgE8y0e2jeFlIYSW/Xr1/FczegZ3okj46GSm6DXLmYLMS4hxXl3QiddoDRZ4yC3TV55Uua3vDzcpXMtj5KNJUgsupmu4+LuZ TzpklUxD 7/rG1WlPqcQ/Uf3ClfwHrKmF4p3pVN/9H+oiPP5KtsObT6jtIfBXQ4Ow9loFcnnMaIi1iaEsYx+C0z3CPftE449E+izoocykYuhS8D3ggs/7IUMXZ5mlwO2PaBJYDBIY2CrCq7VMpQdwniBGBdWoLPJiUHN5DqhBL3f8wURt43bSadxyjw50EDYMvz/v9wMtdKIcqwnQ6+B5NlQCA3eznDlHUxkmIeq7XQbDczUXMkaEBypi8uygYoRdqCVImo1JAjPNewYUHm7QEjSDIdr6XBxcoLQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: Em Tue, 16 Jul 2024 16:03:25 +0100 escreveu: > From: Shiju Jose > > 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/// > > Co-developed-by: Jonathan Cameron > Signed-off-by: Jonathan Cameron > Signed-off-by: Shiju Jose > --- > 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 > + > +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 > +#include > + > +#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