[PATCH 0/3] char: misc: assign file->private_data to miscdevice on open

[PATCH 0/3] char: misc: assign file->private_data to miscdevice on open

[Martin Kepplinger]

This slightly changes the behaviour of miscdevice drivers at an open()
syscall. Not only if the driver happens to implement an open() fop itself,
but now just always file->private_data points to struct miscdevice, when
a user opens the device file.

This call for review once more: if anybody has an hour to kill:

Look for the only dangerous situation where a miscdevice driver _depends_ on
file->private_data being NULL when _not_ implementing an open() routine
itself. I didn't find anything.

The one section I was unsure about should be ok, see
http://marc.info/?l=linux-kernel&m=141376535132316&w=2

Bonus: one could look for drivers that _do_ implement open()
and do the (in any case) redundant work themselves.

Martin Kepplinger (3):
  misc: always assign miscdevice to file->private_data in open()
  fbdev: pxa3xx-gcu: remove redundant implementation of open()
  char: misc: document behaviour of open()

 drivers/char/misc.c              |    6 ++++--
 drivers/video/fbdev/pxa3xx-gcu.c |   10 ----------
 2 files changed, 4 insertions(+), 12 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

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.

This provides consistent behaviour for miscdevice developers and will
always provide 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>
---
 drivers/char/misc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..205ad4c 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -142,8 +142,8 @@ static int misc_open(struct inode * inode, struct file * file)
 
 	err = 0;
 	replace_fops(file, new_fops);
+	file->private_data = c;
 	if (file->f_op->open) {
-		file->private_data = c;
 		err = file->f_op->open(inode,file);
 	}
 fail:
-- 
1.7.10.4

[PATCH 2/3] fbdev: pxa3xx-gcu: remove redundant implementation of open()

[Martin Kepplinger]

the miscdevice core now does the work in any case.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/video/fbdev/pxa3xx-gcu.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
index 4df3657..7678a94 100644
--- a/drivers/video/fbdev/pxa3xx-gcu.c
+++ b/drivers/video/fbdev/pxa3xx-gcu.c
@@ -373,15 +373,6 @@ static inline struct pxa3xx_gcu_priv *to_pxa3xx_gcu_priv(struct file *file)
 	return container_of(dev, struct pxa3xx_gcu_priv, misc_dev);
 }
 
-/*
- * provide an empty .open callback, so the core sets file->private_data
- * for us.
- */
-static int pxa3xx_gcu_open(struct inode *inode, struct file *file)
-{
-	return 0;
-}
-
 static ssize_t
 pxa3xx_gcu_write(struct file *file, const char *buff,
 		 size_t count, loff_t *offp)
@@ -580,7 +571,6 @@ pxa3xx_gcu_free_buffers(struct device *dev,
 
 static const struct file_operations pxa3xx_gcu_miscdev_fops = {
 	.owner =		THIS_MODULE,
-	.open =			pxa3xx_gcu_open,
 	.write =		pxa3xx_gcu_write,
 	.unlocked_ioctl =	pxa3xx_gcu_ioctl,
 	.mmap =			pxa3xx_gcu_mmap,
-- 
1.7.10.4

[PATCH 3/3] char: misc: document behaviour of open()

[Martin Kepplinger]

an open syscall now assignes file->private_data to a pointer to the
miscdevice structure. This reminds driver developers not to duplicate
code if they need this.

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/char/misc.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 205ad4c..69a08a6 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -169,7 +169,9 @@ static const struct file_operations misc_fops = {
  *	the minor number requested is used.
  *
  *	The structure passed is linked into the kernel and may not be
- *	destroyed until it has been unregistered.
+ *	destroyed until it has been unregistered. By default, an open()
+ *	syscall to the device sets file->private_data to point to the
+ *	structure.
  *
  *	A zero is returned on success and a negative errno code for
  *	failure.
-- 
1.7.10.4

Re: [PATCH] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

Am 2014-10-09 17:50, schrieb Greg KH:
> On Thu, Oct 09, 2014 at 03:10:21PM +0200, Martin Kepplinger wrote:
>> 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.
> 
> There's no documentation for misc devices?  If not, just put it in
> kerneldoc format in the misc .c file.
> 
>> >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.
> 
> No, no one will notice this in the vfs.txt file, and the vfs doesn't
> care about misc devices.
> 
>>>> 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?
>>>

applying said change to misc_open() core and removing open() from
video/fbdev/pxa3xx-gcu.c generates these warnings that I, at the moment,
don't fully understand. Do you konw what happens here?

In file included from arch/x86/vdso/vdso2c.c:161:0:
arch/x86/vdso/vdso2c.c: In function ‘main’:
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does not
occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
In file included from arch/x86/vdso/vdso2c.c:165:0:
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does not
occur when assuming that (X + c) < X is always false [-Wstrict-overflow]

>>
>> 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.
> 
> You just came up with one there, that should be a good start.
> 
> good luck,
> 
> greg k-h
> 

[PATCH 1/3] misc: always assign miscdevice to file->private_data in open()

[Martin Kepplinger]

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.

This provides consistent behaviour for miscdevice developers and will
always provide 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>
---

The mentioned warning is appearently unrelated here, and happens on mainline
v3.17 awell -.- sorry for the confusion.

This applies to 3.17 and is a call for review and opinions. Especially on
the followup patches.


 drivers/char/misc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..205ad4c 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -142,8 +142,8 @@ static int misc_open(struct inode * inode, struct file * file)
 
 	err = 0;
 	replace_fops(file, new_fops);
+	file->private_data = c;
 	if (file->f_op->open) {
-		file->private_data = c;
 		err = file->f_op->open(inode,file);
 	}
 fail:
-- 
1.7.10.4