Discussion:
[PATCH] misc: always assign miscdevice to file->private_data in open()
Martin Kepplinger
2014-10-08 08:47:54 UTC
Permalink
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 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 <***@posteo.de>
---
This is really only a question: Do I understand this correctly, and,
could this change then hurt any existing driver?
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?

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.

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
Greg KH
2014-10-08 13:43:07 UTC
Permalink
Post by 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.
Yeah, that's messy, do we have any in-kernel misc drivers that do this?
Post by Martin Kepplinger
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.
---
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.
Post by Martin Kepplinger
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 :)
Post by Martin Kepplinger
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?

thanks,

greg k-h
Martin Kepplinger
2014-10-09 13:10:21 UTC
Permalink
Post by Greg KH
Post by 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.
Yeah, that's messy, do we have any in-kernel misc drivers that do this?
Post by Martin Kepplinger
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.
---
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.
Post by Martin Kepplinger
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.
Post by Greg KH
From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
From: Martin Kepplinger <***@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 <***@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
Post by Greg KH
Post by Martin Kepplinger
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
Greg KH
2014-10-09 15:50:20 UTC
Permalink
Post by Martin Kepplinger
Post by Greg KH
Post by 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.
Yeah, that's messy, do we have any in-kernel misc drivers that do this?
Post by Martin Kepplinger
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.
---
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.
Post by Martin Kepplinger
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.
Post by Martin Kepplinger
Post by Greg KH
From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
Date: Thu, 9 Oct 2014 14:54:28 +0200
Subject: [PATCH] documentation: misc_open sets private_data for driver's
open()
---
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.
Post by Martin Kepplinger
Post by Greg KH
Post by Martin Kepplinger
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.
You just came up with one there, that should be a good start.

good luck,

greg k-h
Martin Kepplinger
2014-10-09 16:37:35 UTC
Permalink
This might prevent code duplication in the future.

Signed-off-by: Martin Kepplinger <***@posteo.de>
---
This is a suggestion for a place to put this information. I think this
makes sense but there might be a more appropriate place elsewhere.

