Re: [PATCH 05/14] wmi: Turn WMI into a bus driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jan 16, 2016 at 4:56 AM, Michał Kępień <kernel@xxxxxxxxxx> wrote:
>> WMI is logically a bus: the WMI driver binds to an ACPI node (or
>> more than one), and each instance of the WMI driver enumerates its
>> children and hopes that drivers will attach to the children that are
>> useful.
>>
>> This patch gives WMI a driver model bus type and the ability to
>> match to drivers.  The bus itself is a device in the new "wmi_bus"
>> class, and all of the individual WMI devices are slotted into the
>> device hierarchy correctly.
>>
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> ---
>>  drivers/platform/x86/wmi.c | 198 +++++++++++++++++++++++++++++++++++----------
>>  include/linux/wmi.h        |  47 +++++++++++
>>  2 files changed, 202 insertions(+), 43 deletions(-)
>>  create mode 100644 include/linux/wmi.h
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index dcb34a5f5669..2fa9493bd35c 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -37,14 +37,13 @@
>>  #include <linux/acpi.h>
>>  #include <linux/slab.h>
>>  #include <linux/module.h>
>> +#include <linux/wmi.h>
>>
>>  ACPI_MODULE_NAME("wmi");
>>  MODULE_AUTHOR("Carlos Corbacho");
>>  MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
>>  MODULE_LICENSE("GPL");
>>
>> -#define ACPI_WMI_CLASS "wmi"
>> -
>>  static LIST_HEAD(wmi_block_list);
>>
>>  struct guid_block {
>> @@ -61,12 +60,12 @@ struct guid_block {
>>  };
>>
>>  struct wmi_block {
>> +     struct wmi_device dev;
>>       struct list_head list;
>>       struct guid_block gblock;
>>       struct acpi_device *acpi_device;
>>       wmi_notify_handler handler;
>>       void *handler_data;
>> -     struct device dev;
>>  };
>>
>>
>> @@ -101,8 +100,8 @@ static const struct acpi_device_id wmi_device_ids[] = {
>>  MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
>>
>>  static struct acpi_driver acpi_wmi_driver = {
>> -     .name = "wmi",
>> -     .class = ACPI_WMI_CLASS,
>> +     .name = "acpi-wmi",
>> +     .owner = THIS_MODULE,
>>       .ids = wmi_device_ids,
>>       .ops = {
>>               .add = acpi_wmi_add,
>> @@ -623,77 +622,146 @@ bool wmi_has_guid(const char *guid_string)
>>  }
>>  EXPORT_SYMBOL_GPL(wmi_has_guid);
>>
>> +static struct wmi_block *dev_to_wblock(struct device *dev)
>> +{
>> +     return container_of(dev, struct wmi_block, dev.dev);
>> +}
>> +
>> +static struct wmi_device *dev_to_wdev(struct device *dev)
>> +{
>> +     return container_of(dev, struct wmi_device, dev);
>> +}
>> +
>>  /*
>>   * sysfs interface
>>   */
>>  static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
>>                            char *buf)
>>  {
>> -     struct wmi_block *wblock;
>> -
>> -     wblock = dev_get_drvdata(dev);
>> -     if (!wblock) {
>> -             strcat(buf, "\n");
>> -             return strlen(buf);
>> -     }
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>>
>>       return sprintf(buf, "wmi:%pUL\n", wblock->gblock.guid);
>>  }
>>  static DEVICE_ATTR_RO(modalias);
>>
>> +static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
>> +                      char *buf)
>> +{
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>> +
>> +     return sprintf(buf, "%pUL\n", wblock->gblock.guid);
>> +}
>> +static DEVICE_ATTR_RO(guid);
>> +
>>  static struct attribute *wmi_attrs[] = {
>>       &dev_attr_modalias.attr,
>> +     &dev_attr_guid.attr,
>>       NULL,
>>  };
>>  ATTRIBUTE_GROUPS(wmi);
>>
>>  static int wmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
>>  {
>> -     char guid_string[37];
>> -
>> -     struct wmi_block *wblock;
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>>
>> -     if (add_uevent_var(env, "MODALIAS="))
>> +     if (add_uevent_var(env, "MODALIAS=wmi:%pUL", wblock->gblock.guid))
>>               return -ENOMEM;
>>
>> -     wblock = dev_get_drvdata(dev);
>> -     if (!wblock)
>> +     if (add_uevent_var(env, "WMI_GUID=%pUL", wblock->gblock.guid))
>>               return -ENOMEM;
>>
>> -     sprintf(guid_string, "%pUL", wblock->gblock.guid);
>> +     return 0;
>> +}
>> +
>> +static void wmi_dev_release(struct device *dev)
>> +{
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>> +
>> +     kfree(wblock);
>> +}
>> +
>> +static int wmi_dev_match(struct device *dev, struct device_driver *driver)
>> +{
>> +     struct wmi_driver *wmi_driver =
>> +             container_of(driver, struct wmi_driver, driver);
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>> +     const struct wmi_device_id *id = wmi_driver->id_table;
>>
>> -     strcpy(&env->buf[env->buflen - 1], "wmi:");
>> -     memcpy(&env->buf[env->buflen - 1 + 4], guid_string, 36);
>> -     env->buflen += 40;
>> +     while (id->guid_string) {
>> +             u8 tmp[16], driver_guid[16];
>> +
>> +             wmi_parse_guid(id->guid_string, tmp);
>> +             wmi_swap_bytes(tmp, driver_guid);
>> +             if (!memcmp(driver_guid, wblock->gblock.guid, 16))
>> +                     return 1;
>> +
>> +             id++;
>> +     }
>>
>>       return 0;
>>  }
>>
>> -static void wmi_dev_free(struct device *dev)
>> +int wmi_dev_probe(struct device *dev)
>
> Can be static.

Indeed, thanks.

>
>>  {
>> -     struct wmi_block *wmi_block = container_of(dev, struct wmi_block, dev);
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>> +     struct wmi_driver *wdriver =
>> +             container_of(dev->driver, struct wmi_driver, driver);
>> +     int ret = 0;
>> +
>> +     if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
>> +             dev_warn(dev, "failed to enable device -- probing anyway\n");
>> +
>> +     if (wdriver->probe) {
>> +             ret = wdriver->probe(dev_to_wdev(dev));
>> +             if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
>> +                     dev_warn(dev, "failed to disable device\n");
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +int wmi_dev_remove(struct device *dev)
>
> Can be static.

Fixed.

>
>> +{
>> +     struct wmi_block *wblock = dev_to_wblock(dev);
>> +     struct wmi_driver *wdriver =
>> +             container_of(dev->driver, struct wmi_driver, driver);
>> +     int ret = 0;
>> +
>> +     if (wdriver->remove)
>> +             ret = wdriver->remove(dev_to_wdev(dev));
>> +
>> +     if (ACPI_FAILURE(wmi_method_enable(wblock, 0)))
>> +             dev_warn(dev, "failed to disable device\n");
>>
>> -     kfree(wmi_block);
>> +     return ret;
>>  }
>>
>> -static struct class wmi_class = {
>> +static struct class wmi_bus_class = {
>> +     .name = "wmi_bus",
>> +};
>> +
>> +static struct bus_type wmi_bus_type = {
>>       .name = "wmi",
>> -     .dev_release = wmi_dev_free,
>> -     .dev_uevent = wmi_dev_uevent,
>>       .dev_groups = wmi_groups,
>> +     .match = wmi_dev_match,
>> +     .uevent = wmi_dev_uevent,
>> +     .probe = wmi_dev_probe,
>> +     .remove = wmi_dev_remove,
>>  };
>>
>> -static int wmi_create_device(const struct guid_block *gblock,
>> +static int wmi_create_device(struct device *wmi_bus_dev,
>> +                          const struct guid_block *gblock,
>>                            struct wmi_block *wblock,
>>                            struct acpi_device *device)
>>  {
>> -     wblock->dev.class = &wmi_class;
>> +     wblock->dev.dev.bus = &wmi_bus_type;
>> +     wblock->dev.dev.parent = wmi_bus_dev;
>>
>> -     dev_set_name(&wblock->dev, "%pUL", gblock->guid);
>> +     dev_set_name(&wblock->dev.dev, "%pUL", gblock->guid);
>>
>> -     dev_set_drvdata(&wblock->dev, wblock);
>> +     wblock->dev.dev.release = wmi_dev_release;
>>
>> -     return device_register(&wblock->dev);
>> +     return device_register(&wblock->dev.dev);
>>  }
>>
>>  static void wmi_free_devices(struct acpi_device *device)
>> @@ -704,8 +772,8 @@ 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.class)
>> -                             device_unregister(&wblock->dev);
>> +                     if (wblock->dev.dev.bus)
>> +                             device_unregister(&wblock->dev.dev);
>>                       else
>>                               kfree(wblock);
>>               }
>> @@ -737,7 +805,7 @@ static bool guid_already_parsed(struct acpi_device *device,
>>  /*
>>   * Parse the _WDG method for the GUID data blocks
>>   */
>> -static int parse_wdg(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;
>> @@ -781,7 +849,8 @@ static int parse_wdg(struct acpi_device *device)
>>                 for device creation.
>>               */
>>               if (!guid_already_parsed(device, gblock[i].guid)) {
>> -                     retval = wmi_create_device(&gblock[i], wblock, device);
>> +                     retval = wmi_create_device(wmi_bus_dev, &gblock[i],
>> +                                                wblock, device);
>>                       if (retval) {
>>                               wmi_free_devices(device);
>>                               goto out_free_pointer;
>> @@ -881,12 +950,15 @@ static int acpi_wmi_remove(struct acpi_device *device)
>>       acpi_remove_address_space_handler(device->handle,
>>                               ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler);
>>       wmi_free_devices(device);
>> +     device_unregister((struct device *)acpi_driver_data(device));
>> +     device->driver_data = NULL;
>>
>>       return 0;
>>  }
>>
>>  static int acpi_wmi_add(struct acpi_device *device)
>>  {
>> +     struct device *wmi_bus_dev;
>>       acpi_status status;
>>       int error;
>>
>> @@ -899,14 +971,26 @@ static int acpi_wmi_add(struct acpi_device *device)
>>               return -ENODEV;
>>       }
>>
>> -     error = parse_wdg(device);
>> +     wmi_bus_dev = device_create(&wmi_bus_class, &device->dev, MKDEV(0, 0),
>> +                                 NULL, "wmi_bus-%s", dev_name(&device->dev));
>> +     if (IS_ERR(wmi_bus_dev)) {
>> +             error = PTR_ERR(wmi_bus_dev);
>> +             goto err_remove_handler;
>> +     }
>> +     device->driver_data = wmi_bus_dev;
>> +
>> +     error = parse_wdg(wmi_bus_dev, device);
>>       if (error) {
>>               pr_err("Failed to parse WDG method\n");
>> -             goto err_remove_handler;
>> +             goto err_remove_busdev;
>>       }
>>
>>       return 0;
>>
>> +err_remove_busdev:
>> +     device_unregister(wmi_bus_dev);
>> +     put_device(wmi_bus_dev);
>
> I believe this put_device() call is excessive.  device_create() calls
> device_initialize() and device_add(); device_unregister() calls
> device_del() and put_device(), so it should already take care of
> removing all references to wmi_bus_dev.  Am I missing something?
>

I think you're right, and my acpi_wmi_remove agrees.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux