EDAC, dsm_edac: Wrap ACPI DSM methods for address translation
Borislav Petkov
maintainerPATCH 4/5 EDAC, dsm_edac: Wrap ACPI DSM methods for address translation
Quoted 2 lines · Tony
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.
Quoted 8 lines
+/**
+ *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 fromadxl_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.
Review
Quoted 4 lines
+ */
(quoted diff) +3
-0+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.
Quoted 20 lines
(quoted diff) +20
-0+} +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)
Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 16 lines
(quoted diff) +16
-0+ 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); +
Review
Quoted 10 lines
(quoted diff) +10
-0+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.
Quoted 13 lines
(quoted diff) +13
-0+ 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;
Review
Quoted 10 lines
(quoted diff) +10
-0+ } + + 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.
Quoted 13 lines
(quoted diff) +13
-0+ 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; +}