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

[Daniel Ferguson <danielf@os.amperecomputing.com>]

> +static int ras2_report_cap_error(u32 cap_status)
> +{
> +	switch (cap_status) {
> +	case ACPI_RAS2_NOT_VALID:
> +	case ACPI_RAS2_NOT_SUPPORTED:
> +		return -EPERM;
> +	case ACPI_RAS2_BUSY:
> +		return -EBUSY;
> +	case ACPI_RAS2_FAILED:
> +	case ACPI_RAS2_ABORTED:
> +	case ACPI_RAS2_INVALID_DATA:
> +		return -EINVAL;
> +	default: /* 0 or other, Success */
> +		return 0;
> +	}
> +}
> +
> +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;
> +}

We still have an outstanding problem. This may sound familiar.

If a user specifies an invalid address, our firmware will set an error code in
the set_caps_status field of the acpi_ras2_shmem structure. In our case, the
error code is ACPI_RAS2_INVALID_DATA, and the user will observe an EINVAL. This
is expected.

However, if the user then subsequently attempts to write a VALID address,
ras2_get_patrol_scrub_running will indirectly call ras2_check_pcc_chan using the
previously INVALID address to determine if the scrubber is still running.
Unfortunately, the INVALID address causes ras2_get_patrol_scrub_running to fail,
therefore preventing the user from specifying a VALID address after specifying
an INVALID address.

The only way to move forward from this inescapable condition is to reboot the
system.

Here is a demo of the problem as I roughly see it on our system (I've labeled
the line numbers for sake of discussion):
1  [root@myhost scrub0]# echo 0x100000000 > size
2  [root@myhost scrub0]# echo 0x1f00000000 > addr
3  [root@myhost scrub0]# echo 0xcf00000000 > addr
4  write error: Invalid argument
5  [  214.446338] PCCT PCCT: Failed to start demand scrubbing
6  [root@myhost scrub0]# echo 0x1f00000000 > addr
7  write error: Invalid argument
8  [  242.263909] PCCT PCCT: failed to read parameters
9  [root@myhost scrub0]# echo 0x100000000 > size
10 write error: Invalid argument
11 [  246.190196] PCCT PCCT: failed to read parameters

The upper most memory address on this system is 0xbf00000000. Line 1 and 2 use
valid values, and line 2 produces the expected results. On line 3, I've
specified an INVALID address (outside of valid range). The error on line 5 is
expected after executing the START_PATROL_SCRUBBER command with an INVALID address.

Line 6 show how I attempt to specify a VALID address. Unfortunately,
ras2_get_patrol_scrub_running encounters and error after executing
GET_PATROL_PARAMETERS because it used the OLD INVALID values in
ps_sm->params.req_addr_range. Line 7 and 8 are the result. Since the flow of
execution if aborted at this point, you can never rectify the situation and
insert a valid value into ps_sm->params.req_addr_range, unless you reboot
the system.

One half baked solution to this problem, is to modify
ras2_get_patrol_scrub_running so that if there is a non-zero address or size
specified, AND the last error code we received was INVALID DATA, then assume the
scrubber is NOT running.

Regards,
~Daniel