> At some point, we'll want to subdrivers to get references to other
Did you mean "(...) we'll want subdrivers to get references to (...)"
(without the first "to")?
> devices on the same WMI bus. This change is needed to avoid races.
>
> This ends up simplifying the setup code and fixng some leaks, too.
fixing
>
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
> ---
> drivers/platform/x86/wmi.c | 49 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 49be61207c4a..7d8a11a45bf9 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -869,7 +869,7 @@ static struct device_type wmi_type_data = {
> .release = wmi_dev_release,
> };
>
> -static int wmi_create_device(struct device *wmi_bus_dev,
> +static void wmi_create_device(struct device *wmi_bus_dev,
> const struct guid_block *gblock,
> struct wmi_block *wblock,
> struct acpi_device *device)
> @@ -923,7 +923,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>
> }
>
> - return device_register(&wblock->dev.dev);
> + device_initialize(&wblock->dev.dev);
> }
>
> static void wmi_free_devices(struct acpi_device *device)
> @@ -934,10 +934,14 @@ 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_unregister(&wblock->dev.dev);
> - else
> + if (wblock->dev.dev.bus) {
> + /* Device was initialized. */
> + device_del(&wblock->dev.dev);
> + put_device(&wblock->dev.dev);
Is there any specific reason for splitting device_unregister() into two
separate calls it performs?
> + } else {
> + /* device_initialize was not called. */
> kfree(wblock);
This kfree() obviously wasn't introduced by you, but I believe that
after patch 06 from this series is applied, this branch cannot be
reached any more. If I understand correctly, it could only be reached
(using code without this patch series applied) when duplicate GUIDs were
present, for which a struct wmi_block was allocated and added to
wmi_block_list, but for which wmi_create_device() wasn't called. Patch
06 prevents a struct wmi_block from ever being allocated for duplicate
GUIDs and this patch ensures that even if device_add() fails for some
reason, the relevant struct wmi_block is removed from wmi_block_list and
freed anyway. I also don't see any way for a struct wmi_block to be
inserted into wmi_block_list without both wmi_create_device() (so
device_initialize()) and device_add() being called. So perhaps this
kfree() call (and the relevant conditional expression) could be removed
in another patch?
> + }
> }
> }
> }
> @@ -972,9 +976,9 @@ 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;
> + struct wmi_block *wblock, *next;
> acpi_status status;
> - int retval;
> + int retval = 0;
> u32 i, total;
>
> status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
> @@ -1007,19 +1011,15 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
> continue;
>
> wblock = kzalloc(sizeof(struct wmi_block), GFP_KERNEL);
> - if (!wblock)
> - return -ENOMEM;
> + if (!wblock) {
> + retval = -ENOMEM;
> + break;
> + }
>
> wblock->acpi_device = device;
> wblock->gblock = gblock[i];
>
> - retval = wmi_create_device(wmi_bus_dev, &gblock[i],
> - wblock, device);
> - if (retval) {
> - put_device(&wblock->dev.dev);
> - wmi_free_devices(device);
> - goto out_free_pointer;
> - }
> + wmi_create_device(wmi_bus_dev, &gblock[i], wblock, device);
>
> list_add_tail(&wblock->list, &wmi_block_list);
>
> @@ -1029,11 +1029,24 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
> }
> }
>
> - retval = 0;
> -
> out_free_pointer:
> kfree(out.pointer);
Perhaps this label and the kfree() call should be moved closer to the
return statement below to avoid confusion? If obj is not a buffer then
no new struct wmi_block(s) will be added to wmi_block_list, so there is
little point in iterating over the latter below.
>
> + /*
> + * 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);
A put_device(&wblock->dev.dev) is missing here. If we don't do that,
the reference count for wblock->dev.dev will still be at 1 due to
device_initialize() being called before and we will leak wblock.
I have one more minor issue with error handling for device_add(). A bit
earlier, in the GUID-processing loop, there's this snippet:
if (debug_event) {
wblock->handler = wmi_notify_debug;
wmi_method_enable(wblock, 1);
}
So we should probably put:
if (debug_event)
wmi_method_enable(wblock, 0);
before the put_device() call I mentioned above.
--
Best regards,
Michał Kępień
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
