>-----Original Message-----
>From: Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>
>Sent: 16 May 2025 20:05
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>; linux-edac@xxxxxxxxxxxxxxx; linux-
>acpi@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
>Cc: bp@xxxxxxxxx; rafael@xxxxxxxxxx; tony.luck@xxxxxxxxx; lenb@xxxxxxxxxx;
>leo.duran@xxxxxxx; Yazen.Ghannam@xxxxxxx; mchehab@xxxxxxxxxx;
>Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>; linux-mm@xxxxxxxxx;
>Linuxarm <linuxarm@xxxxxxxxxx>; rientjes@xxxxxxxxxx;
>jiaqiyan@xxxxxxxxxx; Jon.Grimm@xxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
>naoya.horiguchi@xxxxxxx; james.morse@xxxxxxx; jthoughton@xxxxxxxxxx;
>somasundaram.a@xxxxxxx; erdemaktas@xxxxxxxxxx; pgonda@xxxxxxxxxx;
>duenwen@xxxxxxxxxx; gthelen@xxxxxxxxxx;
>wschwartz@xxxxxxxxxxxxxxxxxxx; dferguson@xxxxxxxxxxxxxxxxxxx;
>wbs@xxxxxxxxxxxxxxxxxxxxxx; nifan.cxl@xxxxxxxxx; tanxiaofei
><tanxiaofei@xxxxxxxxxx>; Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; Roberto
>Sassu <roberto.sassu@xxxxxxxxxx>; kangkang.shen@xxxxxxxxxxxxx;
>wanghuiqiang <wanghuiqiang@xxxxxxxxxx>
>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
