[PATCH] wmi: Set wmi devices' parents

[PATCH] wmi: Set wmi devices' parents

[Andy Lutomirski]

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(-)

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

Re: [PATCH] wmi: Set wmi devices' parents

[Darren Hart, Carlos Corbacho, Len Brown, Greg Kroah-Hartman]

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

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] wmi: Set wmi devices' parents

[Matthew Garrett]

Works for me.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] wmi: Set wmi devices' parents

[Andy Lutomirski]

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

Re: [PATCH] wmi: Set wmi devices' parents

[Rafael J. Wysocki]

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

Re: [PATCH] wmi: Set wmi devices' parents

[Andy Lutomirski]

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

Re: [PATCH] wmi: Set wmi devices' parents

[Rafael J. Wysocki]

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

Re: [PATCH] wmi: Set wmi devices' parents

[Darren Hart]

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.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] wmi: Set wmi devices' parents

[Andy Lutomirski]

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

Re: [PATCH] wmi: Set wmi devices' parents

[Darren Hart]

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.

-- 
Darren Hart
Intel Open Source Technology Center