[PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Michael Ellerman]

Currently it's not easy to share file_operations between multiple
instances of a miscdevice. In order to do this, the device code needs to
store a list of all it's miscdevice instances, and when fops->open() is
called, search the list and find the right device based on the minor
number.

However the generic miscdevice code already has a list of miscdevices,
and uses this to find the right device in misc_open(). If misc_open()
would store the miscdevice it found in file->private_data, then the
device code wouldn't need to worry about storing it's own separate list
and searching that as well.

The rest of the miscdevice code does not use file->private_data, so the
device code is still free to use file->private_data for something else
if it wants to.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

---
I'm not all that across VFS or driver core stuff, but this looks safe to
me, lots of other devices use file->private_data and this should be no
different AFAICT.


 drivers/char/misc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index a5e0db9..bb91d68 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -147,11 +147,13 @@ static int misc_open(struct inode * inode, struct file * file)
 	err = 0;
 	old_fops = file->f_op;
 	file->f_op = new_fops;
+	file->private_data = c;
 	if (file->f_op->open) {
 		err=file->f_op->open(inode,file);
 		if (err) {
 			fops_put(file->f_op);
 			file->f_op = fops_get(old_fops);
+			file->private_data = NULL;
 		}
 	}
 	fops_put(old_fops);
-- 
1.5.5

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Greg KH]

On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> Currently it's not easy to share file_operations between multiple
> instances of a miscdevice. In order to do this, the device code needs to
> store a list of all it's miscdevice instances, and when fops->open() is
> called, search the list and find the right device based on the minor
> number.
> 
> However the generic miscdevice code already has a list of miscdevices,
> and uses this to find the right device in misc_open(). If misc_open()
> would store the miscdevice it found in file->private_data, then the
> device code wouldn't need to worry about storing it's own separate list
> and searching that as well.
> 
> The rest of the miscdevice code does not use file->private_data, so the
> device code is still free to use file->private_data for something else
> if it wants to.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

Do you have a follow-on patch for some misc device using code that would
take advantage of this change?

thanks,

greg k-h

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
> On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> > Currently it's not easy to share file_operations between multiple
> > instances of a miscdevice. In order to do this, the device code needs to
> > store a list of all it's miscdevice instances, and when fops->open() is
> > called, search the list and find the right device based on the minor
> > number.
> > 
> > However the generic miscdevice code already has a list of miscdevices,
> > and uses this to find the right device in misc_open(). If misc_open()
> > would store the miscdevice it found in file->private_data, then the
> > device code wouldn't need to worry about storing it's own separate list
> > and searching that as well.
> > 
> > The rest of the miscdevice code does not use file->private_data, so the
> > device code is still free to use file->private_data for something else
> > if it wants to.
> > 
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> 
> Do you have a follow-on patch for some misc device using code that would
> take advantage of this change?

Ah, good point. I do, but not for upstream :/

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Greg KH]

On Fri, Nov 14, 2008 at 10:54:41AM +1100, Michael Ellerman wrote:
> On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
> > On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> > > Currently it's not easy to share file_operations between multiple
> > > instances of a miscdevice. In order to do this, the device code needs to
> > > store a list of all it's miscdevice instances, and when fops->open() is
> > > called, search the list and find the right device based on the minor
> > > number.
> > > 
> > > However the generic miscdevice code already has a list of miscdevices,
> > > and uses this to find the right device in misc_open(). If misc_open()
> > > would store the miscdevice it found in file->private_data, then the
> > > device code wouldn't need to worry about storing it's own separate list
> > > and searching that as well.
> > > 
> > > The rest of the miscdevice code does not use file->private_data, so the
> > > device code is still free to use file->private_data for something else
> > > if it wants to.
> > > 
> > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > 
> > Do you have a follow-on patch for some misc device using code that would
> > take advantage of this change?
> 
> Ah, good point. I do, but not for upstream :/

Hm, then I have to ask why should we take this change?

And why would the driver not be availble for upstream to take?

thanks,

greg k-h

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

