Hi Mark, Andy,
So as mentioned in my reply to patch 1/3, overall this looks pretty good.
There are a few very small issues remaining, but they are so small that
I've decided to fix them up and merge this into my review-hans branch
with the issues fixed up.
I plan to let this sit in review-hans a bit longer then usual to
give you (Mark) a chance to check out the changes and ack them
and to give Andy the time to check if his review remarks were
addressed to his liking.
I've put remarks inline / below about the 2 things which
I've fixed up in this patch.
Andy, thank you for your review of this. Your suggestions have
improved this driver, esp. the use of kasprintf has made some
of the functions a lot better.
On 5/26/21 10:14 PM, Mark Pearson wrote:
<snip>
> +/* Extract error string from WMI return buffer */
> +static int tlmi_extract_error(const struct acpi_buffer *output)
> +{
> + const union acpi_object *obj;
> +
> + obj = output->pointer;
> + if (!obj)
> + return -ENOMEM;
> + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> + kfree(obj);
This kfree() should not be here, all the callers of
tlmi_extract_error() unconditionally call:
kfree(output->pointer);
after calling tlmi_extract_error() so if hit this path with
the kfree(obj) in place we get a double free.
I've dropped the kfree() in my review-hans branch, as well as the
now no longer necessary {}.
> + return -EIO;
> + }
> + return tlmi_errstr_to_err(obj->string.pointer);
> +}
> +
> +/* Utility function to execute WMI call to BIOS */
> +static int tlmi_simple_call(const char *guid, const char *arg)
> +{
> + const struct acpi_buffer input = { strlen(arg), (char *)arg };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + acpi_status status;
> + int i, err;
> +
> + /*
> + * Duplicated call required to match bios workaround for behavior
> + * seen when WMI accessed via scripting on other OS.
> + */
> + for (i = 0; i < 2; i++) {
> + /* (re)initialize output buffer to default state */
> + output.length = ACPI_ALLOCATE_BUFFER;
> + output.pointer = NULL;
> +
> + status = wmi_evaluate_method(guid, 0, 0, &input, &output);
> + if (ACPI_FAILURE(status)) {
> + kfree(output.pointer);
> + return -EIO;
> + }
> + err = tlmi_extract_error(&output);
> + kfree(output.pointer);
> + if (err)
> + return err;
> + }
> + return 0;
> +}
> +
> +/* Extract output string from WMI return buffer */
> +static int tlmi_extract_output_string(const struct acpi_buffer *output,
> + char **string)
> +{
> + const union acpi_object *obj;
> + char *s;
> +
> + obj = output->pointer;
> + if (!obj)
> + return -ENOMEM;
> + if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> + kfree(obj);
Same as above, also fixed in the same way.
> + return -EIO;
> + }
> +
> + s = kstrdup(obj->string.pointer, GFP_KERNEL);
> + if (!s)
> + return -ENOMEM;
> + *string = s;
> + return 0;
> +}
> +
> +/* ------ Core interface functions ------------*/
Regards,
Hans
