Messages in this thread Patch in this message | | | Date | Thu, 09 Oct 2014 15:10:21 +0200 | From | Martin Kepplinger <> | Subject | Re: [PATCH] misc: always assign miscdevice to file->private_data in open() |
| |
Am 2014-10-08 15:43, schrieb Greg KH: > On Wed, Oct 08, 2014 at 10:47:54AM +0200, Martin Kepplinger wrote: >> As of now, a miscdevice driver has to provide an implementation of >> the open() file operation if it wants to have misc_open() assign a >> pointer to struct miscdevice to file->private_data for other file >> operations to use (given the user calls open()). >> >> This leads to situations where a miscdevice driver that doesn't need >> internal operations during open() has to implement open() that only >> returns immediately, in order to use the data in private_data in other >> fops. > > Yeah, that's messy, do we have any in-kernel misc drivers that do this? > >> This change provides consistent behaviour for miscdevice developers by >> always providing the pointer in private_data. A driver's open() fop would, >> of course, just overwrite it, when using private_data itself. >> >> Signed-off-by: Martin Kepplinger <martink@posteo.de> >> --- >> This is really only a question: Do I understand this correctly, and, >> could this change then hurt any existing driver? > > I don't know, take a look at the existing ones and see please. > >> As a driver developer it took me a while to figure out what happens here, >> and in my situation it would have been nice to just have this feature as >> part of the miscdevice API. Possibly documented somewhere? > > Patches always accepted for documentation :)
What would be a good place for this? Documentation/driver-model/device.txt or Documentation/filesystem/vfs.txt like so? I'm not sure.
From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001 From: Martin Kepplinger <martink@posteo.de> Date: Thu, 9 Oct 2014 14:54:28 +0200 Subject: [PATCH] documentation: misc_open sets private_data for driver's open()
Signed-off-by: Martin Kepplinger <martink@posteo.de> --- Documentation/filesystems/vfs.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt index 61d65cc..06df9d9 100644 --- a/Documentation/filesystems/vfs.txt +++ b/Documentation/filesystems/vfs.txt @@ -869,7 +869,8 @@ otherwise noted. done the way it is because it makes filesystems simpler to implement. The open() method is a good place to initialize the "private_data" member in the file structure if you want to point - to a device structure + to a device structure. In the case of "struct miscdevice", when + you implement open() this is done automatically.
flush: called by the close(2) system call to flush a file
-- 1.7.10.4
> >> misc_open() is called in any case, on open(). As long as miscdevice drivers >> don't explicitly rely on private_data being NULL exactly IF they don't >> implement an open() fop (which I wouldn't imagine), this would make things >> even more convenient. > > I agree, but it would be great if you can audit the existing misc > drivers to ensure we don't break anything with this change. Can you do > that please? >
I would grep -r "struct miscdevice" ./drivers/; and look at struct file_operations of these results, see how their open() looks like, and where they assign something to private_data.
If you have an idea for a script that lists all relevant files for me, please tell me.
I queue this up but can't tell at all when it actually gets scheduled in ;)
I guess some do this work on their own because they don't know that misc_open() already does it for them. It would probably be too much to check what drivers could then just drop their open(). Interesting though ;) But in the short term, I think the appended documentation would help.
martin
| |