always assign miscdevice to file->private_data
I've been trying this a few months ago, ( https://lkml.org/lkml/2014/10/8/98 ) in a very bad attempt that (thankfully) failed. I'm happy to see you trying this now and got the change in drivers/fuse merged now. I've been running a kernel with this change for quite some time, it's obviously fine. I also only found the btrfs ioctl miscdevice as a main user that has to change, and the lguest driver (I sent off the lguest patch to check if this is correct.) I append the additional changes I have applied locally (to the fuse change) for you. Feel free to use, test, use my signed-off-by (if you have exactly these changes) or send them off, however you want. Please be careful and double check to have all necessary changes merged before doing the change in misc_open(). Also, try to find drivers that assign or use struct miscdevice themselves. Those can be simplified. And of course drivers that basically do nothing in open(), just to have the link in private_data. Thanks for the work! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
always assign miscdevice to file-private_data
I've been trying this a few months ago, ( https://lkml.org/lkml/2014/10/8/98 ) in a very bad attempt that (thankfully) failed. I'm happy to see you trying this now and got the change in drivers/fuse merged now. I've been running a kernel with this change for quite some time, it's obviously fine. I also only found the btrfs ioctl miscdevice as a main user that has to change, and the lguest driver (I sent off the lguest patch to check if this is correct.) I append the additional changes I have applied locally (to the fuse change) for you. Feel free to use, test, use my signed-off-by (if you have exactly these changes) or send them off, however you want. Please be careful and double check to have all necessary changes merged before doing the change in misc_open(). Also, try to find drivers that assign or use struct miscdevice themselves. Those can be simplified. And of course drivers that basically do nothing in open(), just to have the link in private_data. Thanks for the work! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] misc: always assign miscdevice to file->private_data in open()
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] misc: always assign miscdevice to file-private_data in open()
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 mart...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] misc: always assign miscdevice to file->private_data in open()
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file->private_data in open()
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 --- 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 >> 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 >> --- >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file-private_data in open()
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 mart...@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 mart...@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 mart...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] misc: always assign miscdevice to file-private_data in open()
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 mart...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file->private_data in open()
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? 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. >>> 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 --- 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 >> 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 >> --- >> 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? >>> >> >> 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file-private_data in open()
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? 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. 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 mart...@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 mart...@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 mart...@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? 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file->private_data in open()
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 > >> --- > >> 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 > 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 > --- > 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? > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 >> --- >> 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 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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 mart...@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 mart...@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 mart...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file-private_data in open()
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 mart...@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 mart...@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 mart...@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? 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file->private_data in open()
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 > --- > 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 :) > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] misc: always assign miscdevice to file->private_data in open()
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] misc: always assign miscdevice to file-private_data in open()
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 mart...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] misc: always assign miscdevice to file-private_data in open()
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 mart...@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 :) 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
