| Message ID | 17fcf9748e83ede2751a00d8248cb32cbdc610d6.1506571188.git.mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > For WMI operations that are only Set or Query read or write sysfs > attributes created by WMI vendor drivers make sense. > > For other WMI operations that are run on Method, there needs to be a > way to guarantee to userspace that the results from the method call > belong to the data request to the method call. Sysfs attributes don't > work well in this scenario because two userspace processes may be > competing at reading/writing an attribute and step on each other's > data. > > When a WMI vendor driver declares a set of functions in a > file_operations object the WMI bus driver will create a character > device that maps to those file operations. > > That character device will correspond to this path: > /dev/wmi/$driver > > This policy is selected as one driver may map and use multiple > GUIDs and it would be better to only expose a single character > device. > > The WMI vendor drivers will be responsible for managing access to > this character device and proper locking on it. > > When a WMI vendor driver is unloaded the WMI bus driver will clean > up the character device. > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > --- > drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- > include/linux/wmi.h | 1 + > 2 files changed, 94 insertions(+), 5 deletions(-) +Greg, Rafael, Matthew, and Christoph You each provided feedback regarding the method of exposing WMI methods to userspace. This and subsequent patches from Mario lay some of the core groundwork. They implement an implicit whitelist as only drivers requesting the char dev will see it created. https://lkml.org/lkml/2017/9/28/8
On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote: > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > For WMI operations that are only Set or Query read or write sysfs > > attributes created by WMI vendor drivers make sense. > > > > For other WMI operations that are run on Method, there needs to be a > > way to guarantee to userspace that the results from the method call > > belong to the data request to the method call. Sysfs attributes don't > > work well in this scenario because two userspace processes may be > > competing at reading/writing an attribute and step on each other's > > data. > > > > When a WMI vendor driver declares a set of functions in a > > file_operations object the WMI bus driver will create a character > > device that maps to those file operations. > > > > That character device will correspond to this path: > > /dev/wmi/$driver > > > > This policy is selected as one driver may map and use multiple > > GUIDs and it would be better to only expose a single character > > device. > > > > The WMI vendor drivers will be responsible for managing access to > > this character device and proper locking on it. > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > up the character device. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > --- > > drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- > > include/linux/wmi.h | 1 + > > 2 files changed, 94 insertions(+), 5 deletions(-) > > +Greg, Rafael, Matthew, and Christoph > > You each provided feedback regarding the method of exposing WMI methods > to userspace. This and subsequent patches from Mario lay some of the > core groundwork. > > They implement an implicit whitelist as only drivers requesting the char > dev will see it created. > > https://lkml.org/lkml/2017/9/28/8 If you want patchs reviewed, it's best to actually cc: us on the patch itself :(
> -----Original Message----- > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > Sent: Saturday, September 30, 2017 3:12 AM > To: Darren Hart <dvhart@infradead.org> > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>; > platform-driver-x86@vger.kernel.org; Andy Lutomirski <luto@kernel.org>; > quasisec@google.com; pali.rohar@gmail.com; Rafael Wysocki > <rjw@rjwysocki.net>; Matthew Garrett <mjg59@google.com>; Christoph Hellwig > <hch@lst.de> > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when > requested by drivers > > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote: > > > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > > For WMI operations that are only Set or Query read or write sysfs > > > attributes created by WMI vendor drivers make sense. > > > > > > For other WMI operations that are run on Method, there needs to be a > > > way to guarantee to userspace that the results from the method call > > > belong to the data request to the method call. Sysfs attributes don't > > > work well in this scenario because two userspace processes may be > > > competing at reading/writing an attribute and step on each other's > > > data. > > > > > > When a WMI vendor driver declares a set of functions in a > > > file_operations object the WMI bus driver will create a character > > > device that maps to those file operations. > > > > > > That character device will correspond to this path: > > > /dev/wmi/$driver > > > > > > This policy is selected as one driver may map and use multiple > > > GUIDs and it would be better to only expose a single character > > > device. > > > > > > The WMI vendor drivers will be responsible for managing access to > > > this character device and proper locking on it. > > > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > > up the character device. > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > > --- > > > drivers/platform/x86/wmi.c | 98 > +++++++++++++++++++++++++++++++++++++++++++--- > > > include/linux/wmi.h | 1 + > > > 2 files changed, 94 insertions(+), 5 deletions(-) > > > > +Greg, Rafael, Matthew, and Christoph > > > > You each provided feedback regarding the method of exposing WMI methods > > to userspace. This and subsequent patches from Mario lay some of the > > core groundwork. > > > > They implement an implicit whitelist as only drivers requesting the char > > dev will see it created. > > > > https://lkml.org/lkml/2017/9/28/8 > > If you want patchs reviewed, it's best to actually cc: us on the patch > itself :( Greg, I'll make sure to CC you on V4 after I address the other concerns that have been recently raised. I think what's Darren's most interested in is the that conceptually this is an approach you can agree with. "A WMI vendor driver binds to the WMI bus and requests the WMI bus to create a character device. The WMI bus creates a character device /dev/wmi/$driver which the WMI vendor driver will process all various file operations." Thanks,
On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote: > I think what's Darren's most interested in is the that conceptually this is > an approach you can agree with. > > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to create > a character device. The WMI bus creates a character device /dev/wmi/$driver > which the WMI vendor driver will process all various file operations." We've been over this before, let's see that actual implementation which defines the "will process all..." implementation to see if that is actually an acceptable thing. thanks, greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Sunday, October 1, 2017 8:24 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net; > mjg59@google.com; hch@lst.de > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when > requested by drivers > > On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote: > > I think what's Darren's most interested in is the that conceptually this is > > an approach you can agree with. > > > > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to > create > > a character device. The WMI bus creates a character device /dev/wmi/$driver > > which the WMI vendor driver will process all various file operations." > > We've been over this before, let's see that actual implementation which > defines the "will process all..." implementation to see if that is > actually an acceptable thing. > > thanks, > > greg k-h I'm working on changes still (that you'll be CC on), but I don't expect this specific concept to change in the future changes: Bus creating character device: https://patchwork.kernel.org/patch/9975283/ Driver requesting character device: https://patchwork.kernel.org/patch/9975263/
On Sun, Oct 01, 2017 at 02:25:00PM +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Sunday, October 1, 2017 8:24 AM > > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org; > > quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net; > > mjg59@google.com; hch@lst.de > > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when > > requested by drivers > > > > On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote: > > > I think what's Darren's most interested in is the that conceptually this is > > > an approach you can agree with. > > > > > > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to > > create > > > a character device. The WMI bus creates a character device /dev/wmi/$driver > > > which the WMI vendor driver will process all various file operations." > > > > We've been over this before, let's see that actual implementation which > > defines the "will process all..." implementation to see if that is > > actually an acceptable thing. > > > > thanks, > > > > greg k-h > > I'm working on changes still (that you'll be CC on), but I don't expect this specific > concept to change in the future changes: > > Bus creating character device: > https://patchwork.kernel.org/patch/9975283/ A bit hard to comment on web pages, and I would like to point out your obvious memory leak, but it's hard to do that this way :( > Driver requesting character device: > https://patchwork.kernel.org/patch/9975263/ There's other issues with this patch as well, I'll wait for a real one in my inbox to be able to actually review it :( thanks, greg k-h
On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote: > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote: > > > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > > For WMI operations that are only Set or Query read or write sysfs > > > attributes created by WMI vendor drivers make sense. > > > > > > For other WMI operations that are run on Method, there needs to be a > > > way to guarantee to userspace that the results from the method call > > > belong to the data request to the method call. Sysfs attributes don't > > > work well in this scenario because two userspace processes may be > > > competing at reading/writing an attribute and step on each other's > > > data. > > > > > > When a WMI vendor driver declares a set of functions in a > > > file_operations object the WMI bus driver will create a character > > > device that maps to those file operations. > > > > > > That character device will correspond to this path: > > > /dev/wmi/$driver > > > > > > This policy is selected as one driver may map and use multiple > > > GUIDs and it would be better to only expose a single character > > > device. > > > > > > The WMI vendor drivers will be responsible for managing access to > > > this character device and proper locking on it. > > > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > > up the character device. > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > > --- > > > drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- > > > include/linux/wmi.h | 1 + > > > 2 files changed, 94 insertions(+), 5 deletions(-) > > > > +Greg, Rafael, Matthew, and Christoph > > > > You each provided feedback regarding the method of exposing WMI methods > > to userspace. This and subsequent patches from Mario lay some of the > > core groundwork. > > > > They implement an implicit whitelist as only drivers requesting the char > > dev will see it created. > > > > https://lkml.org/lkml/2017/9/28/8 > > If you want patchs reviewed, it's best to actually cc: us on the patch > itself :( > Of course. I didn't send the series, but thought you should see it. I could have asked Mario to resend, but I thought a pointer would have made it easy enough to find in your lkml folder, and it would avoid splitting the conversation which resending would inevitably lead to. I pruned this one because Christoph gets upset if I don't. We can wait for v4 I guess. And next time I want to get your take on something someone doesn't Cc you on, I'll just ask them to resend the whole series with you on Cc.
On Sun, Oct 01, 2017 at 05:57:20PM -0700, Darren Hart wrote: > On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote: > > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote: > > > > > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > > > For WMI operations that are only Set or Query read or write sysfs > > > > attributes created by WMI vendor drivers make sense. > > > > > > > > For other WMI operations that are run on Method, there needs to be a > > > > way to guarantee to userspace that the results from the method call > > > > belong to the data request to the method call. Sysfs attributes don't > > > > work well in this scenario because two userspace processes may be > > > > competing at reading/writing an attribute and step on each other's > > > > data. > > > > > > > > When a WMI vendor driver declares a set of functions in a > > > > file_operations object the WMI bus driver will create a character > > > > device that maps to those file operations. > > > > > > > > That character device will correspond to this path: > > > > /dev/wmi/$driver > > > > > > > > This policy is selected as one driver may map and use multiple > > > > GUIDs and it would be better to only expose a single character > > > > device. > > > > > > > > The WMI vendor drivers will be responsible for managing access to > > > > this character device and proper locking on it. > > > > > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > > > up the character device. > > > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > > > --- > > > > drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- > > > > include/linux/wmi.h | 1 + > > > > 2 files changed, 94 insertions(+), 5 deletions(-) > > > > > > +Greg, Rafael, Matthew, and Christoph > > > > > > You each provided feedback regarding the method of exposing WMI methods > > > to userspace. This and subsequent patches from Mario lay some of the > > > core groundwork. > > > > > > They implement an implicit whitelist as only drivers requesting the char > > > dev will see it created. > > > > > > https://lkml.org/lkml/2017/9/28/8 > > > > If you want patchs reviewed, it's best to actually cc: us on the patch > > itself :( > > > > Of course. I didn't send the series, but thought you should see it. I > could have asked Mario to resend, but I thought a pointer would have > made it easy enough to find in your lkml folder, and it would avoid > splitting the conversation which resending would inevitably lead to. I > pruned this one because Christoph gets upset if I don't. > > We can wait for v4 I guess. And next time I want to get your take on > something someone doesn't Cc you on, I'll just ask them to resend the > whole series with you on Cc. Dude, that's the way it's always been, you know this! Nothing new here, respining a patch series is normal, and asking people to review patch sets that you know already needs to be changed is strange. Just do the fixes, and resend it, that's the simplest, quickest, and easiest thing to do for everyone involved. Asking someone to review a patch based on a url just doesn't work, as how can I point out where the memory leak is exactly? :) Reviewers get thousands of emails a week (or a day in some cases), and making it as easy as possible for them to review the code is the key here. thanks, greg k-h
On Mon, Oct 02, 2017 at 11:24:53AM +0200, Greg Kroah-Hartman wrote: > On Sun, Oct 01, 2017 at 05:57:20PM -0700, Darren Hart wrote: > > On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote: > > > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote: > > > > > > > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > > > > For WMI operations that are only Set or Query read or write sysfs > > > > > attributes created by WMI vendor drivers make sense. > > > > > > > > > > For other WMI operations that are run on Method, there needs to be a > > > > > way to guarantee to userspace that the results from the method call > > > > > belong to the data request to the method call. Sysfs attributes don't > > > > > work well in this scenario because two userspace processes may be > > > > > competing at reading/writing an attribute and step on each other's > > > > > data. > > > > > > > > > > When a WMI vendor driver declares a set of functions in a > > > > > file_operations object the WMI bus driver will create a character > > > > > device that maps to those file operations. > > > > > > > > > > That character device will correspond to this path: > > > > > /dev/wmi/$driver > > > > > > > > > > This policy is selected as one driver may map and use multiple > > > > > GUIDs and it would be better to only expose a single character > > > > > device. > > > > > > > > > > The WMI vendor drivers will be responsible for managing access to > > > > > this character device and proper locking on it. > > > > > > > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > > > > up the character device. > > > > > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > > > > --- > > > > > drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- > > > > > include/linux/wmi.h | 1 + > > > > > 2 files changed, 94 insertions(+), 5 deletions(-) > > > > > > > > +Greg, Rafael, Matthew, and Christoph > > > > > > > > You each provided feedback regarding the method of exposing WMI methods > > > > to userspace. This and subsequent patches from Mario lay some of the > > > > core groundwork. > > > > > > > > They implement an implicit whitelist as only drivers requesting the char > > > > dev will see it created. > > > > > > > > https://lkml.org/lkml/2017/9/28/8 > > > > > > If you want patchs reviewed, it's best to actually cc: us on the patch > > > itself :( > > > > > > > Of course. I didn't send the series, but thought you should see it. I > > could have asked Mario to resend, but I thought a pointer would have > > made it easy enough to find in your lkml folder, and it would avoid > > splitting the conversation which resending would inevitably lead to. I > > pruned this one because Christoph gets upset if I don't. > > > > We can wait for v4 I guess. And next time I want to get your take on > > something someone doesn't Cc you on, I'll just ask them to resend the > > whole series with you on Cc. > > Dude, that's the way it's always been, you know this! Nothing new here, > respining a patch series is normal, and asking people to review patch > sets that you know already needs to be changed is strange. Just do the > fixes, and resend it, that's the simplest, quickest, and easiest thing > to do for everyone involved. Loud and clear Greg, won't happen again. > Asking someone to review a patch based on a url just doesn't work, as > how can I point out where the memory leak is exactly? :) As I said above, I provided the URL to make it easy to see the subjects, senders, dates, etc. from the thread so you could find it in your LKML folder and respond from there, not asking you to review from a webpage. I do this from time to time when someone forgets to Cc me. > Reviewers get thousands of emails a week (or a day in some cases), and > making it as easy as possible for them to review the code is the key > here. I give this same message to people on a regular basis, and usually include some of your load numbers to help drive the scale home. I guess even those of us who know this are still prone to underestimating the scale of the one:many developer:maintainer relationship. Sorry for the lapse.
On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > For WMI operations that are only Set or Query read or write sysfs > attributes created by WMI vendor drivers make sense. > > For other WMI operations that are run on Method, there needs to be a > way to guarantee to userspace that the results from the method call > belong to the data request to the method call. Sysfs attributes don't > work well in this scenario because two userspace processes may be > competing at reading/writing an attribute and step on each other's > data. > > When a WMI vendor driver declares a set of functions in a > file_operations object the WMI bus driver will create a character > device that maps to those file operations. > > That character device will correspond to this path: > /dev/wmi/$driver > > This policy is selected as one driver may map and use multiple > GUIDs and it would be better to only expose a single character > device. > > The WMI vendor drivers will be responsible for managing access to > this character device and proper locking on it. > > When a WMI vendor driver is unloaded the WMI bus driver will clean > up the character device. Ok, thanks to Darren, I've gone and dug these up while my boxes were building stable kernels... Why are you not just using the misc device interface here? Why do you need a whole new major and minor range? Why not just register misc devices dynamically as-needed? Should be much simpler and easier to maintain and reduce your code size a lot. thanks, greg k-h
On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > For WMI operations that are only Set or Query read or write sysfs > attributes created by WMI vendor drivers make sense. > > For other WMI operations that are run on Method, there needs to be a > way to guarantee to userspace that the results from the method call > belong to the data request to the method call. Sysfs attributes don't > work well in this scenario because two userspace processes may be > competing at reading/writing an attribute and step on each other's > data. > > When a WMI vendor driver declares a set of functions in a > file_operations object the WMI bus driver will create a character > device that maps to those file operations. > > That character device will correspond to this path: > /dev/wmi/$driver > > This policy is selected as one driver may map and use multiple > GUIDs and it would be better to only expose a single character > device. > > The WMI vendor drivers will be responsible for managing access to > this character device and proper locking on it. > > When a WMI vendor driver is unloaded the WMI bus driver will clean > up the character device. > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > --- > drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- > include/linux/wmi.h | 1 + > 2 files changed, 94 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 4d73a87c2ddf..17399df87948 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -34,7 +34,9 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include <linux/acpi.h> > +#include <linux/cdev.h> > #include <linux/device.h> > +#include <linux/idr.h> > #include <linux/init.h> > #include <linux/kernel.h> > #include <linux/list.h> > @@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho"); > MODULE_DESCRIPTION("ACPI-WMI Mapping Driver"); > MODULE_LICENSE("GPL"); > > +#define WMI_MAX_DEVS MINORMASK > +static DEFINE_IDR(wmi_idr); You never free the idr's memory when you unload the module :(
> -----Original Message----- > From: Greg KH [mailto:greg@kroah.com] > Sent: Tuesday, October 3, 2017 4:23 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; > Andy Lutomirski <luto@kernel.org>; quasisec@google.com; > pali.rohar@gmail.com > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when > requested by drivers > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > For WMI operations that are only Set or Query read or write sysfs > > attributes created by WMI vendor drivers make sense. > > > > For other WMI operations that are run on Method, there needs to be a > > way to guarantee to userspace that the results from the method call > > belong to the data request to the method call. Sysfs attributes don't > > work well in this scenario because two userspace processes may be > > competing at reading/writing an attribute and step on each other's > > data. > > > > When a WMI vendor driver declares a set of functions in a > > file_operations object the WMI bus driver will create a character > > device that maps to those file operations. > > > > That character device will correspond to this path: > > /dev/wmi/$driver > > > > This policy is selected as one driver may map and use multiple > > GUIDs and it would be better to only expose a single character > > device. > > > > The WMI vendor drivers will be responsible for managing access to > > this character device and proper locking on it. > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > up the character device. > > Ok, thanks to Darren, I've gone and dug these up while my boxes were > building stable kernels... > > Why are you not just using the misc device interface here? Why do you > need a whole new major and minor range? Why not just register misc > devices dynamically as-needed? Should be much simpler and easier to > maintain and reduce your code size a lot. > Thanks for this feedback. I'll look into this as an alternative.
On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote: > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > For WMI operations that are only Set or Query read or write sysfs > > attributes created by WMI vendor drivers make sense. > > > > For other WMI operations that are run on Method, there needs to be a > > way to guarantee to userspace that the results from the method call > > belong to the data request to the method call. Sysfs attributes don't > > work well in this scenario because two userspace processes may be > > competing at reading/writing an attribute and step on each other's > > data. > > > > When a WMI vendor driver declares a set of functions in a > > file_operations object the WMI bus driver will create a character > > device that maps to those file operations. > > > > That character device will correspond to this path: > > /dev/wmi/$driver > > > > This policy is selected as one driver may map and use multiple > > GUIDs and it would be better to only expose a single character > > device. > > > > The WMI vendor drivers will be responsible for managing access to > > this character device and proper locking on it. > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > up the character device. > > Ok, thanks to Darren, I've gone and dug these up while my boxes were > building stable kernels... > > Why are you not just using the misc device interface here? Why do you > need a whole new major and minor range? Why not just register misc > devices dynamically as-needed? Should be much simpler and easier to > maintain and reduce your code size a lot. Thank you Greg, this simplifies things quite a bit. Mario, the misc device interface will remove a lot of the boiler plate setup and eliminate the need to allocate a new major number.
> -----Original Message----- > From: Greg KH [mailto:greg@kroah.com] > Sent: Tuesday, October 3, 2017 4:24 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; > Andy Lutomirski <luto@kernel.org>; quasisec@google.com; > pali.rohar@gmail.com > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when > requested by drivers > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > > For WMI operations that are only Set or Query read or write sysfs > > attributes created by WMI vendor drivers make sense. > > > > For other WMI operations that are run on Method, there needs to be a > > way to guarantee to userspace that the results from the method call > > belong to the data request to the method call. Sysfs attributes don't > > work well in this scenario because two userspace processes may be > > competing at reading/writing an attribute and step on each other's > > data. > > > > When a WMI vendor driver declares a set of functions in a > > file_operations object the WMI bus driver will create a character > > device that maps to those file operations. > > > > That character device will correspond to this path: > > /dev/wmi/$driver > > > > This policy is selected as one driver may map and use multiple > > GUIDs and it would be better to only expose a single character > > device. > > > > The WMI vendor drivers will be responsible for managing access to > > this character device and proper locking on it. > > > > When a WMI vendor driver is unloaded the WMI bus driver will clean > > up the character device. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> > > --- > > drivers/platform/x86/wmi.c | 98 > +++++++++++++++++++++++++++++++++++++++++++--- > > include/linux/wmi.h | 1 + > > 2 files changed, 94 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > > index 4d73a87c2ddf..17399df87948 100644 > > --- a/drivers/platform/x86/wmi.c > > +++ b/drivers/platform/x86/wmi.c > > @@ -34,7 +34,9 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > #include <linux/acpi.h> > > +#include <linux/cdev.h> > > #include <linux/device.h> > > +#include <linux/idr.h> > > #include <linux/init.h> > > #include <linux/kernel.h> > > #include <linux/list.h> > > @@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho"); > > MODULE_DESCRIPTION("ACPI-WMI Mapping Driver"); > > MODULE_LICENSE("GPL"); > > > > +#define WMI_MAX_DEVS MINORMASK > > +static DEFINE_IDR(wmi_idr); > > You never free the idr's memory when you unload the module :( Whoops thanks, will fix. ☹
On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote: >> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: >> > For WMI operations that are only Set or Query read or write sysfs >> > attributes created by WMI vendor drivers make sense. >> > >> > For other WMI operations that are run on Method, there needs to be a >> > way to guarantee to userspace that the results from the method call >> > belong to the data request to the method call. Sysfs attributes don't >> > work well in this scenario because two userspace processes may be >> > competing at reading/writing an attribute and step on each other's >> > data. >> > >> > When a WMI vendor driver declares a set of functions in a >> > file_operations object the WMI bus driver will create a character >> > device that maps to those file operations. >> > >> > That character device will correspond to this path: >> > /dev/wmi/$driver >> > >> > This policy is selected as one driver may map and use multiple >> > GUIDs and it would be better to only expose a single character >> > device. >> > >> > The WMI vendor drivers will be responsible for managing access to >> > this character device and proper locking on it. >> > >> > When a WMI vendor driver is unloaded the WMI bus driver will clean >> > up the character device. >> >> Ok, thanks to Darren, I've gone and dug these up while my boxes were >> building stable kernels... >> >> Why are you not just using the misc device interface here? Why do you >> need a whole new major and minor range? Why not just register misc >> devices dynamically as-needed? Should be much simpler and easier to >> maintain and reduce your code size a lot. > > Thank you Greg, this simplifies things quite a bit. > > Mario, the misc device interface will remove a lot of the boiler plate > setup and eliminate the need to allocate a new major number. > In my mind, the problem with misc is that you may end up forever stuck with a misc device, and they're distinct (visibly to userspace) from all other character devices. If you really want to be fancy, you could try to dust off a non-awful character device API, a la: https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&id=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7
On Tue, Oct 03, 2017 at 09:48:05AM -0700, Andy Lutomirski wrote: > On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote: > >> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > >> > For WMI operations that are only Set or Query read or write sysfs > >> > attributes created by WMI vendor drivers make sense. > >> > > >> > For other WMI operations that are run on Method, there needs to be a > >> > way to guarantee to userspace that the results from the method call > >> > belong to the data request to the method call. Sysfs attributes don't > >> > work well in this scenario because two userspace processes may be > >> > competing at reading/writing an attribute and step on each other's > >> > data. > >> > > >> > When a WMI vendor driver declares a set of functions in a > >> > file_operations object the WMI bus driver will create a character > >> > device that maps to those file operations. > >> > > >> > That character device will correspond to this path: > >> > /dev/wmi/$driver > >> > > >> > This policy is selected as one driver may map and use multiple > >> > GUIDs and it would be better to only expose a single character > >> > device. > >> > > >> > The WMI vendor drivers will be responsible for managing access to > >> > this character device and proper locking on it. > >> > > >> > When a WMI vendor driver is unloaded the WMI bus driver will clean > >> > up the character device. > >> > >> Ok, thanks to Darren, I've gone and dug these up while my boxes were > >> building stable kernels... > >> > >> Why are you not just using the misc device interface here? Why do you > >> need a whole new major and minor range? Why not just register misc > >> devices dynamically as-needed? Should be much simpler and easier to > >> maintain and reduce your code size a lot. > > > > Thank you Greg, this simplifies things quite a bit. > > > > Mario, the misc device interface will remove a lot of the boiler plate > > setup and eliminate the need to allocate a new major number. > > > > In my mind, the problem with misc is that you may end up forever stuck > with a misc device, and they're distinct (visibly to userspace) from > all other character devices. What do you mean by "distinct"? No one cares about the major number for a device node anymore. > If you really want to be fancy, you could try to dust off a non-awful > character device API, a la: > > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&id=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7 Yeah, fixing this up and doing something like this has been on my "TODO" for over a decade now :( thanks, greg k-h
> -----Original Message----- > From: Andy Lutomirski [mailto:luto@kernel.org] > Sent: Tuesday, October 3, 2017 11:48 AM > To: Darren Hart <dvhart@infradead.org> > Cc: Greg KH <greg@kroah.com>; Limonciello, Mario > <Mario_Limonciello@Dell.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>; > Platform Driver <platform-driver-x86@vger.kernel.org>; Andy Lutomirski > <luto@kernel.org>; quasisec@google.com; Pali Rohár <pali.rohar@gmail.com> > Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when > requested by drivers > > On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote: > >> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: > >> > For WMI operations that are only Set or Query read or write sysfs > >> > attributes created by WMI vendor drivers make sense. > >> > > >> > For other WMI operations that are run on Method, there needs to be a > >> > way to guarantee to userspace that the results from the method call > >> > belong to the data request to the method call. Sysfs attributes don't > >> > work well in this scenario because two userspace processes may be > >> > competing at reading/writing an attribute and step on each other's > >> > data. > >> > > >> > When a WMI vendor driver declares a set of functions in a > >> > file_operations object the WMI bus driver will create a character > >> > device that maps to those file operations. > >> > > >> > That character device will correspond to this path: > >> > /dev/wmi/$driver > >> > > >> > This policy is selected as one driver may map and use multiple > >> > GUIDs and it would be better to only expose a single character > >> > device. > >> > > >> > The WMI vendor drivers will be responsible for managing access to > >> > this character device and proper locking on it. > >> > > >> > When a WMI vendor driver is unloaded the WMI bus driver will clean > >> > up the character device. > >> > >> Ok, thanks to Darren, I've gone and dug these up while my boxes were > >> building stable kernels... > >> > >> Why are you not just using the misc device interface here? Why do you > >> need a whole new major and minor range? Why not just register misc > >> devices dynamically as-needed? Should be much simpler and easier to > >> maintain and reduce your code size a lot. > > > > Thank you Greg, this simplifies things quite a bit. > > > > Mario, the misc device interface will remove a lot of the boiler plate > > setup and eliminate the need to allocate a new major number. > > > > In my mind, the problem with misc is that you may end up forever stuck > with a misc device, and they're distinct (visibly to userspace) from > all other character devices. > > If you really want to be fancy, you could try to dust off a non-awful > character device API, a la: > > https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&i > d=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7 That's two years old. What's the history of it? Did you try to get it merged?
I think David Hermann had some review comments. --Andy On Oct 3, 2017, at 11:38 AM, <Mario.Limonciello@dell.com> <Mario.Limonciello@dell.com> wrote: >> -----Original Message----- >> From: Andy Lutomirski [mailto:luto@kernel.org] >> Sent: Tuesday, October 3, 2017 11:48 AM >> To: Darren Hart <dvhart@infradead.org> >> Cc: Greg KH <greg@kroah.com>; Limonciello, Mario >> <Mario_Limonciello@Dell.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>; >> Platform Driver <platform-driver-x86@vger.kernel.org>; Andy Lutomirski >> <luto@kernel.org>; quasisec@google.com; Pali Rohár <pali.rohar@gmail.com> >> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when >> requested by drivers >> >>> On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote: >>>> On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote: >>>>> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote: >>>>> For WMI operations that are only Set or Query read or write sysfs >>>>> attributes created by WMI vendor drivers make sense. >>>>> >>>>> For other WMI operations that are run on Method, there needs to be a >>>>> way to guarantee to userspace that the results from the method call >>>>> belong to the data request to the method call. Sysfs attributes don't >>>>> work well in this scenario because two userspace processes may be >>>>> competing at reading/writing an attribute and step on each other's >>>>> data. >>>>> >>>>> When a WMI vendor driver declares a set of functions in a >>>>> file_operations object the WMI bus driver will create a character >>>>> device that maps to those file operations. >>>>> >>>>> That character device will correspond to this path: >>>>> /dev/wmi/$driver >>>>> >>>>> This policy is selected as one driver may map and use multiple >>>>> GUIDs and it would be better to only expose a single character >>>>> device. >>>>> >>>>> The WMI vendor drivers will be responsible for managing access to >>>>> this character device and proper locking on it. >>>>> >>>>> When a WMI vendor driver is unloaded the WMI bus driver will clean >>>>> up the character device. >>>> >>>> Ok, thanks to Darren, I've gone and dug these up while my boxes were >>>> building stable kernels... >>>> >>>> Why are you not just using the misc device interface here? Why do you >>>> need a whole new major and minor range? Why not just register misc >>>> devices dynamically as-needed? Should be much simpler and easier to >>>> maintain and reduce your code size a lot. >>> >>> Thank you Greg, this simplifies things quite a bit. >>> >>> Mario, the misc device interface will remove a lot of the boiler plate >>> setup and eliminate the need to allocate a new major number. >>> >> >> In my mind, the problem with misc is that you may end up forever stuck >> with a misc device, and they're distinct (visibly to userspace) from >> all other character devices. >> >> If you really want to be fancy, you could try to dust off a non-awful >> character device API, a la: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&i >> d=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7 > > That's two years old. What's the history of it? Did you try to get it merged?
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 4d73a87c2ddf..17399df87948 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -34,7 +34,9 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/acpi.h> +#include <linux/cdev.h> #include <linux/device.h> +#include <linux/idr.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/list.h> @@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho"); MODULE_DESCRIPTION("ACPI-WMI Mapping Driver"); MODULE_LICENSE("GPL"); +#define WMI_MAX_DEVS MINORMASK +static DEFINE_IDR(wmi_idr); +static DEFINE_MUTEX(wmi_minor_lock); static LIST_HEAD(wmi_block_list); struct guid_block { @@ -69,6 +74,8 @@ struct wmi_block { struct wmi_device dev; struct list_head list; struct guid_block gblock; + struct cdev *cdev; + int minor; struct acpi_device *acpi_device; wmi_notify_handler handler; void *handler_data; @@ -86,6 +93,7 @@ struct wmi_block { #define ACPI_WMI_STRING 0x4 /* GUID takes & returns a string */ #define ACPI_WMI_EVENT 0x8 /* GUID is an event */ +static dev_t wmi_devt; static bool debug_event; module_param(debug_event, bool, 0444); MODULE_PARM_DESC(debug_event, @@ -782,21 +790,88 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver) return 0; } +static struct class wmi_bus_class = { + .name = "wmi_bus", +}; + +static int wmi_minor_get(struct wmi_block *wblock) +{ + int ret; + + mutex_lock(&wmi_minor_lock); + ret = idr_alloc(&wmi_idr, wblock, 0, WMI_MAX_DEVS, GFP_KERNEL); + if (ret >= 0) + wblock->minor = ret; + else if (ret == -ENOSPC) + dev_err(&wblock->dev.dev, "too many wmi devices\n"); + + mutex_unlock(&wmi_minor_lock); + return ret; +} + +static void wmi_minor_free(struct wmi_block *wblock) +{ + mutex_lock(&wmi_minor_lock); + idr_remove(&wmi_idr, wblock->minor); + mutex_unlock(&wmi_minor_lock); +} + + static int wmi_dev_probe(struct device *dev) { struct wmi_block *wblock = dev_to_wblock(dev); struct wmi_driver *wdriver = container_of(dev->driver, struct wmi_driver, driver); - int ret = 0; + struct device *clsdev; + int ret = 0, devno; if (ACPI_FAILURE(wmi_method_enable(wblock, 1))) dev_warn(dev, "failed to enable device -- probing anyway\n"); + /* driver wants a character device made */ + if (wdriver->file_operations) { + dev->devt = wmi_devt; + wblock->cdev = cdev_alloc(); + if (!wblock->cdev) { + dev_err(dev, "failed to allocate cdev\n"); + return -ENOMEM; + } + cdev_init(wblock->cdev, wdriver->file_operations); + wblock->cdev->owner = wdriver->file_operations->owner; + ret = wmi_minor_get(wblock); + if (ret < 0) + return ret; + devno = MKDEV(MAJOR(wmi_devt), wblock->minor); + ret = cdev_add(wblock->cdev, devno, 1); + if (ret) { + dev_err(dev, "unable to create device %d:%d\n", + MAJOR(wmi_devt), wblock->minor); + goto err_probe_cdev; + } + clsdev = device_create(&wmi_bus_class, dev, + MKDEV(MAJOR(wmi_devt), wblock->minor), + NULL, "wmi/%s", wdriver->driver.name); + + if (IS_ERR(clsdev)) { + dev_err(dev, "unable to create device %d:%d\n", + MAJOR(wmi_devt), wblock->minor); + ret = PTR_ERR(clsdev); + goto err_probe_class; + } + } + 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; + +err_probe_class: + cdev_del(wblock->cdev); + +err_probe_cdev: + wmi_minor_free(wblock); return ret; } @@ -808,6 +883,13 @@ static int wmi_dev_remove(struct device *dev) container_of(dev->driver, struct wmi_driver, driver); int ret = 0; + if (wdriver->file_operations) { + device_destroy(&wmi_bus_class, + MKDEV(MAJOR(wmi_devt), wblock->minor)); + cdev_del(wblock->cdev); + wmi_minor_free(wblock); + } + if (wdriver->remove) ret = wdriver->remove(dev_to_wdev(dev)); @@ -817,10 +899,6 @@ static int wmi_dev_remove(struct device *dev) return ret; } -static struct class wmi_bus_class = { - .name = "wmi_bus", -}; - static struct bus_type wmi_bus_type = { .name = "wmi", .dev_groups = wmi_groups, @@ -1270,8 +1348,17 @@ static int __init acpi_wmi_init(void) goto err_unreg_bus; } + error = alloc_chrdev_region(&wmi_devt, 0, WMI_MAX_DEVS, "wmi"); + if (error < 0) { + pr_err("unable to allocate char dev region\n"); + goto err_unreg_platform; + } + return 0; +err_unreg_platform: + platform_driver_unregister(&acpi_wmi_driver); + err_unreg_bus: bus_unregister(&wmi_bus_type); @@ -1283,6 +1370,7 @@ static int __init acpi_wmi_init(void) static void __exit acpi_wmi_exit(void) { + unregister_chrdev_region(wmi_devt, WMI_MAX_DEVS); platform_driver_unregister(&acpi_wmi_driver); bus_unregister(&wmi_bus_type); class_unregister(&wmi_bus_class); diff --git a/include/linux/wmi.h b/include/linux/wmi.h index 2cd10c3b89e9..ba5f75a32f4c 100644 --- a/include/linux/wmi.h +++ b/include/linux/wmi.h @@ -47,6 +47,7 @@ struct wmi_device_id { struct wmi_driver { struct device_driver driver; const struct wmi_device_id *id_table; + const struct file_operations *file_operations; int (*probe)(struct wmi_device *wdev); int (*remove)(struct wmi_device *wdev);
For WMI operations that are only Set or Query read or write sysfs attributes created by WMI vendor drivers make sense. For other WMI operations that are run on Method, there needs to be a way to guarantee to userspace that the results from the method call belong to the data request to the method call. Sysfs attributes don't work well in this scenario because two userspace processes may be competing at reading/writing an attribute and step on each other's data. When a WMI vendor driver declares a set of functions in a file_operations object the WMI bus driver will create a character device that maps to those file operations. That character device will correspond to this path: /dev/wmi/$driver This policy is selected as one driver may map and use multiple GUIDs and it would be better to only expose a single character device. The WMI vendor drivers will be responsible for managing access to this character device and proper locking on it. When a WMI vendor driver is unloaded the WMI bus driver will clean up the character device. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++--- include/linux/wmi.h | 1 + 2 files changed, 94 insertions(+), 5 deletions(-)