I know I have probably started sounding like a broken record by now, but
I still have not seen any response (apart from the typos getting fixed)
to my comments on this patch which I posted in January 2016 [1].
None of the issues I found back then are really critical, but I did
point out a potential memory leak (granted, an unlikely one), so it
might be a good idea to at least take a second look before merging.
[1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html
--
Best regards,
Michał Kępień
On Thu, Jun 01, 2017 at 10:43:39PM +0200, Michał Kępień wrote:
> I know I have probably started sounding like a broken record by now, but
> I still have not seen any response (apart from the typos getting fixed)
> to my comments on this patch which I posted in January 2016 [1].
>
> None of the issues I found back then are really critical, but I did
> point out a potential memory leak (granted, an unlikely one), so it
> might be a good idea to at least take a second look before merging.
>
> [1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html
Thanks for being persistent, some good points in there. I'd like to just squash
these into this patch (9/16), but I'll include them here for an ack from you and
Andy L. that this is what you meant, and consistent with his
intent/understanding:
>From 2512da1593574a66eb48d7105885e959b38db410 Mon Sep 17 00:00:00 2001
Message-Id: <2512da1593574a66eb48d7105885e959b38db410.1496717988.git.dvhart@infradead.org>
From: "Darren Hart (VMware)" <[email protected]>
Date: Mon, 5 Jun 2017 19:54:03 -0700
Subject: [PATCH] =?UTF-8?q?platform/x86:=20wmi:=20Apply=20fixes=20per=20Mi?=
=?UTF-8?q?cha=C5=82=20K=C4=99pie=C5=84=20(squash)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Per:
https://www.spinics.net/lists/platform-driver-x86/msg08201.html
Eliminate the kfree of a wblock with a null bus now that we ignore
duplicate GUIDs in parse_wdg.
Move the out_free_pointer: label and kfree to the end of the function,
clearly marking it as a return path.
Rework the device_add loop at the end of parse_wdg to avoid leaking the
wblock, and symmetrically call wmi_method_enable() if (debug_event).
Signed-off-by: Darren Hart (VMware) <[email protected]>
---
drivers/platform/x86/wmi.c | 34 +++++++++++++++-------------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bfc0a3f..fbce876 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -863,14 +863,7 @@ static void wmi_free_devices(struct acpi_device *device)
list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
if (wblock->acpi_device == device) {
list_del(&wblock->list);
- if (wblock->dev.dev.bus) {
- /* Device was initialized. */
- device_del(&wblock->dev.dev);
- put_device(&wblock->dev.dev);
- } else {
- /* device_initialize was not called. */
- kfree(wblock);
- }
+ device_unregister(&wblock->dev.dev);
}
}
}
@@ -903,9 +896,9 @@ static bool guid_already_parsed(struct acpi_device *device,
static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
{
struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
- union acpi_object *obj;
const struct guid_block *gblock;
struct wmi_block *wblock, *next;
+ union acpi_object *obj;
acpi_status status;
int retval = 0;
u32 i, total;
@@ -958,24 +951,27 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
}
}
-out_free_pointer:
- kfree(out.pointer);
-
/*
* Now that all of the devices are created, add them to the
* device tree and probe subdrivers.
*/
list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
- if (wblock->acpi_device == device) {
- if (device_add(&wblock->dev.dev) != 0) {
- dev_err(wmi_bus_dev,
- "failed to register %pULL\n",
- wblock->gblock.guid);
- list_del(&wblock->list);
- }
+ if (wblock->acpi_device != device)
+ continue;
+
+ retval = device_add(&wblock->dev.dev);
+ if (retval) {
+ dev_err(wmi_bus_dev, "failed to register %pULL\n",
+ wblock->gblock.guid);
+ if (debug_event)
+ wmi_method_enable(wblock, 0);
+ list_del(&wblock->list);
+ put_device(&wblock->dev.dev);
}
}
+out_free_pointer:
+ kfree(out.pointer);
return retval;
}
--
2.9.4
--
Darren Hart
VMware Open Source Technology Center
On Mon, Jun 5, 2017 at 8:03 PM, Darren Hart <[email protected]> wrote:
> On Thu, Jun 01, 2017 at 10:43:39PM +0200, Michał Kępień wrote:
>> I know I have probably started sounding like a broken record by now, but
>> I still have not seen any response (apart from the typos getting fixed)
>> to my comments on this patch which I posted in January 2016 [1].
>>
>> None of the issues I found back then are really critical, but I did
>> point out a potential memory leak (granted, an unlikely one), so it
>> might be a good idea to at least take a second look before merging.
>>
>> [1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html
>
> Thanks for being persistent, some good points in there. I'd like to just squash
> these into this patch (9/16), but I'll include them here for an ack from you and
> Andy L. that this is what you meant, and consistent with his
> intent/understanding:
>
Looks good to me.
> On Mon, Jun 5, 2017 at 8:03 PM, Darren Hart <[email protected]> wrote:
> > On Thu, Jun 01, 2017 at 10:43:39PM +0200, Michał Kępień wrote:
> >> I know I have probably started sounding like a broken record by now, but
> >> I still have not seen any response (apart from the typos getting fixed)
> >> to my comments on this patch which I posted in January 2016 [1].
> >>
> >> None of the issues I found back then are really critical, but I did
> >> point out a potential memory leak (granted, an unlikely one), so it
> >> might be a good idea to at least take a second look before merging.
> >>
> >> [1] https://www.spinics.net/lists/platform-driver-x86/msg08201.html
> >
> > Thanks for being persistent, some good points in there. I'd like to just squash
> > these into this patch (9/16), but I'll include them here for an ack from you and
> > Andy L. that this is what you meant, and consistent with his
> > intent/understanding:
> >
>
> Looks good to me.
Same here.
--
Best regards,
Michał Kępień