Received-SPF: pass (google.com: domain of linux-kernel+bounces-203301-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <fcd0621b-dd68-4e0d-96e1-15c16a3278d0@os.amperecomputing.com>
Date: Wed, 5 Jun 2024 14:33:22 -0700
User-Agent: Mozilla Thunderbird
From: Daniel Ferguson <danielf@os.amperecomputing.com>
Subject: Re: [RFC PATCH v8 10/10] ras: scrub: ACPI RAS2: Add memory ACPI RAS2
 driver
To: shiju.jose@huawei.com
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
 david@redhat.com, Vilas.Sridharan@amd.com, leo.duran@amd.com,
 Yazen.Ghannam@amd.com, rientjes@google.com, jiaqiyan@google.com,
 tony.luck@intel.com, Jon.Grimm@amd.com, dave.hansen@linux.intel.com,
 rafael@kernel.org, lenb@kernel.org, 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@huawei.com,
 prime.zeng@hisilicon.com, kangkang.shen@futurewei.com,
 wanghuiqiang@huawei.com, linuxarm@huawei.com, ira.weiny@intel.com,
 vishal.l.verma@intel.com, alison.schofield@intel.com, dave.jiang@intel.com,
 jonathan.cameron@huawei.com, dave@stgolabs.net, dan.j.williams@intel.com,
 linux-mm@kvack.org, linux-acpi@vger.kernel.org, linux-cxl@vger.kernel.org
References: <20240419164720.1765-1-shiju.jose@huawei.com>
 <20240419164720.1765-11-shiju.jose@huawei.com>
Content-Language: en-US
In-Reply-To: <20240419164720.1765-11-shiju.jose@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?cGlRb25rRDJuK3lRbWNDSE5GTktMMXhtdmRzVkU1U2MwT2cwc1hXckdTTk83?=
 =?utf-8?B?U05Bd056Ylk4QW5HZkVnMStJQWRnU29MdkNhb3lGdWRWckZZOUR4OUNLNWtC?=
 =?utf-8?B?aUJ4RDNtdlNSSUVVWTc1MzZtakVHcW00QTBOeHVRamtuUStHalovR2YweVdB?=
 =?utf-8?B?R25NcVcvVktxMStNeTh0NE9Kd0I1VUlNSDN0bnhTWklpT3lSbXRZTW4rMEhz?=
 =?utf-8?B?cElDa1RDZC9XUGxqc1hURnhEY0ZkVnpObzVTSlRKSEhHTGY3V1BHOE82eEs4?=
 =?utf-8?B?b2RGbHR6SExrNVZVbzhsRC9zTW9KQmkrb3dPSU9vRGtRTmhZTEwyclB1UkpS?=
 =?utf-8?B?cVQwdWlHTTd1NkI2SWZyM25ZNStZYXRYYnY5VzFwSXFYZ2sweHVGSG9LK25n?=
 =?utf-8?B?WjFxbExvN3hqOUwrUmR5NXRjZzNMTHBiWWlFdWptWm1PY3lCNm8wYVRkclJy?=
 =?utf-8?B?ejlVUkJnT2txaDdBM1Zua0RsNmVlb0xKaFR4ck0ydVpJODZnM09uZm9GMUIz?=
 =?utf-8?B?Z010QTdhVHdVQ2NzMUtEcTQ2clNpQTFDSWNSd0s5YTh3TFdaN085c040QUtD?=
 =?utf-8?B?VGZKckZHN3pJN3p6c01OeTk3ZlBTcFM2ajJNZDZ5eVdkQmx3dmdOclg5S05i?=
 =?utf-8?B?K1o5K2VTUURFWjdWYjJ5eGUzeGFrQW9XN1RnOXpUY0N3L1Q1SXFtbmJ0UzJr?=
 =?utf-8?B?T1ZRUUhvbGh1dUVraVlhYllIY2JvK3JZUnEvdDZkS21jWVBYeXcvWWlteUZq?=
 =?utf-8?B?anJoRmdkOEE3S3grdyt1bHlvRFM3V1NtN2VjNjFYSHRiZnhNNWtPSmV6T1g0?=
 =?utf-8?B?U0ppcnJFendaY3NrQTNvU3lQRXJVb3RCWEUyZmhLQWxQczdlNVlDWDlkSDlT?=
 =?utf-8?B?WkNKWXdyV2VlQ3ZMMDVSR1hrRjBIQU9hSys4S2c1M09OMVNDRDRsa3JWL2V6?=
 =?utf-8?B?ZktmcTh6UzlkSGQ5aG91K3VMNTlDSTQxbU1TZ0RmdWdYckZlMTU1SHBKSU1j?=
 =?utf-8?B?QWRsOUxJZFFBU3pkOEQ5anM5a2wzMEFsaHlnUTkyL1dRUDdPWC90R3dta1pP?=
 =?utf-8?B?Q1grc3VrNTJiaFRZdFhJU21seE9LM0c5QmlzVkxRVjJXa0dNUWE2TnZKZDFt?=
 =?utf-8?B?dzA3Um5kWHZ5UFh0SFJGNWl6MndEZkFaVWY5ZjcwYThQVk9SakN5ZGthMGdp?=
 =?utf-8?B?K0JQclBNR1lWam5NZzk3bC9kbU92TVdtRzVZczlnUStORXVVWE1hZGdXR2Vm?=
 =?utf-8?B?amZWOTU5d3lISkowNUdQdkJoSXJEZThhWUJldDlRdWI1NFNsWXhvcGordU5R?=
 =?utf-8?B?aEEzUGpEUFA5b0o0a1orMVdqYk1aWG83ZjE5TDJqd3hRcE5vN2NhSFNic0Qv?=
 =?utf-8?B?M0FDcEpSUFVJN2x2cDM1WnpGZW5jbEhvY1llYWNPdW9Ca1NEaGR2VGFyMmJn?=
 =?utf-8?B?TXNYcFVwWWlUVUU1Q3QwOU9OMkE0MDFwVk1ZS2xWWmNUTndYL3A4NW1WZnFt?=
 =?utf-8?B?NCt5czU2YmRwamc3Vjc3ZW0vZlZ4R09kVnVlSkFlbEYxbmJNL3NqQlFUbWp1?=
 =?utf-8?B?RFNLZkgwRGxTUjFadTk5ZFlHb0dFdXNQcW1CTitmRktrM00xcThwRlVNTlBa?=
 =?utf-8?B?MUR6TU1qRUk5VE5NK0JxWUpNY1RKekMxbisxamZHMDhzR1libHZ5aUQ3Qlha?=
 =?utf-8?B?ZHZHWnlJMXphNUJnWm1RTVMzc2ZKR3FPbng3TzROR0NydHZBOGZIV1ZuNlNN?=
 =?utf-8?B?YndVUVRBZ2oweU9JTkVYbGZhd05CKzBxLy91WkV5ZmZ1QmMrNTdScElxVnBY?=
 =?utf-8?B?ZUpvc2l4YXphcGx3UXpaNEhRdk83a2FyR0FRbUJnTWlvTVJpV1dKc3ZQNi9t?=
 =?utf-8?B?TmJmUDRXdE4yOEMzSTVSYjd0VngyVmFLcDBiOHNId2VHME9OamUxcGNoZjIx?=
 =?utf-8?B?MU1KNWcyQzRlZ1hvbERJeURlanpVb3ZMam5jRDM5cjJ1KzNJb25BY0FDbWxU?=
 =?utf-8?B?c2hNbE0rSlU5b25keTVReWU2Z0xyNENGaU1rTVh0aitWcTl2anZJWG81QmxW?=
 =?utf-8?B?ZkF6cXJ3cmtIdjJQaXhYMzF3TzUxQ1MrSm1OYjl0NWJlaFM5YlArbHlpR0NM?=
 =?utf-8?B?VmFYSzE0cDlDN05sNHZPY25Vb1gvNGFLWFJtQW95VUd0aXFORmVvdVRwdkht?=
 =?utf-8?Q?CYnz6wMPYc8KFkrHAkhCAIo=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5d775375-8cb4-4fc9-def5-08dc85a721ad
X-MS-Exchange-CrossTenant-AuthSource: CO1PR01MB7370.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 21:33:25.1594
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: frie/ux3zzyMi1FMXLo2vFQeTygs9mH5WsFgI0QkQsjeMdqiF0I33B+fpdb3505h5Q8zhdYFKAqtqc3ZPoBuBH1DkEvSymAdVpU8dwfGjKnZgjUrMbP16PKxwNUyrdsc
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV3PR01MB8462

> +/* Context - lock must be held */
> +static int ras2_get_patrol_scrub_running(struct ras2_scrub_ctx *ras2_ctx,
> +					 bool *running)
> +{
> +	struct acpi_ras2_ps_shared_mem __iomem *ps_sm = (void *)
> +					ras2_ctx->pcc_subspace->pcc_comm_addr;
> +	int ret;
> +
> +	if (ras2_ctx->bg)
> +		*running = true;
> +
> +	ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
> +	ps_sm->params.patrol_scrub_command = RAS2_GET_PATROL_PARAMETERS;

Need to reset the address range (base and size). A user may have previously
called "Enable Background" where the code zeros out these parameters.
	ps_sm->params.requested_address_range[0] = ras2_ctx->base;
	ps_sm->params.requested_address_range[1] = ras2_ctx->size;


> +
> +	ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
> +	if (ret) {
> +		dev_err(ras2_ctx->dev, "failed to read parameters\n");
> +		return ret;
> +	}
> +
> +	*running = ps_sm->params.flags & RAS2_PATROL_SCRUB_FLAG_SCRUBBER_RUNNING;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_write_rate(struct device *dev, u64 rate)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +	bool running;
> +	int ret;
> +
> +	guard(mutex)(&ras2_ctx->lock);
> +	ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
> +	if (ret)
> +		return ret;
> +
> +	if (running)
> +		return -EBUSY;


I suggest we do not check if the patrol scrub is running when we are merely
updating cached values. More importantly, if we had
previously wrote an invalid value (that is only invalidated by firmware
after
executing a command), then when we try to write a correct value,
this "ras2_get_patrol_scrub_running" check will always fail, therefore
preventing us from correcting our error.

> +
> +	if (rate < ras2_ctx->rate_min || rate > ras2_ctx->rate_max)
> +		return -EINVAL;
> +
> +	ras2_ctx->rate = rate;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_read_rate(struct device *dev, u64 *rate)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +
> +	*rate = ras2_ctx->rate;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_read_rate_avail(struct device *dev, u64 *min, u64 *max)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +
> +	*min = ras2_ctx->rate_min;
> +	*max = ras2_ctx->rate_max;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_read_range(struct device *dev, u64 *base, u64 *size)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +
> +	*base = ras2_ctx->base;
> +	*size = ras2_ctx->size;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_write_range(struct device *dev, u64 base, u64 size)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +	bool running;
> +	int ret;
> +
> +	guard(mutex)(&ras2_ctx->lock);
> +	ret = ras2_get_patrol_scrub_running(ras2_ctx, &running);
> +	if (ret)
> +		return ret;
> +
> +	if (running)
> +		return -EBUSY;

I suggest we do not check if the patrol scrub is running. See previous
comment above.

> +
> +	ras2_ctx->base = base;
> +	ras2_ctx->size = size;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_set_enabled_bg(struct device *dev, bool enable)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +	struct acpi_ras2_ps_shared_mem __iomem *ps_sm = (void *)
> +					ras2_ctx->pcc_subspace->pcc_comm_addr;
> +	int ret;
> +
> +	guard(mutex)(&ras2_ctx->lock);
> +	ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
> +	if (enable) {
> +		ps_sm->params.requested_address_range[0] = 0;
> +		ps_sm->params.requested_address_range[1] = 0;
> +		ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_RATE_IN_MASK;
> +		ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_RATE_IN_MASK,
> +							    ras2_ctx->rate);
> +		ps_sm->params.patrol_scrub_command = RAS2_START_PATROL_SCRUBBER;
> +	} else {
> +		ps_sm->params.patrol_scrub_command = RAS2_STOP_PATROL_SCRUBBER;
> +	}
> +	ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_EN_BACKGROUND;
> +	ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_EN_BACKGROUND,
> +						    enable);
> +
> +	ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
> +	if (ret) {
> +		dev_err(ras2_ctx->dev, "%s: failed to enable(%d) background scrubbing\n",
> +			__func__, enable);
> +		return ret;
> +	}
> +	ras2_ctx->bg = true;
> +
> +	/* Update the cache to account for rounding of supplied parameters and similar */
> +	return ras2_update_patrol_scrub_params_cache(ras2_ctx);
> +}
> +
> +static int ras2_hw_scrub_get_enabled_bg(struct device *dev, bool *enabled)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +
> +	*enabled = ras2_ctx->bg;
> +
> +	return 0;
> +}
> +
> +static int ras2_hw_scrub_set_enabled_od(struct device *dev, bool enable)
> +{
> +	struct ras2_scrub_ctx *ras2_ctx = dev_get_drvdata(dev);
> +	struct acpi_ras2_ps_shared_mem __iomem *ps_sm = (void *)
> +					ras2_ctx->pcc_subspace->pcc_comm_addr;
> +	bool enabled;
> +	int ret;
> +
> +	guard(mutex)(&ras2_ctx->lock);
> +	ps_sm->common.set_capabilities[0] = RAS2_SUPPORT_HW_PARTOL_SCRUB;
> +	if (enable) {
> +		if (!ras2_ctx->size) {
> +			dev_warn(ras2_ctx->dev,
> +				 "%s: Invalid requested address range, requested_address_range[0]=0x%llx "
> +				 "requested_address_range[1]=0x%llx\n", __func__,
> +				 ps_sm->params.requested_address_range[0],
> +				 ps_sm->params.requested_address_range[1]);
> +			return -ERANGE;
> +		}
> +		ret = ras2_get_patrol_scrub_running(ras2_ctx, &enabled);
> +		if (ret)
> +			return ret;
> +
> +		if (enabled)
> +			return 0;
> +
> +		ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_RATE_IN_MASK;
> +		ps_sm->params.scrub_params_in |= FIELD_PREP(RAS2_PATROL_SCRUB_RATE_IN_MASK,
> +							    ras2_ctx->rate);
> +		ps_sm->params.requested_address_range[0] = ras2_ctx->base;
> +		ps_sm->params.requested_address_range[1] = ras2_ctx->size;


We need to clear the RAS2_PATROL_SCRUB_EN_BACKGROUND bit in the input
parameters.
This is in case "Enable Background" was previously called, and this bit
was set.

		ps_sm->params.scrub_params_in &= ~RAS2_PATROL_SCRUB_EN_BACKGROUND;


> +		ps_sm->params.patrol_scrub_command = RAS2_START_PATROL_SCRUBBER;
> +	} else {
> +		ps_sm->params.patrol_scrub_command = RAS2_STOP_PATROL_SCRUBBER;
> +	}
> +
> +	ret = ras2_send_pcc_cmd(ras2_ctx, RAS2_PCC_CMD_EXEC);
> +	if (ret) {
> +		dev_err(ras2_ctx->dev, "failed to enable(%d) the demand scrubbing\n", enable);
> +		return ret;
> +	}
> +	ras2_ctx->bg = false;
> +
> +	return ras2_update_patrol_scrub_params_cache(ras2_ctx);
> +}