On Thu, 2008-11-13 at 16:18 -0800, Greg KH wrote:
> On Fri, Nov 14, 2008 at 10:54:41AM +1100, Michael Ellerman wrote:
> > On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
> > > On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> > > > Currently it's not easy to share file_operations between multiple
> > > > instances of a miscdevice. In order to do this, the device code needs to
> > > > store a list of all it's miscdevice instances, and when fops->open() is
> > > > called, search the list and find the right device based on the minor
> > > > number.
> > > > 
> > > > However the generic miscdevice code already has a list of miscdevices,
> > > > and uses this to find the right device in misc_open(). If misc_open()
> > > > would store the miscdevice it found in file->private_data, then the
> > > > device code wouldn't need to worry about storing it's own separate list
> > > > and searching that as well.
> > > > 
> > > > The rest of the miscdevice code does not use file->private_data, so the
> > > > device code is still free to use file->private_data for something else
> > > > if it wants to.
> > > > 
> > > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > > 
> > > Do you have a follow-on patch for some misc device using code that would
> > > take advantage of this change?
> > 
> > Ah, good point. I do, but not for upstream :/
> 
> Hm, then I have to ask why should we take this change?

Because it's seems like a good idea.

If that's your only objection I'll find some in-tree drivers that can
use it, drivers/char/tpm/tpm.c looks like it could use it for starters.
It's a bit hard to tell but there might be some others.

> And why would the driver not be availble for upstream to take?

Because it's a hacky pile of crud, and it's for unreleased and
non-existent hardware.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Greg KH]

On Fri, Nov 14, 2008 at 01:50:52PM +1100, Michael Ellerman wrote:
> On Thu, 2008-11-13 at 16:18 -0800, Greg KH wrote:
> > On Fri, Nov 14, 2008 at 10:54:41AM +1100, Michael Ellerman wrote:
> > > On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
> > > > On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> > > > > Currently it's not easy to share file_operations between multiple
> > > > > instances of a miscdevice. In order to do this, the device code needs to
> > > > > store a list of all it's miscdevice instances, and when fops->open() is
> > > > > called, search the list and find the right device based on the minor
> > > > > number.
> > > > > 
> > > > > However the generic miscdevice code already has a list of miscdevices,
> > > > > and uses this to find the right device in misc_open(). If misc_open()
> > > > > would store the miscdevice it found in file->private_data, then the
> > > > > device code wouldn't need to worry about storing it's own separate list
> > > > > and searching that as well.
> > > > > 
> > > > > The rest of the miscdevice code does not use file->private_data, so the
> > > > > device code is still free to use file->private_data for something else
> > > > > if it wants to.
> > > > > 
> > > > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > > > 
> > > > Do you have a follow-on patch for some misc device using code that would
> > > > take advantage of this change?
> > > 
> > > Ah, good point. I do, but not for upstream :/
> > 
> > Hm, then I have to ask why should we take this change?
> 
> Because it's seems like a good idea.

You know we don't make changes to core code for drivers that aren't in
the main tree, this is not a new thing...

> > And why would the driver not be availble for upstream to take?
> 
> Because it's a hacky pile of crud, and it's for unreleased and
> non-existent hardware.

That's what the drivers/staging/ tree is for, send it on over to me and
I'll add it to that location.

thanks,

greg k-h

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Randy Dunlap]

Greg KH wrote:
> On Fri, Nov 14, 2008 at 01:50:52PM +1100, Michael Ellerman wrote:
>> On Thu, 2008-11-13 at 16:18 -0800, Greg KH wrote:
>>> On Fri, Nov 14, 2008 at 10:54:41AM +1100, Michael Ellerman wrote:
>>>> On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
>>>>> On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
>>>>>> Currently it's not easy to share file_operations between multiple
>>>>>> instances of a miscdevice. In order to do this, the device code needs to
>>>>>> store a list of all it's miscdevice instances, and when fops->open() is
>>>>>> called, search the list and find the right device based on the minor
>>>>>> number.
>>>>>>
>>>>>> However the generic miscdevice code already has a list of miscdevices,
>>>>>> and uses this to find the right device in misc_open(). If misc_open()
>>>>>> would store the miscdevice it found in file->private_data, then the
>>>>>> device code wouldn't need to worry about storing it's own separate list
>>>>>> and searching that as well.
>>>>>>
>>>>>> The rest of the miscdevice code does not use file->private_data, so the
>>>>>> device code is still free to use file->private_data for something else
>>>>>> if it wants to.
>>>>>>
>>>>>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
>>>>> Do you have a follow-on patch for some misc device using code that would
>>>>> take advantage of this change?
>>>> Ah, good point. I do, but not for upstream :/
>>> Hm, then I have to ask why should we take this change?
>> Because it's seems like a good idea.
> 
> You know we don't make changes to core code for drivers that aren't in
> the main tree, this is not a new thing...
> 
>>> And why would the driver not be availble for upstream to take?
>> Because it's a hacky pile of crud, and it's for unreleased and
>> non-existent hardware.
> 
> That's what the drivers/staging/ tree is for, send it on over to me and
> I'll add it to that location.

