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

[Daniel Ferguson <danielf@xxxxxxxxxxxxxxxxxxxxxx>]

> +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:

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