drivers/char/misc.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index ffa97d2..6a3d15b 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -171,6 +171,10 @@ static const struct file_operations misc_fops = {
* The structure passed is linked into the kernel and may not be
* destroyed until it has been unregistered.
*
+ * If struct miscdevice's fops contain an implementation of open()
+ * the struct file's private data will be a pointer back to
+ * struct miscdevice.
+ *
* A zero is returned on success and a negative errno code for
* failure.
*/
--
1.7.10.4
Martin Kepplinger
2014-10-16 11:08:43 UTC
Permalink
Post by Greg KH
Post by Martin Kepplinger
Post by Greg KH
Post by 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.
Yeah, that's messy, do we have any in-kernel misc drivers that do this?
yes, at least drivers/video/fbdev/pxa3xx-gcu.c , maybe more and I don't
know if others do the work theirselves.

An audit if changing to always-set-private_data breaks drivers should be
doable in a reasonable timeframe. I don't think there would be a
problem; it'd be good if others take a look aswell though.
Post by Greg KH
Post by Martin Kepplinger
Post by Greg KH
Post by Martin Kepplinger
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.
---
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.
Post by Martin Kepplinger
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.
Post by Martin Kepplinger
Post by Greg KH
From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2001
Date: Thu, 9 Oct 2014 14:54:28 +0200
Subject: [PATCH] documentation: misc_open sets private_data for driver's
open()
---
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.
Post by Martin Kepplinger
Post by Greg KH
Post by Martin Kepplinger
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.
You just came up with one there, that should be a good start.
good luck,
greg k-h
Martin Kepplinger
2014-10-18 23:12:16 UTC
Permalink
Post by Greg KH
Post by Martin Kepplinger
Post by 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 ne=
ed
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
internal operations during open() has to implement open() that onl=
y
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
returns immediately, in order to use the data in private_data in o=
ther
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
fops.
Yeah, that's messy, do we have any in-kernel misc drivers that do t=
his?
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
This change provides consistent behaviour for miscdevice developer=
s by
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
always providing the pointer in private_data. A driver's open() fo=
p would,
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
of course, just overwrite it, when using private_data itself.
---
This is really only a question: Do I understand this correctly, an=
d,
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
could this change then hurt any existing driver?
I don't know, take a look at the existing ones and see please.
Post by Martin Kepplinger
As a driver developer it took me a while to figure out what happen=
s here,
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
and in my situation it would have been nice to just have this feat=
ure as
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
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.
=20
There's no documentation for misc devices? If not, just put it in
kerneldoc format in the misc .c file.
=20
Post by Martin Kepplinger
From facd10cfa7539755e960dec8cc009934200e68ec Mon Sep 17 00:00:00 2=
001
Post by Greg KH
Post by Martin Kepplinger
Date: Thu, 9 Oct 2014 14:54:28 +0200
Subject: [PATCH] documentation: misc_open sets private_data for driv=
er's
Post by Greg KH
Post by Martin Kepplinger
open()
---
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.
=20
No, no one will notice this in the vfs.txt file, and the vfs doesn't
care about misc devices.
=20
Post by Martin Kepplinger
Post by Martin Kepplinger
misc_open() is called in any case, on open(). As long as miscdevic=
e drivers
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
don't explicitly rely on private_data being NULL exactly IF they d=
on't
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
implement an open() fop (which I wouldn't imagine), this would mak=
e things
Post by Greg KH
Post by Martin Kepplinger
Post by Martin Kepplinger
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 yo=
u do
Post by Greg KH
Post by Martin Kepplinger
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 =91main=92:
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does no=
t
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 no=
t
occur when assuming that (X + c) < X is always false [-Wstrict-overflow=
]
Post by Greg KH
Post by Martin Kepplinger
I would grep -r "struct miscdevice" ./drivers/; and look at struct
file_operations of these results, see how their open() looks like, a=
nd
Post by Greg KH
Post by Martin Kepplinger
where they assign something to private_data.
If you have an idea for a script that lists all relevant files for m=
e,
Post by Greg KH
Post by Martin Kepplinger
please tell me.
=20
You just came up with one there, that should be a good start.
=20
good luck,
=20
greg k-h
=20
Martin Kepplinger
2014-10-19 00:30:58 UTC
Permalink
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 <***@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
Martin Kepplinger
2014-10-19 00:31:00 UTC
Permalink
if we depend on private_data being NULL in write() before initialize()
make sure it is NULL after open().

Signed-off-by: Martin Kepplinger <***@posteo.de>
---

I'm not completely sure if this patch is needed and am still investigating.
What do you think? open() could be called by the user I guess. Does
lguest_user.c depend on private_data being NULL on a first write()?


drivers/lguest/lguest_user.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/lguest/lguest_user.c b/drivers/lguest/lguest_user.c
index 4263f4c..30251b7 100644
--- a/drivers/lguest/lguest_user.c
+++ b/drivers/lguest/lguest_user.c
@@ -497,6 +497,12 @@ static int close(struct inode *inode, struct file *file)
return 0;
}

+static int open(struct inode *inode, struct file *file)
+{
+ /* the miscdevice core sets private_data on open() */
+ file->private_data = NULL;
+}
+
/*L:000
* Welcome to our journey through the Launcher!
*
@@ -514,6 +520,7 @@ static int close(struct inode *inode, struct file *file)
*/
static const struct file_operations lguest_fops = {
.owner = THIS_MODULE,
+ .open = open,
.release = close,
.write = write,
.read = read,
--
1.7.10.4
Martin Kepplinger
2014-10-20 13:41:41 UTC
Permalink
Post by Martin Kepplinger
if we depend on private_data being NULL in write() before initialize()
make sure it is NULL after open().
---
I'm not completely sure if this patch is needed and am still investigating.
What do you think? open() could be called by the user I guess. Does
lguest_user.c depend on private_data being NULL on a first write()?
Could it be that this patch is not needed indeed or did I ask not clear
Post by Martin Kepplinger
hi
Just a question for understanding: open() is not implemented in
lguest_user.c's miscdevice. The miscdevice core, in this case, does
_not_ set file->private_data on a user's open() call. Is open() called
by the user here? and do you here _depend_ on file->private_data being
NULL after open()? (could you even?)
Would the following force to NULL be necessary if the miscdevice core
_would_ set private_data?
Hi Martin!
Yes, the private_data is NULL on a new file. See
get_empty_filp in fs/file_table.c, which does kmem_cache_zalloc
(zeroing all the contents).
So this isn't necessary here.
Thanks!
Rusty.
Martin Kepplinger
2014-10-19 00:30:59 UTC
Permalink
the miscdevice core now does the work in any case.

Signed-off-by: Martin Kepplinger <***@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
Loading...