Thanks Hans
On 2021-05-27 5:17 a.m., Hans de Goede wrote:
> 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.
Sounds good to me.
>
> 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.
Seconded :)
Thank you to both of you for the reviews - I learnt a lot with this
particular patch set.
>
> 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 {}.
Agreed - I should have noticed that one. Thanks
>
>> + 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.
Thanks
>
>> + return -EIO;
>> + }
>> +
>> + s = kstrdup(obj->string.pointer, GFP_KERNEL);
>> + if (!s)
>> + return -ENOMEM;
>> + *string = s;
>> + return 0;
>> +}
>> +
>> +/* ------ Core interface functions ------------*/
>
Mark
