| Message ID | 7c58f0a208e0c903008989aec457230e5e58608e.1448306649.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: > Without this patch, wmi devices are in /sys/virtual/wmi. They're > logically children of the ACPI WMI device, so slot them into the > device hierarchy. With this change, on my laptop, they end up in > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> I'd like to hear from some of the main contributors to this driver: Matthew? Carlos? Len? Any cocnerns on this change? My initial concern is about changign how we expose this to userspace, but I believe where it appears in the /sys/devices FS is NOT part of the kernel-userspace interface commitment (per sysfs-rules.txt). Greg, is that correct? > --- > > The WMI driver doesn't really play well with the driver model. This > helps a little. > > Depending on how much time I find, I may send some followups to turn > it into a real bus driver since it really does work like a bus. > > drivers/platform/x86/wmi.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index eb391a281833..e58768787677 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -684,9 +684,11 @@ static struct class wmi_class = { > }; > > static int wmi_create_device(const struct guid_block *gblock, > - struct wmi_block *wblock, acpi_handle handle) > + struct wmi_block *wblock, > + struct acpi_device *device) > { > wblock->dev.class = &wmi_class; > + wblock->dev.parent = &device->dev; > > dev_set_name(&wblock->dev, "%pUL", gblock->guid); > > @@ -723,7 +725,7 @@ static bool guid_already_parsed(const char *guid_string) > /* > * Parse the _WDG method for the GUID data blocks > */ > -static int parse_wdg(acpi_handle handle) > +static int parse_wdg(struct acpi_device *device) > { > struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL}; > union acpi_object *obj; > @@ -733,7 +735,7 @@ static int parse_wdg(acpi_handle handle) > int retval; > u32 i, total; > > - status = acpi_evaluate_object(handle, "_WDG", NULL, &out); > + status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out); > if (ACPI_FAILURE(status)) > return -ENXIO; > > @@ -757,7 +759,7 @@ static int parse_wdg(acpi_handle handle) > if (!wblock) > return -ENOMEM; > > - wblock->handle = handle; > + wblock->handle = device->handle; > wblock->gblock = gblock[i]; > > /* > @@ -767,7 +769,7 @@ static int parse_wdg(acpi_handle handle) > for device creation. > */ > if (!guid_already_parsed(gblock[i].guid)) { > - retval = wmi_create_device(&gblock[i], wblock, handle); > + retval = wmi_create_device(&gblock[i], wblock, device); > if (retval) { > wmi_free_devices(); > goto out_free_pointer; > @@ -884,7 +886,7 @@ static int acpi_wmi_add(struct acpi_device *device) > return -ENODEV; > } > > - error = parse_wdg(device->handle); > + error = parse_wdg(device); > if (error) { > acpi_remove_address_space_handler(device->handle, > ACPI_ADR_SPACE_EC, > -- > 2.5.0 > >
Works for me.
On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: >> Without this patch, wmi devices are in /sys/virtual/wmi. They're >> logically children of the ACPI WMI device, so slot them into the >> device hierarchy. With this change, on my laptop, they end up in >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > I'd like to hear from some of the main contributors to this driver: > > Matthew? > Carlos? > Len? > > Any cocnerns on this change? > > My initial concern is about changign how we expose this to userspace, but I > believe where it appears in the /sys/devices FS is NOT part of the > kernel-userspace interface commitment (per sysfs-rules.txt). Let's drop this, actually. I have mostly-working patches to make wmi into an actual bus driver, and this intermediate step seems like it'll just confuse people. Question, though: where do the WMI devices belong? Multiple choice: /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/[GUID] /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi/[GUID] /sys/devices/platform/PNP0C14:01/[GUID] /sys/devices/platform/PNP0C14:01/wmi/[GUID] Currently I've implemented the first one because it's the smallest diff. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, November 25, 2015 05:28:54 PM Andy Lutomirski wrote: > On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: > >> Without this patch, wmi devices are in /sys/virtual/wmi. They're > >> logically children of the ACPI WMI device, so slot them into the > >> device hierarchy. With this change, on my laptop, they end up in > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. > >> > >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > > > I'd like to hear from some of the main contributors to this driver: > > > > Matthew? > > Carlos? > > Len? > > > > Any cocnerns on this change? > > > > My initial concern is about changign how we expose this to userspace, but I > > believe where it appears in the /sys/devices FS is NOT part of the > > kernel-userspace interface commitment (per sysfs-rules.txt). > > > Let's drop this, actually. I have mostly-working patches to make wmi > into an actual bus driver, and this intermediate step seems like it'll > just confuse people. > > Question, though: where do the WMI devices belong? Multiple choice: > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/[GUID] > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi/[GUID] > > /sys/devices/platform/PNP0C14:01/[GUID] > > /sys/devices/platform/PNP0C14:01/wmi/[GUID] > > Currently I've implemented the first one because it's the smallest diff. That probably is not the right choice, though. ACPI "devices" are counterparts of DT device nodes and having other things exported under them would be quite confusing. In fact, you can argue that the whole /sys/devices/LNXSYSTM:00/ directory should be located under /sys/firmware/acpi, but it turns out to be difficult to move it there for various reasons. Personally, I'd go for the last one. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 26, 2015 at 6:09 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, November 25, 2015 05:28:54 PM Andy Lutomirski wrote: >> On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: >> > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: >> >> Without this patch, wmi devices are in /sys/virtual/wmi. They're >> >> logically children of the ACPI WMI device, so slot them into the >> >> device hierarchy. With this change, on my laptop, they end up in >> >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and >> >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. >> >> >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> > >> > I'd like to hear from some of the main contributors to this driver: >> > >> > Matthew? >> > Carlos? >> > Len? >> > >> > Any cocnerns on this change? >> > >> > My initial concern is about changign how we expose this to userspace, but I >> > believe where it appears in the /sys/devices FS is NOT part of the >> > kernel-userspace interface commitment (per sysfs-rules.txt). >> >> >> Let's drop this, actually. I have mostly-working patches to make wmi >> into an actual bus driver, and this intermediate step seems like it'll >> just confuse people. >> >> Question, though: where do the WMI devices belong? Multiple choice: >> >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/[GUID] >> >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi/[GUID] >> >> /sys/devices/platform/PNP0C14:01/[GUID] >> >> /sys/devices/platform/PNP0C14:01/wmi/[GUID] >> >> Currently I've implemented the first one because it's the smallest diff. > > That probably is not the right choice, though. > > ACPI "devices" are counterparts of DT device nodes and having other things > exported under them would be quite confusing. In fact, you can argue that > the whole /sys/devices/LNXSYSTM:00/ directory should be located under > /sys/firmware/acpi, but it turns out to be difficult to move it there > for various reasons. > > Personally, I'd go for the last one. Mechanically, how do I do that? Should I register as just a platform driver and use acpi_device_install_notify_handler to get the ACPI notifications, or should I register as *both* a platform driver and ACPI driver? --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 26, 2015 07:53:20 AM Andy Lutomirski wrote: > On Thu, Nov 26, 2015 at 6:09 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, November 25, 2015 05:28:54 PM Andy Lutomirski wrote: > >> On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: > >> > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: > >> >> Without this patch, wmi devices are in /sys/virtual/wmi. They're > >> >> logically children of the ACPI WMI device, so slot them into the > >> >> device hierarchy. With this change, on my laptop, they end up in > >> >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and > >> >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. > >> >> > >> >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > >> > > >> > I'd like to hear from some of the main contributors to this driver: > >> > > >> > Matthew? > >> > Carlos? > >> > Len? > >> > > >> > Any cocnerns on this change? > >> > > >> > My initial concern is about changign how we expose this to userspace, but I > >> > believe where it appears in the /sys/devices FS is NOT part of the > >> > kernel-userspace interface commitment (per sysfs-rules.txt). > >> > >> > >> Let's drop this, actually. I have mostly-working patches to make wmi > >> into an actual bus driver, and this intermediate step seems like it'll > >> just confuse people. > >> > >> Question, though: where do the WMI devices belong? Multiple choice: > >> > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/[GUID] > >> > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi/[GUID] > >> > >> /sys/devices/platform/PNP0C14:01/[GUID] > >> > >> /sys/devices/platform/PNP0C14:01/wmi/[GUID] > >> > >> Currently I've implemented the first one because it's the smallest diff. > > > > That probably is not the right choice, though. > > > > ACPI "devices" are counterparts of DT device nodes and having other things > > exported under them would be quite confusing. In fact, you can argue that > > the whole /sys/devices/LNXSYSTM:00/ directory should be located under > > /sys/firmware/acpi, but it turns out to be difficult to move it there > > for various reasons. > > > > Personally, I'd go for the last one. > > Mechanically, how do I do that? Should I register as just a platform > driver and use acpi_device_install_notify_handler to get the ACPI > notifications, or should I register as *both* a platform driver and > ACPI driver? If you have a platform device for this thing, registering an ACPI driver for it is pretty much as valid as registering a driver for a DT device node. So yes, a platform driver is a way to go here IMO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 26, 2015 at 03:09:29PM +0100, Rafael Wysocki wrote: > On Wednesday, November 25, 2015 05:28:54 PM Andy Lutomirski wrote: > > On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: > > > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: > > >> Without this patch, wmi devices are in /sys/virtual/wmi. They're > > >> logically children of the ACPI WMI device, so slot them into the > > >> device hierarchy. With this change, on my laptop, they end up in > > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and > > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. > > >> > > >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > > > > > I'd like to hear from some of the main contributors to this driver: > > > > > > Matthew? > > > Carlos? > > > Len? > > > > > > Any cocnerns on this change? > > > > > > My initial concern is about changign how we expose this to userspace, but I > > > believe where it appears in the /sys/devices FS is NOT part of the > > > kernel-userspace interface commitment (per sysfs-rules.txt). > > > > > > Let's drop this, actually. I have mostly-working patches to make wmi > > into an actual bus driver, and this intermediate step seems like it'll > > just confuse people. > > > > Question, though: where do the WMI devices belong? Multiple choice: > > > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/[GUID] > > > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi/[GUID] > > > > /sys/devices/platform/PNP0C14:01/[GUID] > > > > /sys/devices/platform/PNP0C14:01/wmi/[GUID] > > > > Currently I've implemented the first one because it's the smallest diff. > > That probably is not the right choice, though. > > ACPI "devices" are counterparts of DT device nodes and having other things > exported under them would be quite confusing. In fact, you can argue that > the whole /sys/devices/LNXSYSTM:00/ directory should be located under > /sys/firmware/acpi, but it turns out to be difficult to move it there > for various reasons. > > Personally, I'd go for the last one. That looks like a logical place to me.
On Mon, Nov 30, 2015 at 10:51 AM, Darren Hart <dvhart@infradead.org> wrote: > On Thu, Nov 26, 2015 at 03:09:29PM +0100, Rafael Wysocki wrote: >> On Wednesday, November 25, 2015 05:28:54 PM Andy Lutomirski wrote: >> > On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: >> > > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: >> > >> Without this patch, wmi devices are in /sys/virtual/wmi. They're >> > >> logically children of the ACPI WMI device, so slot them into the >> > >> device hierarchy. With this change, on my laptop, they end up in >> > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and >> > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. >> > >> >> > >> Signed-off-by: Andy Lutomirski <luto@kernel.org> >> > > >> > > I'd like to hear from some of the main contributors to this driver: >> > > >> > > Matthew? >> > > Carlos? >> > > Len? >> > > >> > > Any cocnerns on this change? >> > > >> > > My initial concern is about changign how we expose this to userspace, but I >> > > believe where it appears in the /sys/devices FS is NOT part of the >> > > kernel-userspace interface commitment (per sysfs-rules.txt). >> > >> > >> > Let's drop this, actually. I have mostly-working patches to make wmi >> > into an actual bus driver, and this intermediate step seems like it'll >> > just confuse people. >> > >> > Question, though: where do the WMI devices belong? Multiple choice: >> > >> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/[GUID] >> > >> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi/[GUID] >> > >> > /sys/devices/platform/PNP0C14:01/[GUID] >> > >> > /sys/devices/platform/PNP0C14:01/wmi/[GUID] >> > >> > Currently I've implemented the first one because it's the smallest diff. >> >> That probably is not the right choice, though. >> >> ACPI "devices" are counterparts of DT device nodes and having other things >> exported under them would be quite confusing. In fact, you can argue that >> the whole /sys/devices/LNXSYSTM:00/ directory should be located under >> /sys/firmware/acpi, but it turns out to be difficult to move it there >> for various reasons. >> >> Personally, I'd go for the last one. > > That looks like a logical place to me. I have a monstrous patch set queued up for this. I still need to work out some kinks, but I hope to have it ready to send in the next couple days. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 25, 2015 at 05:28:54PM -0800, Andy Lutomirski wrote: > On Mon, Nov 23, 2015 at 11:37 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Mon, Nov 23, 2015 at 11:25:30AM -0800, Andy Lutomirski wrote: > >> Without this patch, wmi devices are in /sys/virtual/wmi. They're > >> logically children of the ACPI WMI device, so slot them into the > >> device hierarchy. With this change, on my laptop, they end up in > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and > >> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. > >> > >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > > > I'd like to hear from some of the main contributors to this driver: > > > > Matthew? > > Carlos? > > Len? > > > > Any cocnerns on this change? > > > > My initial concern is about changign how we expose this to userspace, but I > > believe where it appears in the /sys/devices FS is NOT part of the > > kernel-userspace interface commitment (per sysfs-rules.txt). > > > Let's drop this, actually. I have mostly-working patches to make wmi > into an actual bus driver, and this intermediate step seems like it'll > just confuse people. Dropped.
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index eb391a281833..e58768787677 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -684,9 +684,11 @@ static struct class wmi_class = { }; static int wmi_create_device(const struct guid_block *gblock, - struct wmi_block *wblock, acpi_handle handle) + struct wmi_block *wblock, + struct acpi_device *device) { wblock->dev.class = &wmi_class; + wblock->dev.parent = &device->dev; dev_set_name(&wblock->dev, "%pUL", gblock->guid); @@ -723,7 +725,7 @@ static bool guid_already_parsed(const char *guid_string) /* * Parse the _WDG method for the GUID data blocks */ -static int parse_wdg(acpi_handle handle) +static int parse_wdg(struct acpi_device *device) { struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL}; union acpi_object *obj; @@ -733,7 +735,7 @@ static int parse_wdg(acpi_handle handle) int retval; u32 i, total; - status = acpi_evaluate_object(handle, "_WDG", NULL, &out); + status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out); if (ACPI_FAILURE(status)) return -ENXIO; @@ -757,7 +759,7 @@ static int parse_wdg(acpi_handle handle) if (!wblock) return -ENOMEM; - wblock->handle = handle; + wblock->handle = device->handle; wblock->gblock = gblock[i]; /* @@ -767,7 +769,7 @@ static int parse_wdg(acpi_handle handle) for device creation. */ if (!guid_already_parsed(gblock[i].guid)) { - retval = wmi_create_device(&gblock[i], wblock, handle); + retval = wmi_create_device(&gblock[i], wblock, device); if (retval) { wmi_free_devices(); goto out_free_pointer; @@ -884,7 +886,7 @@ static int acpi_wmi_add(struct acpi_device *device) return -ENODEV; } - error = parse_wdg(device->handle); + error = parse_wdg(device); if (error) { acpi_remove_address_space_handler(device->handle, ACPI_ADR_SPACE_EC,
Without this patch, wmi devices are in /sys/virtual/wmi. They're logically children of the ACPI WMI device, so slot them into the device hierarchy. With this change, on my laptop, they end up in /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:00/wmi and /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C14:01/wmi. Signed-off-by: Andy Lutomirski <luto@kernel.org> --- The WMI driver doesn't really play well with the driver model. This helps a little. Depending on how much time I find, I may send some followups to turn it into a real bus driver since it really does work like a bus. drivers/platform/x86/wmi.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)