RE: [PATCH v6 1/2] ACPI:RAS2: Add ACPI RAS2 driver

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Daniel Ferguson <danielf@os.amperecomputing.com>
>Sent: 16 May 2025 20:05
>To: Shiju Jose <shiju.jose@huawei.com>; linux-edac@vger.kernel.org; linux-
>acpi@vger.kernel.org; linux-doc@vger.kernel.org
>Cc: bp@alien8.de; rafael@kernel.org; tony.luck@intel.com; lenb@kernel.org;
>leo.duran@amd.com; Yazen.Ghannam@amd.com; mchehab@kernel.org;
>Jonathan Cameron <jonathan.cameron@huawei.com>; linux-mm@kvack.org;
>Linuxarm <linuxarm@huawei.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; gthelen@google.com;
>wschwartz@amperecomputing.com; dferguson@amperecomputing.com;
>wbs@os.amperecomputing.com; nifan.cxl@gmail.com; tanxiaofei
><tanxiaofei@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>; Roberto
>Sassu <roberto.sassu@huawei.com>; kangkang.shen@futurewei.com;
>wanghuiqiang <wanghuiqiang@huawei.com>
>Subject: Re: [PATCH v6 1/2] ACPI:RAS2: Add ACPI RAS2 driver
>
>> +static int ras2_check_pcc_chan(struct ras2_pcc_subspace
>> +*pcc_subspace) {
>> +	struct acpi_ras2_shmem __iomem *gen_comm_base = pcc_subspace-
>>comm_addr;
>> +	u32 cap_status;
>> +	u16 status;
>> +	u32 rc;
>> +
>> +	/*
>> +	 * As per ACPI spec, the PCC space will be initialized by
>> +	 * platform and should have set the command completion bit when
>> +	 * PCC can be used by OSPM.
>> +	 *
>> +	 * Poll PCC status register every 3us(delay_us) for maximum of
>> +	 * deadline_us(timeout_us) until PCC command complete bit is
>set(cond).
>> +	 */
>> +	rc = readw_relaxed_poll_timeout(&gen_comm_base->status, status,
>> +					status &
>PCC_STATUS_CMD_COMPLETE, 3,
>> +					pcc_subspace->deadline_us);
>> +	if (rc) {
>> +		pr_warn("PCC check channel failed for : %d rc=%d\n",
>> +			pcc_subspace->pcc_id, rc);
>> +		return rc;
>> +	}
>> +
>> +	if (status & PCC_STATUS_ERROR) {
>> +		cap_status = readw_relaxed(&gen_comm_base-
>>set_caps_status);
>> +		rc = ras2_report_cap_error(cap_status);
>> +
>> +		status &= ~PCC_STATUS_ERROR;
>> +		writew_relaxed(status, &gen_comm_base->status);
>> +		return rc;
>> +	}
>> +
>> +	if (status & PCC_STATUS_CMD_COMPLETE)
>> +		return 0;
>> +
>> +	return -EIO;
>> +}
>
>Hi, I'm terribly sorry for the late churn
>
>It is our current belief that checking the set_caps_status is not dependent on if
>the PCC_STATUS_ERROR bit is set. It seems to us, that the PCC_STATUS_ERROR
>bit should only be set if there is a problem with the PCC protocol. We've
>interpreted the set_caps_status as a capability specific error reporting
>mechanism. We have tested the following amendment to this flow, and urge you
>to consider this change, or a functionally equivalent one:

Thanks Daniel for the suggestion. I have tested with the changes and will incorporate
in v7.

Thanks,
Shiju   
>
>diff --git a/drivers/acpi/ras2.c b/drivers/acpi/ras2.c index
>6bbb0091b4b3..3f73c9ff33a3 100644
>--- a/drivers/acpi/ras2.c
>+++ b/drivers/acpi/ras2.c
>@@ -116,18 +116,20 @@ static int ras2_check_pcc_chan(struct
>ras2_pcc_subspace
>*pcc_subspace)
>        }
>
>        if (status & PCC_STATUS_ERROR) {
>-               cap_status = readw_relaxed(&gen_comm_base->set_caps_status);
>-               rc = ras2_report_cap_error(cap_status);
>-
>                status &= ~PCC_STATUS_ERROR;
>                writew_relaxed(status, &gen_comm_base->status);
>-               return rc;
>+               return -EIO;
>        }
>
>-       if (status & PCC_STATUS_CMD_COMPLETE)
>-               return 0;
>
>-       return -EIO;
>+       if (!(status & PCC_STATUS_CMD_COMPLETE))
>+               return -EIO;
>+
>+       // Cache, Clear, and Report feature specific status
>+       cap_status = readw_relaxed(&gen_comm_base->set_caps_status);
>+       writew_relaxed(0x0, &gen_comm_base->set_caps_status);
>+       rc = ras2_report_cap_error(cap_status);
>+       return rc;
> }
>
>Thanks again,
>~Daniel