Re: [RFC PATCH v6 12/12] memory: RAS2: Add memory RAS2 driver
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Date: 2024-02-20 16:12:37
Also in:
linux-acpi, linux-cxl, linux-edac, lkml
On Thu, 15 Feb 2024 19:14:54 +0800 [off-list ref] wrote:
From: Shiju Jose <redacted> Memory RAS2 driver binds to the platform device add by the ACPI RAS2 driver. Driver registers the PCC channel for communicating with the ACPI compliant platform that contains RAS2 command support in the hardware. Add interface functions to support configuring the parameters of HW patrol scrubs in the system, which exposed to the kernel via the RAS2 and PCC, using the RAS2 commands. Add support for RAS2 platform devices to register with scrub subsystem driver. This enables user to configure the parameters of HW patrol scrubs, which exposed to the kernel via the RAS2 table, through the scrub sysfs attributes. Open Question: Sysfs scrub control attribute "enable_background_scrub" is added for RAS2, based on the feedback from Bill Schwartz <wschwartz@amperecomputing.com on v4 to enable/disable the background_scrubbing in the platform as defined in the “Configure Scrub Parameters [INPUT]“ field in RAS2 Table 5.87: Parameter Block Structure for PATROL_SCRUB. Is it a right approach to support "enable_background_scrub" in the sysfs scrub control?
Does anyone know what this means? IIUC patrol scrub is always background...
Signed-off-by: Shiju Jose <redacted>
A few minor comments inline. Rushed review as out of time for today though so may have missed stuff.
quoted hunk ↗ jump to hunk
diff --git a/drivers/memory/rasf_common.c b/drivers/memory/rasf_common.c new file mode 100644 index 000000000000..85f67308698d --- /dev/null +++ b/drivers/memory/rasf_common.c@@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * rasf_common.c - Common functions for memory RASF driver + * + * Copyright (c) 2023 HiSilicon Limited. + * + * This driver implements call back functions for the scrub + * configure driver to configure the parameters of the hw patrol + * scrubbers in the system, which exposed via the ACPI RASF/RAS2 + * table and PCC. + */ + +#define pr_fmt(fmt) "MEMORY RASF COMMON: " fmt + +#include <linux/acpi.h> +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/mailbox_controller.h> +#include <linux/mailbox_client.h> +#include <linux/module.h> +#include <linux/of.h>
of.h Shouldn't be here in an ACPI driver!
+#include <linux/platform_device.h>
+
+#include <acpi/rasf_acpi.h>
+#include <memory/rasf.h>
+
+static int enable_write(struct rasf_context *rasf_ctx, long val)
+{
+ int ret;
+ bool enable = val;
+
+ ret = rasf_ctx->ops->enable_scrub(rasf_ctx, enable);
+ if (ret) {
+ pr_err("enable patrol scrub fail, enable=%d ret=%d\n",
+ enable, ret);dev_err(rasf_ctx->dev,...
+ return ret; + } + + return 0; +}
+
+/**
+ * rasf_hw_scrub_is_visible() - Callback to return attribute visibility
+ * @drv_data: Pointer to driver-private data structure passed
+ * as argument to devm_scrub_device_register().
+ * @attr_id: Scrub attribute
+ * @region_id: ID of the memory region
+ *
+ * Returns: 0 on success, an error otherwise
+ */
+umode_t rasf_hw_scrub_is_visible(struct device *dev, u32 attr_id, int region_id)
+{
+ switch (attr_id) {
+ case scrub_rate_available:
+ return 0444;
+ case scrub_enable:
+ case scrub_enable_background_scrub:
+ return 0200;
+ case scrub_addr_base:
+ case scrub_addr_size:
+ case scrub_rate:
+ return 0644;As before, I'd prefer to see this passed the current permissions then just return those rather than encoding them here and in the attributes where they may end up out of sync
+ default: + return 0; + } +}
+ +/** + * rasf_hw_scrub_read_strings() - Read callback for string attributes + * @device: Pointer to scrub device + * @attr_id: Scrub attribute + * @region_id: ID of the memory region + * @buf: Pointer to the buffer for copying returned string + * + * Returns: 0 on success, an error otherwise + */ +int rasf_hw_scrub_read_strings(struct device *device, u32 attr_id, int region_id, + char *buf)
dev maybe instead of device. Shorter lines and it's very common shorthand.
+{
+ struct rasf_context *rasf_ctx;
+
+ rasf_ctx = dev_get_drvdata(device);struct rasf_context *rasf_ctx = dev_get_drvdata(dev); Same throughout.
+
+ switch (attr_id) {
+ case scrub_rate_available:
+ return rate_available_read(rasf_ctx, buf);
+ default:
+ return -ENOTSUPP;
+ }
+}