for non-existent hardware??  that's not a good plan.


-- 
~Randy

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Greg KH]

On Thu, Nov 13, 2008 at 08:01:47PM -0800, Randy Dunlap wrote:
> Greg KH wrote:
> > On Fri, Nov 14, 2008 at 01:50:52PM +1100, Michael Ellerman wrote:
> >> On Thu, 2008-11-13 at 16:18 -0800, Greg KH wrote:
> >>> On Fri, Nov 14, 2008 at 10:54:41AM +1100, Michael Ellerman wrote:
> >>>> On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
> >>>>> On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> >>>>>> Currently it's not easy to share file_operations between multiple
> >>>>>> instances of a miscdevice. In order to do this, the device code needs to
> >>>>>> store a list of all it's miscdevice instances, and when fops->open() is
> >>>>>> called, search the list and find the right device based on the minor
> >>>>>> number.
> >>>>>>
> >>>>>> However the generic miscdevice code already has a list of miscdevices,
> >>>>>> and uses this to find the right device in misc_open(). If misc_open()
> >>>>>> would store the miscdevice it found in file->private_data, then the
> >>>>>> device code wouldn't need to worry about storing it's own separate list
> >>>>>> and searching that as well.
> >>>>>>
> >>>>>> The rest of the miscdevice code does not use file->private_data, so the
> >>>>>> device code is still free to use file->private_data for something else
> >>>>>> if it wants to.
> >>>>>>
> >>>>>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> >>>>> Do you have a follow-on patch for some misc device using code that would
> >>>>> take advantage of this change?
> >>>> Ah, good point. I do, but not for upstream :/
> >>> Hm, then I have to ask why should we take this change?
> >> Because it's seems like a good idea.
> > 
> > You know we don't make changes to core code for drivers that aren't in
> > the main tree, this is not a new thing...
> > 
> >>> And why would the driver not be availble for upstream to take?
> >> Because it's a hacky pile of crud, and it's for unreleased and
> >> non-existent hardware.
> > 
> > That's what the drivers/staging/ tree is for, send it on over to me and
> > I'll add it to that location.
> 
> for non-existent hardware??  that's not a good plan.

I read that as "not public yet" hardware.  If that is incorrect, Randy
is right, this isn't a good idea at all, sorry.

thanks,

greg k-h

Re: [PATCH] Store the relevant miscdevice in file->private_data in misc_open()

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]

On Thu, 2008-11-13 at 19:23 -0800, Greg KH wrote:
> On Fri, Nov 14, 2008 at 01:50:52PM +1100, Michael Ellerman wrote:
> > On Thu, 2008-11-13 at 16:18 -0800, Greg KH wrote:
> > > On Fri, Nov 14, 2008 at 10:54:41AM +1100, Michael Ellerman wrote:
> > > > On Thu, 2008-11-13 at 09:31 -0800, Greg KH wrote: 
> > > > > On Thu, Nov 13, 2008 at 03:49:50PM +1100, Michael Ellerman wrote:
> > > > > > Currently it's not easy to share file_operations between multiple
> > > > > > instances of a miscdevice. In order to do this, the device code needs to
> > > > > > store a list of all it's miscdevice instances, and when fops->open() is
> > > > > > called, search the list and find the right device based on the minor
> > > > > > number.
> > > > > > 
> > > > > > However the generic miscdevice code already has a list of miscdevices,
> > > > > > and uses this to find the right device in misc_open(). If misc_open()
> > > > > > would store the miscdevice it found in file->private_data, then the
> > > > > > device code wouldn't need to worry about storing it's own separate list
> > > > > > and searching that as well.
> > > > > > 
> > > > > > The rest of the miscdevice code does not use file->private_data, so the
> > > > > > device code is still free to use file->private_data for something else
> > > > > > if it wants to.
> > > > > > 
> > > > > > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > > > > 
> > > > > Do you have a follow-on patch for some misc device using code that would
> > > > > take advantage of this change?
> > > > 
> > > > Ah, good point. I do, but not for upstream :/
> > > 
> > > Hm, then I have to ask why should we take this change?
> > 
> > Because it's seems like a good idea.
> 
> You know we don't make changes to core code for drivers that aren't in
> the main tree, this is not a new thing...

Sure. I'm not asking you to.

It seemed clear to me that this was a sensible thing for the misc device
infrastructure to do, but if you disagree that's fine.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]