[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Borislav Petkov <bp@alien8.de>]

On Mon, Oct 08, 2018 at 09:57:06AM -0700, Luck, Tony wrote:
> This shouldn't conflict with other BIOS "value add" code for firmware
> first mode, etc.

"value add", yeah. So something has been added - dunno if it is of value.

> +/**
> + * adxl_get_component_names - get list of memory component names
> + * Returns NULL terminated list of string names
> + *
> + * Give the caller a pointer to the list of memory component names
> + * e.g. { "SystemAddress", "ProcessorSocketId", "ChannelId", ... NULL }
> + * Caller should count how many strings in order to allocate a buffer
> + * for the return from adxl_decode()

Nit: please end all your comments' sentences with a full stop.

Not so nitty: there's no guarantee that the order and string formatting of
all those is stable, right? I think you should state that in the driver
so that people don't get crazy ideas of relying on any of this. It is a
best effort and no more, I'd assume.

Also, you could point to how skx_edac is using this interface so that
people can get a good example.

> + */
> +char **adxl_get_component_names(void)
> +{
> +	return adxl_component_names;

Should this array be immutable? I.e., const char * const. So that people
don't accidentally overwrite it or poke funny chars in it.

> +}
> +EXPORT_SYMBOL_GPL(adxl_get_component_names);
> +
> +/**
> + * adxl_decode - ask BIOS to decode a system address to memory address
> + * @addr: the address to decode
> + * @component_values: pointer to array of values for each component
> + * Returns 0 on success, negative error code otherwise
> + *
> + * The index of each value returned in the array matches the index of
> + * each component name returned by adxl_get_component_names().
> + * Components that are not defined for this address translation (e.g.
> + * mirror channel number for a non-mirrored address) are set to ~0ull
> + */
> +int adxl_decode(u64 addr, u64 component_values[])
> +{
> +	union acpi_object argv4[2], *results, *r;
> +	int i, cnt;
> +
> +	if (!adxl_component_names)
> +		return -EOPNOTSUPP;
> +
> +	argv4[0].type = ACPI_TYPE_PACKAGE;
> +	argv4[0].package.count = 1;
> +	argv4[0].package.elements = &argv4[1];
> +	argv4[1].integer.type = ACPI_TYPE_INTEGER;
> +	argv4[1].integer.value = addr;
> +
> +	results = adxl_dsm(ADXL_IDX_FORWARD_TRANSLATE, argv4);
> +

Superfluous newline.

> +	if (!results)
> +		return -EINVAL;
> +
> +	r = results->package.elements + 1;
> +	cnt = r->package.count;
> +	r = r->package.elements;
> +
> +	for (i = 0; i < cnt; i++)
> +		component_values[i] = r[i].integer.value;
> +
> +	ACPI_FREE(results);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(adxl_decode);
> +
> +static bool adxl_detect(void)
> +{
> +	char *path = ACPI_ADXL_PATH;
> +	union acpi_object *p;
> +	acpi_status status;
> +	int i, cnt;
> +
> +	status = acpi_get_handle(NULL, path, &handle);
> +	if (ACPI_FAILURE(status)) {
> +		pr_debug("No ACPI handle for path %s\n", path);

Shouldn't all those be pr_info?

We had the same problem with einj.ko and not knowing why it wouldn't load.

> +		return false;
> +	}
> +
> +	if (!acpi_has_method(handle, "_DSM")) {
> +		pr_debug("No DSM method\n");
> +		return false;
> +	}
> +
> +	if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION,
> +			    ADXL_IDX_GET_ADDR_PARAMS |
> +			    ADXL_IDX_FORWARD_TRANSLATE)) {
> +		pr_debug("No ADXL DSM methods\n");
> +		return false;
> +	}
> +
> +	params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL);
> +	if (!params) {
> +		pr_debug("Failed to get params\n");
> +		return false;
> +	}
> +
> +	p = params->package.elements + 1;
> +	cnt = p->package.count;

I guess you need to sanity-check that count - we don't trust BIOS and
you have a firmware-controlled allocation size.

> +	p = p->package.elements;
> +
> +	adxl_component_names = kcalloc(cnt + 1, sizeof(char *), GFP_KERNEL);
> +	if (!adxl_component_names) {
> +		ACPI_FREE(params);
> +		return false;
> +	}
> +
> +	for (i = 0; i < cnt; i++)
> +		adxl_component_names[i] = p[i].string.pointer;
> +
> +	return true;
> +}