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 D86BAC3DA49 for ; Thu, 18 Jul 2024 06:19:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0B2736B0082; Thu, 18 Jul 2024 02:19:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 062A66B0083; Thu, 18 Jul 2024 02:19:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E46386B0085; Thu, 18 Jul 2024 02:19:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id C0A4F6B0082 for ; Thu, 18 Jul 2024 02:19:37 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 49FE914188A for ; Thu, 18 Jul 2024 06:19:37 +0000 (UTC) X-FDA: 82351871994.28.900F4E9 Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf08.hostedemail.com (Postfix) with ESMTP id EBB2A160007 for ; Thu, 18 Jul 2024 06:19:33 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=r4nkZpUL; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf08.hostedemail.com: domain of mchehab+huawei@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=mchehab+huawei@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721283544; a=rsa-sha256; cv=none; b=Tu7GhLbUvpyyq2QPgfFbHbh7Xq2um7pjOoroLFEm6NwFcBB9U8LLcbtHV+YjAEe11vArUD 3ZWy7xb6xw6CpK7I7N5bYWSSdSIp8XxAs1OLjiWIF7Ay/VP41V5wmKgSfEIr/9vlOUfRef E8MV313V3257ML72cE4Qa2cVFC/HKyM= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=r4nkZpUL; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf08.hostedemail.com: domain of mchehab+huawei@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=mchehab+huawei@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1721283544; 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=+IfOS/ICnvXU5sfRD/baWnQOxA0McC6mYOkomSUaBkw=; b=stz/Yx4CUKhYGYLgsz4HyijPpHa22yq0KqUCCKTqtrLay7JTGxcCxOPKoUX4nCf58BGH5X du738ISzzLeDa1ysLzTpDRxZMZuCrIhEuxD1Pz8pfl/puYcKD1qvr39ik66RbBjC423nR/ 0D7gnmkbOVmMrfJybRrYRk0LuF39R9c= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id F3525CE12CB; Thu, 18 Jul 2024 06:19:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A2DC1C116B1; Thu, 18 Jul 2024 06:19:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721283568; bh=4Clifxq7cRdC7gqNNrtV/aDjuvKrcihIvqZOJQVZ0e8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=r4nkZpULnd0Yq27Yj+TqpJXL7SxXakKd3FwPHLr7Mvjqm8N0/c15uzAL6f1S3VjsF xVzfximTLc1KJzGEMgKitrknuk5Bhmav7qGcMALylhiHhxsxqfjycmfFbfHRhMbmLu 0pppvCMDjDTf7BSqn9ZEaRXTaZTcPyUyc20AQMMcf5liWz6SXX014hSfePRnswyxza qWFc/D/Xkd0bKZAf1uMq3ezY+3dqlhjmaub8DX+SAFSRKzsTm7S3IJqdouuIWflYIH JYKZ8GLL4i9cV58IqS+8+DfmWAYZAfyDlKROHRWcRMBRU6jwv3nG+HbQKlV0c01xKW z1W1XMjeMA2Vw== Date: Thu, 18 Jul 2024 08:19:16 +0200 From: Mauro Carvalho Chehab To: Shiju Jose Cc: "linux-edac@vger.kernel.org" , "linux-cxl@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , "bp@alien8.de" , "tony.luck@intel.com" , "rafael@kernel.org" , "lenb@kernel.org" , "mchehab@kernel.org" , "dan.j.williams@intel.com" , "dave@stgolabs.net" , "Jonathan Cameron" , "dave.jiang@intel.com" , "alison.schofield@intel.com" , "vishal.l.verma@intel.com" , "ira.weiny@intel.com" , "david@redhat.com" , "Vilas.Sridharan@amd.com" , "leo.duran@amd.com" , "Yazen.Ghannam@amd.com" , "rientjes@google.com" , "jiaqiyan@google.com" , "Jon.Grimm@amd.com" , "dave.hansen@linux.intel.com" , "naoya.horiguchi@nec.com" , "james.morse@arm.com" , "jthoughton@google.com" , "somasundaram.a@hpe.com" , "erdemaktas@google.com" , "pgonda@google.com" , "duenwen@google.com" , "mike.malvestuto@intel.com" , "gthelen@google.com" , "wschwartz@amperecomputing.com" , "dferguson@amperecomputing.com" , "wbs@os.amperecomputing.com" , "nifan.cxl@gmail.com" , tanxiaofei , "Zengtao (B)" , "Roberto Sassu" , "kangkang.shen@futurewei.com" , wanghuiqiang , Linuxarm Subject: Re: [RFC PATCH v9 01/11] EDAC: Add generic EDAC RAS feature driver Message-ID: <20240718081916.3c9afb65@foz.lan> In-Reply-To: <2cb0dde458bd4eb79b0a96cb99fe1ef5@huawei.com> References: <20240716150336.2042-1-shiju.jose@huawei.com> <20240716150336.2042-2-shiju.jose@huawei.com> <20240717120027.7168536a@foz.lan> <2cb0dde458bd4eb79b0a96cb99fe1ef5@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-Rspamd-Queue-Id: EBB2A160007 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: x5k76ekqts6zr8d5b9tbzicgd31ydis4 X-HE-Tag: 1721283573-237864 X-HE-Meta: U2FsdGVkX1/CNTmlWjYuvRz7nQp8/RN7ybk31s90/h/fB9sQbx8yWecI/9g7b46gytoEWSyI5wg+0e0UDO9IMOselFRBO5hxzvhS2UDv8fXie9TYW0mk2XH2jdt5ujBKuJ/0AneC5h8/aHotYbNYGJ6hOyfB84uw7mXtB/UecJocUiPhVqaMZSEDaqYK5CVF38cSUjKgzlTXMcGNoDb0sqVRHbeP8RBaR2oRHiqt42t20W6UftZ35DcPna54NvxvNcJkfoMt5M67dZUlsp6bBxiztT4jIsmWzHDZRx9T7aB92cfyRE3A+Az7GxnwkBOM/DlqrygiT5gbiZNERIsyqQtGoHYNZ0j74Q8um6Kcl9WE9V1Qy/nc0VlPRxqkibcL+IMsb9NhnydqXNjhReFDgHpU5KWbQseHzw4Ojl7TdBU2p4Rjg1ep/v/cra6YjFR1xFMtkfssZb0SDGwuXrN5I8IlCV87RarAevijJuQJeVf4nWy90fOabMTA6pvIv7S2vbT9RGrVFS2bnSkpRQaCh/jMDE7VTBp6fsLaWDGxO3/1tSJP+0Id6rJtKqo7JBZQvtxC84veeFQh1D+eYgBW6ZtTy9nl7GxP59MzxVjv2aCaZ9dqq2Rh104hUkT/gAqobiifAIoSGCDyIaHU2gP3r14B1TlL2AM4tFf6ysG4oVpWyITqIFOYq1wEdfXrZxc86e8YX/F9R0TVpQHduGtkuw96IDUVz+9ku74DU7lVY/CZCrAwLztOfikwCUZslkq+gRDCzYH6ZcSG/ahb2LBG4polC39HjJ0qSEzX8LBGOlFD24VataPmdXGadCbi4TVP4gADIt3S3eEmF2RFt/tDb0Ei7D0eZb/c1dFJMbNLFa5Ex4ivihj9SGEF7MJ37XLDN8rjCUrJ0X9hh3Hp9SAke1zGmLL7/HVlELA+I69fQvWNbIuC6U/RUcnGGbQ4kjGNU9hprBzMsTzGP2BqdMe arFwHN3l lBkRbx1j+kAvZCrhmVyIpos1GTqUanawWqRRPdxSLo+sE3yuXR6NOziHoAySaNSn/MjgEvVeY5miDvEPdNFhsf5vgpF8uQgssOOvJgjbi/oyMv0CJDbFegrSB2lkxbyoO+ecM8coDjMPYICn7stYNVEVjlkul7Ez0ta2ECfdQCJD9mO4ujWgr4bR88tdfs1gYo34Nyapw0jTlOKxHIM+6oir3ygLmIua6EhgZ1t2cTwQKCWSiKRjC2dNIYeVl+bohkJARUqZPvQefH/aetRISuZrM/8o/Xp1KEnXrEH7qFxRYKmfI0uRRNQZi47Yd8EAvtx4m+jibOxoTH4Iy6gZxHnVqDczrOSCYEkrOgzlgmlTFVqVxQ5edQCN6iUL9NBfNGrSN6miIlCmz6AS0JQrhAUDad+SKUisAdXYBnXfo1AC3iy7HQvNyg9tiumSVkp3PnCFpnCw6Vr+c4WuDt6rKoq0EAM4wpRjcDR8QbFJLTpEjCtnASasIVCV337LbOhQwlhHJj8ygYoqMgJFUx0DrXTiyqaOIK0lQj3Q9KGYQ2k2kXom/3thsylWpOuQYyggMHsXowf8v9JwLPwmSZ8W22pXJtiTTn1uFfwMk29IN0K+7/tAGtCDePDhJ8jYK10hCmeIW6UbHuycrYuumZC8RH1MrAEIIGpihe/avmm8Jw1gzhGPJWZEyTdKgDTKWpieJW3/OhS/kVRC8zgiQTRhPZi676QPUKt/Y6bQ1t55KHOkB2YHUAt2mG2bVumSu8K9JTbOrNhkDnTSf01ImiE/L+iJrnDJEZmZmy85glHbS+ycfXS0ueUt6leL3itfHxMMaOMI7SAlvVtrSKt4IVTO5Ux1yhG86nD2ZuYvowxC+pU6lLBYcqEiHUn3lYFGuoJPBvhttMCRgyU85YbZACDJgfRcPsc4vMwBNslQJVSGFPyBlWIf52EVrCLQX+g== 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 Wed, 17 Jul 2024 11:01:58 +0000 Shiju Jose escreveu: > Hi Mauro, > > Thanks for the feedbacks. > > >-----Original Message----- > >From: Mauro Carvalho Chehab > >Sent: 17 July 2024 11:00 > >To: Shiju Jose > >Cc: linux-edac@vger.kernel.org; linux-cxl@vger.kernel.org; linux- > >acpi@vger.kernel.org; linux-mm@kvack.org; linux-kernel@vger.kernel.org; > >bp@alien8.de; tony.luck@intel.com; rafael@kernel.org; lenb@kernel.org; > >mchehab@kernel.org; dan.j.williams@intel.com; dave@stgolabs.net; Jonathan > >Cameron ; dave.jiang@intel.com; > >alison.schofield@intel.com; vishal.l.verma@intel.com; ira.weiny@intel.com; > >david@redhat.com; Vilas.Sridharan@amd.com; leo.duran@amd.com; > >Yazen.Ghannam@amd.com; rientjes@google.com; jiaqiyan@google.com; > >Jon.Grimm@amd.com; dave.hansen@linux.intel.com; > >naoya.horiguchi@nec.com; james.morse@arm.com; jthoughton@google.com; > >somasundaram.a@hpe.com; erdemaktas@google.com; pgonda@google.com; > >duenwen@google.com; mike.malvestuto@intel.com; gthelen@google.com; > >wschwartz@amperecomputing.com; dferguson@amperecomputing.com; > >wbs@os.amperecomputing.com; nifan.cxl@gmail.com; tanxiaofei > >; Zengtao (B) ; Roberto > >Sassu ; kangkang.shen@futurewei.com; > >wanghuiqiang ; Linuxarm > > > >Subject: Re: [RFC PATCH v9 01/11] EDAC: Add generic EDAC RAS feature driver > > > >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. > Will do. Previously it was "EDAC RAS FEAT" > > > > >> + > >> +#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. > 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 > >> +#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. > 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