diff mbox

[09/16] platform/x86: wmi: Instantiate all devices before adding them

Message ID 20170606030302.GC27270@fury (mailing list archive)
State Accepted, archived
Delegated to: Darren Hart
Headers show

Commit Message

Darren Hart June 6, 2017, 3:03 a.m. UTC 
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)" <dvhart@infradead.org>
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) <dvhart@infradead.org>
---
 drivers/platform/x86/wmi.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Comments

Andy Lutomirski June 6, 2017, 4:03 p.m. UTC | #1 
On Mon, Jun 5, 2017 at 8:03 PM, Darren Hart <dvhart@infradead.org> 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.
Michał Kępień June 8, 2017, 4:43 a.m. UTC | #2 
> On Mon, Jun 5, 2017 at 8:03 PM, Darren Hart <dvhart@infradead.org> 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.
diff mbox

Patch

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;
 }