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 705BCC48BC3 for ; Tue, 20 Feb 2024 12:39:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E529F6B0075; Tue, 20 Feb 2024 07:39:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DDBAE6B0078; Tue, 20 Feb 2024 07:39:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C7C376B007B; Tue, 20 Feb 2024 07:39:31 -0500 (EST) 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 B6AD56B0075 for ; Tue, 20 Feb 2024 07:39:31 -0500 (EST) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 8236116066C for ; Tue, 20 Feb 2024 12:39:30 +0000 (UTC) X-FDA: 81812138100.11.782A287 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf22.hostedemail.com (Postfix) with ESMTP id 02AC9C0007 for ; Tue, 20 Feb 2024 12:39:27 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf22.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1708432768; a=rsa-sha256; cv=none; b=HyP76tSlREeQhiNh0yXm+LgBnXl4J6BrNacfdAB2rYNMWwKEBr5UqMQcYzwp9ho4nxGzJ7 4HJXvm7UJ+gEYknEsNaNbZC7eNNAWFrB6dIQgeLdGRAK+aa4OXEdU6FmM63HC2mN4qADaC Eju7AyBW70eUQGuoH5K07SF2C7s6bKA= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf22.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1708432768; 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; bh=1NUE54QJJn0oJJFoQjZVUw5YJZD930Si++My4UC+dy0=; b=6TVc/9U4MrtqTyV/8SAAs0wBIvFyf2RpDBFcixKDirRs/ODR113+p4gbffvdql1mgYJhVN bCvjqNXhRB2NTCli9RftOTZiYpeeHvwJH1FLhrAMJCTH14kOQHZD5tYqSwGySZ+WPXQvst cu6HHLwsbOorLxou/8Z0qRTnJoVdjSg= Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4TfJm54fYvz67l0C; Tue, 20 Feb 2024 20:35:49 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 3F78F14163B; Tue, 20 Feb 2024 20:39:24 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Tue, 20 Feb 2024 12:39:23 +0000 Date: Tue, 20 Feb 2024 12:39:22 +0000 From: Jonathan Cameron To: CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [RFC PATCH v6 06/12] memory: scrub: Add scrub subsystem driver supports configuring memory scrubs in the system Message-ID: <20240220123922.00007142@Huawei.com> In-Reply-To: <20240215111455.1462-7-shiju.jose@huawei.com> References: <20240215111455.1462-1-shiju.jose@huawei.com> <20240215111455.1462-7-shiju.jose@huawei.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml500004.china.huawei.com (7.191.163.9) To lhrpeml500005.china.huawei.com (7.191.163.240) X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 02AC9C0007 X-Stat-Signature: cdkzga9wdj5amsfcofz55np8oshn47ah X-Rspam-User: X-HE-Tag: 1708432767-192185 X-HE-Meta: U2FsdGVkX1+HL2lraRH69kOK0FqiUJ1ZTARCm9v3HMN4YiyMxUyCllKCRNnAFGKYH40rAOcPokZoW3OKWdPfs0gy2QkziiPnf6TsxOQLPnnuXc+cPDdtZ4ouvDZK9jWJYmfZ/nWfcTwXHXlXxibh1tMJT84QrcygvhfulGjOQZ3H+RYblUL82bLIZ2t+49AFLXYI7u6MXXRUTntILNky9c9N/+UXFIyxxC0ea2KgJD1VqW/NbVootbhq3nV+PdcpC2jNPI16HvtSH13nqsLFMXpR1zvFIrg+BIwS1Mm7WedeeIdI7qNuZt7944/OTAzZPfDMOGWLUhyJQ6Fiz69XCJ8F7Gb26CR5dR7aT2YJFcV6wLkTntCx0ivbbgrS/i5EFZZKj6DP5Vp6gmlFAxnOFVuQJY/XyhnZ5PflShIjSJOjOjvqLg+OmF8H9B85OvpmE2M2g0j6pimif5xz6bXLl6TVwnlMeaOaOHqzQcKt54b/pKRffbNZcCOdfiZAzXar0zsdDRLc41i750cqRCGDGLpGM0CDtzKqVisnBaXuMZoAxOeT0MmLxJBO2A0Q3oqFMRODYPocuGibS3VveUDilr0Z8cZTB5Yi1aEg2TyLIr9HAh880ngiz1rKK6UAKYXSVgBafabMMco8PgZQLbatlxFOKbrlkxmixI5RFSOC5dpEd8n2ucCAJ6gJcxumJfVGKBZZ8csN7C+rn4WFWg7A23ESdzX+5TK75MGBqQWhQyM4vFiaYZziLtFG1ZDyt1gTZIDrKIjt0JmJDEwzBlZj/hbTGLIG7Nv1nbcV1GZsnd7huNVYTT8CCC9KI5yO1Y2/70w/hmN76+8aJw7LQe2OqaxlGz5NCiMJI7LhJP3Gri5SoXvsEVNCNEDWwwDf6BgN 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: > From: Shiju Jose > > Add scrub driver supports configuring the memory scrubs in the system. > The scrub driver provides the interface for registering the scrub devices > and supports configuring memory scrubs in the system. > Driver exposes the sysfs scrub control attributes to the user in > /sys/class/scrub/scrubX/regionN/ > > Signed-off-by: Shiju Jose Hi Shiju, A few minor things inline. Given I reviewed this internally I don't have that much to add! Jonathan > --- > .../ABI/testing/sysfs-class-scrub-configure | 91 +++++ > drivers/memory/Kconfig | 1 + > drivers/memory/Makefile | 1 + > drivers/memory/scrub/Kconfig | 11 + > drivers/memory/scrub/Makefile | 6 + > drivers/memory/scrub/memory-scrub.c | 367 ++++++++++++++++++ > include/memory/memory-scrub.h | 78 ++++ > 7 files changed, 555 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-scrub-configure > create mode 100644 drivers/memory/scrub/Kconfig > create mode 100644 drivers/memory/scrub/Makefile > create mode 100755 drivers/memory/scrub/memory-scrub.c > create mode 100755 include/memory/memory-scrub.h > > diff --git a/Documentation/ABI/testing/sysfs-class-scrub-configure b/Documentation/ABI/testing/sysfs-class-scrub-configure > new file mode 100644 > index 000000000000..d2d422b667cf > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-scrub-configure > +What: /sys/class/scrub/scrubX/regionN/rate_available > +Date: January 2024 > +KernelVersion: 6.8 > +Contact: linux-kernel@vger.kernel.org > +Description: > + (RO) Supported range for the scrub rate) > + by the scrubber for a memory region. > + The unit of the scrub rate vary depends on the scrub. Not good to have a unit that is dependent on scrub. We need to figure out how to either define that, or provide an interface to expose it to userspace and make it a userspace tool problem. > diff --git a/drivers/memory/scrub/memory-scrub.c b/drivers/memory/scrub/memory-scrub.c > new file mode 100755 > index 000000000000..a160b7a047e4 > --- /dev/null > +SCRUB_ATTR_RW(addr_base); > +SCRUB_ATTR_RW(addr_size); > +SCRUB_ATTR_RW(enable); > +SCRUB_ATTR_RW(enable_background_scrub); > +SCRUB_ATTR_RW(rate); > +SCRUB_ATTR_RO(rate_available); > + > +static struct attribute *scrub_attrs[] = { > + &dev_attr_addr_base.attr, > + &dev_attr_addr_size.attr, > + &dev_attr_enable.attr, > + &dev_attr_enable_background_scrub.attr, > + &dev_attr_rate.attr, > + &dev_attr_rate_available.attr, > + NULL, no comma > +}; > + > +static struct device * > +scrub_device_register(struct device *dev, const char *name, void *drvdata, > + const struct scrub_ops *ops, > + int region_id, > + struct attribute_group *attr_group) > +{ > + struct scrub_device *scrub_dev; > + struct device *hdev; > + int err; > + > + scrub_dev = kzalloc(sizeof(*scrub_dev), GFP_KERNEL); > + if (!scrub_dev) > + return ERR_PTR(-ENOMEM); > + hdev = &scrub_dev->dev; > + > + scrub_dev->id = ida_alloc(&scrub_ida, GFP_KERNEL); > + if (scrub_dev->id < 0) { > + kfree(scrub_dev); > + return ERR_PTR(-ENOMEM); > + } > + > + snprintf((char *)scrub_dev->region_name, SCRUB_MAX_SYSFS_ATTR_NAME_LENGTH, > + "region%d", region_id); > + if (attr_group) { I'd like a comment on this. Not immediately obvious what this parameter is to me, or when we would and wouldn't have one. > + attr_group->name = (char *)scrub_dev->region_name; > + scrub_dev->groups[0] = attr_group; > + scrub_dev->region_id = region_id; > + } else { > + scrub_dev->group.name = (char *)scrub_dev->region_name; In both paths, drop out of if / else > + scrub_dev->group.attrs = scrub_attrs; > + scrub_dev->group.is_visible = scrub_attr_visible; > + scrub_dev->groups[0] = &scrub_dev->group; > + scrub_dev->ops = ops; > + scrub_dev->region_id = region_id; Set in both paths, so drop out of the if / else; > + } > + > + hdev->groups = scrub_dev->groups; > + hdev->class = &scrub_class; > + hdev->parent = dev; > + dev_set_drvdata(hdev, drvdata); > + dev_set_name(hdev, SCRUB_ID_FORMAT, scrub_dev->id); > + snprintf(scrub_dev->name, SCRUB_DEV_MAX_NAME_LENGTH, "%s", name); > + err = device_register(hdev); > + if (err) { > + put_device(hdev); > + return ERR_PTR(err); > + } > + > + return hdev; > +} > + > +static void devm_scrub_release(void *dev) > +{ > + struct device *hdev = dev; > + > + device_unregister(hdev); Trivial but local variable doesn't really add anything. deivce_unregister(dev); is pretty clear on types! > +} > diff --git a/include/memory/memory-scrub.h b/include/memory/memory-scrub.h > new file mode 100755 > index 000000000000..3d7054e98b9a > --- /dev/null > +++ b/include/memory/memory-scrub.h > @@ -0,0 +1,78 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Memory scrub controller driver support to configure > + * the controls of the memory scrub and enable. > + * > + * Copyright (c) 2023 HiSilicon Limited. > + */ > + > +#ifndef __MEMORY_SCRUB_H > +#define __MEMORY_SCRUB_H > + > +#include > + > +enum scrub_types { > + scrub_common, > + scrub_max, No comma on a terminating entry like this. > +};