Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Richard Purdie
On Mon, 2007-03-12 at 16:31 -0400, Dmitry Torokhov wrote:
> On 3/12/07, Alan Stern <[EMAIL PROTECTED]> wrote:
> > On Mon, 12 Mar 2007, Oliver Neukum wrote:
> > > > >  I don't like reverting my own code. But I predict he'll tell you 
> > > > > that a
> > > > > driver's bond with a device should be represented in a data structure
> > > > > that is to be refcounted.
> >
> > There still would be a synchronization problem.  Refcounts don't solve
> > races; they only solve lifetime problems.  And you would still have to
> > change the sysfs API, plus all the other stuff...
> >
> > Do you think Linus would listen if all three of us (plus maybe Greg) tried
> > to convince him?
> >
> 
> If we'd accompany the argument with the patch that changes scsi to use
> wq to perform deletion so we don't have deadlock regression in the
> kernel he might be more perceptive... He is right about lifetime
> issues but this is not strictly lifetime issue as you correctly point
> out. Plus, refcounting also bloats the kernel so I don't relly want to
> use refcount for every integer I happen to export through sysfs if I
> can simply "revoke" access.

For what its worth, I think it makes sense if the driver no longer has
to worry about sysfs attributes after they've been removed. This is
something the core should look after, not each and every driver.

http://marc.theaimsgroup.com/?l=linux-kernel=117355959020831=2

makes a lot of sense, particularly that "No driver callbacks occur after
unregistration". When writing the backlight class code, I remember
checking into this, concluding that seemed to be the design of sysfs and
thinking it a sane design.

The alternative is to force each and every driver to do its own
refcounting. My experience with locking in the extremely simple
backlight class shows nobody reads the documentation or writes the code
correctly. With that, I've given up and added suitable locking to the
core even if not every driver needs it. In doing so, I made a net
removal of a few hundred lines of broken "ticking timebomb" style code.
I dread to think what would happen if every driver had to deal with
sysfs refcounting.

So count me as a vote for handling this in the sysfs core, not the
drivers.

Richard


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Dmitry Torokhov wrote:

> > Do you think Linus would listen if all three of us (plus maybe Greg) tried
> > to convince him?
> >
> 
> If we'd accompany the argument with the patch that changes scsi to use
> wq to perform deletion so we don't have deadlock regression in the
> kernel he might be more perceptive...

I wrote that patch over the weekend but forgot to bring it in to work.  
I'll post it tonight or tomorrow.

>  He is right about lifetime
> issues but this is not strictly lifetime issue as you correctly point
> out. Plus, refcounting also bloats the kernel so I don't relly want to
> use refcount for every integer I happen to export through sysfs if I
> can simply "revoke" access.

Agreed.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Dmitry Torokhov

On 3/12/07, Alan Stern <[EMAIL PROTECTED]> wrote:

On Mon, 12 Mar 2007, Oliver Neukum wrote:

> > >  I don't like reverting my own code. But I predict he'll tell you that a
> > > driver's bond with a device should be represented in a data structure
> > > that is to be refcounted.
> >
> > :-)
>
> Coming to think about it, he might be right there.

There still would be a synchronization problem.  Refcounts don't solve
races; they only solve lifetime problems.  And you would still have to
change the sysfs API, plus all the other stuff...

Do you think Linus would listen if all three of us (plus maybe Greg) tried
to convince him?



If we'd accompany the argument with the patch that changes scsi to use
wq to perform deletion so we don't have deadlock regression in the
kernel he might be more perceptive... He is right about lifetime
issues but this is not strictly lifetime issue as you correctly point
out. Plus, refcounting also bloats the kernel so I don't relly want to
use refcount for every integer I happen to export through sysfs if I
can simply "revoke" access.

--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 21:03 schrieb Alan Stern:
> On Mon, 12 Mar 2007, Oliver Neukum wrote:
> 
> > > >  I don't like reverting my own code. But I predict he'll tell you that a
> > > > driver's bond with a device should be represented in a data structure
> > > > that is to be refcounted.
> > > 
> > > :-)
> > 
> > Coming to think about it, he might be right there.
> 
> There still would be a synchronization problem.  Refcounts don't solve
> races; they only solve lifetime problems.  And you would still have to
> change the sysfs API, plus all the other stuff...
> 
> Do you think Linus would listen if all three of us (plus maybe Greg) tried 
> to convince him?

No. He'd tell you that a crap API should be changed.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

> > >  I don't like reverting my own code. But I predict he'll tell you that a
> > > driver's bond with a device should be represented in a data structure
> > > that is to be refcounted.
> > 
> > :-)
> 
> Coming to think about it, he might be right there.

There still would be a synchronization problem.  Refcounts don't solve
races; they only solve lifetime problems.  And you would still have to
change the sysfs API, plus all the other stuff...

Do you think Linus would listen if all three of us (plus maybe Greg) tried 
to convince him?

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 20:31 schrieb Alan Stern:
> On Mon, 12 Mar 2007, Oliver Neukum wrote:
> 
> > > I'm with Dmitry; the whole thing becomes much, much simpler if we put back
> > > your patch and prevent sysfs access after unregistering an attribute 
> > > file.  No API changes are needed, no driver changes are needed, no 
> > > radical 
> > > core changes are needed,...  All we would have to do is fix the one SCSI 
> > > method to make it use a workqueue.
> > 
> > Try.
> 
> I did.  Didn't you see this message from Saturday:
> 
> http://marc.theaimsgroup.com/?l=linux-kernel=117355959020831=2

Yes. In this case, silence is partial agreement. However, convincing me
is futile if Linus rejects the approach.

I wrote the original patch. But this problem must be solved. If the
first attempt is rejected, I'll try another.

> I sent it to Linus as well as to all of you.  No replies received so far 
> from anybody.
> 
> >  I don't like reverting my own code. But I predict he'll tell you that a
> > driver's bond with a device should be represented in a data structure
> > that is to be refcounted.
> 
> :-)

Coming to think about it, he might be right there.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

> > I'm with Dmitry; the whole thing becomes much, much simpler if we put back
> > your patch and prevent sysfs access after unregistering an attribute 
> > file.  No API changes are needed, no driver changes are needed, no radical 
> > core changes are needed,...  All we would have to do is fix the one SCSI 
> > method to make it use a workqueue.
> 
> Try.

I did.  Didn't you see this message from Saturday:

http://marc.theaimsgroup.com/?l=linux-kernel=117355959020831=2

I sent it to Linus as well as to all of you.  No replies received so far 
from anybody.

>  I don't like reverting my own code. But I predict he'll tell you that a
> driver's bond with a device should be represented in a data structure
> that is to be refcounted.

:-)

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 17:21 schrieb Alan Stern:
> On Mon, 12 Mar 2007, Oliver Neukum wrote:
> 
> > > > Yes, I was missing the point. In consequence, drivers must not use
> > > > dev_get_drvdata() to get their references to their private data. It's
> 
> You do realize how foolish that sounds?  Why do you think 
> dev_get_drvdata() was written in the first place?

It's still useful in disconnect/suspend/resume/etc...
If everything were alright with the design, we wouldn't be discussing
it now, would we?

 
> I'm with Dmitry; the whole thing becomes much, much simpler if we put back
> your patch and prevent sysfs access after unregistering an attribute 
> file.  No API changes are needed, no driver changes are needed, no radical 
> core changes are needed,...  All we would have to do is fix the one SCSI 
> method to make it use a workqueue.

Try. I don't like reverting my own code. But I predict he'll tell you that a
driver's bond with a device should be represented in a data structure
that is to be refcounted.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

> > > Yes, I was missing the point. In consequence, drivers must not use
> > > dev_get_drvdata() to get their references to their private data. It's

You do realize how foolish that sounds?  Why do you think 
dev_get_drvdata() was written in the first place?

> > > probably necessary to store it in struct sysfs_buffer and include that
> > > in the store/show callbacks.
> > > (The same does apply to interfaces of course)
> > >
> > 
> > Or drivers coudl verify that they still bound to the device they are
> > about to operate on (psmouse does this by taking a lock on device and
> > then checking if driver bound is the same address as psmouse). But I'd
> > rather get rid of all this clutter if we could sever sysfs access
> > after removing corresponding attributes.
> 
> No, the call has to fail if the driver is rebound to the device.

I'm with Dmitry; the whole thing becomes much, much simpler if we put back
your patch and prevent sysfs access after unregistering an attribute 
file.  No API changes are needed, no driver changes are needed, no radical 
core changes are needed,...  All we would have to do is fix the one SCSI 
method to make it use a workqueue.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 16:42 schrieb Dmitry Torokhov:
> On 3/12/07, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> > Am Montag, 12. März 2007 15:57 schrieb Alan Stern
> > > No, you're missing the point.  Let's say driver A's disconnect() is
> > > called, so the driver marks its private data structure as "disconnected"
> > > and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
> > > device, and it does its own dev_set_drvdata().  Then a user still holding
> > > an open sysfs file reference for driver A calls a show() or store()
> > > method.  The method will do dev_get_drvdata(), receiving the pointer to
> > > driver B's private data.  Now you're in trouble, because A's method will
> > > think it owns B's private data!
> >
> > Yes, I was missing the point. In consequence, drivers must not use
> > dev_get_drvdata() to get their references to their private data. It's
> > probably necessary to store it in struct sysfs_buffer and include that
> > in the store/show callbacks.
> > (The same does apply to interfaces of course)
> >
> 
> Or drivers coudl verify that they still bound to the device they are
> about to operate on (psmouse does this by taking a lock on device and
> then checking if driver bound is the same address as psmouse). But I'd
> rather get rid of all this clutter if we could sever sysfs access
> after removing corresponding attributes.

No, the call has to fail if the driver is rebound to the device.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Dmitry Torokhov

On 3/12/07, Oliver Neukum <[EMAIL PROTECTED]> wrote:

Am Montag, 12. März 2007 15:57 schrieb Alan Stern:probably nece
> On Mon, 12 Mar 2007, Oliver Neukum wrote:
>
> > > > Why? What's wrong with simply calling kref_get/put?
> > >
> > > It's the same old problem: the race between unbind and sysfs I/O.  What
> > > good does holding a reference to the private data structure do if the
> > > show/store method gets called after the driver has been unbound from the
> > > device?  dev_get_drvdata() will no longer provide a valid pointer to the
> > > private data, so the method will have no way to access it.  Hence the
> > > method needs another argument.
> >
> > It does half the job. You can make sure the driver is not asked to access
> > freed memory.
> > It is true that a driver will have to mark that device "disconnected"
> > and return errors if that device's attributes are referenced, but this can
> > be done internally.
>
> No, you're missing the point.  Let's say driver A's disconnect() is
> called, so the driver marks its private data structure as "disconnected"
> and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
> device, and it does its own dev_set_drvdata().  Then a user still holding
> an open sysfs file reference for driver A calls a show() or store()
> method.  The method will do dev_get_drvdata(), receiving the pointer to
> driver B's private data.  Now you're in trouble, because A's method will
> think it owns B's private data!

Yes, I was missing the point. In consequence, drivers must not use
dev_get_drvdata() to get their references to their private data. It's
probably necessary to store it in struct sysfs_buffer and include that
in the store/show callbacks.
(The same does apply to interfaces of course)



Or drivers coudl verify that they still bound to the device they are
about to operate on (psmouse does this by taking a lock on device and
then checking if driver bound is the same address as psmouse). But I'd
rather get rid of all this clutter if we could sever sysfs access
after removing corresponding attributes.

--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 15:57 schrieb Alan Stern:probably nece
> On Mon, 12 Mar 2007, Oliver Neukum wrote:
> 
> > > > Why? What's wrong with simply calling kref_get/put?
> > > 
> > > It's the same old problem: the race between unbind and sysfs I/O.  What
> > > good does holding a reference to the private data structure do if the
> > > show/store method gets called after the driver has been unbound from the
> > > device?  dev_get_drvdata() will no longer provide a valid pointer to the
> > > private data, so the method will have no way to access it.  Hence the
> > > method needs another argument.
> > 
> > It does half the job. You can make sure the driver is not asked to access
> > freed memory.
> > It is true that a driver will have to mark that device "disconnected"
> > and return errors if that device's attributes are referenced, but this can
> > be done internally.
> 
> No, you're missing the point.  Let's say driver A's disconnect() is
> called, so the driver marks its private data structure as "disconnected"
> and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
> device, and it does its own dev_set_drvdata().  Then a user still holding
> an open sysfs file reference for driver A calls a show() or store()  
> method.  The method will do dev_get_drvdata(), receiving the pointer to
> driver B's private data.  Now you're in trouble, because A's method will
> think it owns B's private data!

Yes, I was missing the point. In consequence, drivers must not use
dev_get_drvdata() to get their references to their private data. It's
probably necessary to store it in struct sysfs_buffer and include that
in the store/show callbacks.
(The same does apply to interfaces of course)

> > Yes, this is a bit more complicated.
> > {rant mode}
> > Who came up with the idea of making life simpler by adding a code path?
> > All these problems were already solved for device nodes. Ioctl is ugly, but
> > at least a known code path.
> > {rant off}
> 
> I'll let Greg give the complete answer.  :-)  Bear in mind, however, that
> the aim was probably to make life simpler for userspace -- which does not
> mean making life simpler for the kernel.

That doesn't mean that the method needed to be thrown out.
Sysfs could simply pass through the syscalls for a device, like
it is done in character devices. I am tempted to recommend
such radical surgery.

> (Incidentally, I'm not so sure that all these problems really were solved 
> by ioctl on device nodes.  I bet you could find plenty of cases where 
> ioctl races with disconnect if you looked.)

I will look. Death to all race conditions.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

> > > Why? What's wrong with simply calling kref_get/put?
> > 
> > It's the same old problem: the race between unbind and sysfs I/O.  What
> > good does holding a reference to the private data structure do if the
> > show/store method gets called after the driver has been unbound from the
> > device?  dev_get_drvdata() will no longer provide a valid pointer to the
> > private data, so the method will have no way to access it.  Hence the
> > method needs another argument.
> 
> It does half the job. You can make sure the driver is not asked to access
> freed memory.
> It is true that a driver will have to mark that device "disconnected"
> and return errors if that device's attributes are referenced, but this can
> be done internally.

No, you're missing the point.  Let's say driver A's disconnect() is
called, so the driver marks its private data structure as "disconnected"
and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
device, and it does its own dev_set_drvdata().  Then a user still holding
an open sysfs file reference for driver A calls a show() or store()  
method.  The method will do dev_get_drvdata(), receiving the pointer to
driver B's private data.  Now you're in trouble, because A's method will
think it owns B's private data!

> Yes, this is a bit more complicated.
> {rant mode}
> Who came up with the idea of making life simpler by adding a code path?
> All these problems were already solved for device nodes. Ioctl is ugly, but
> at least a known code path.
> {rant off}

I'll let Greg give the complete answer.  :-)  Bear in mind, however, that
the aim was probably to make life simpler for userspace -- which does not
mean making life simpler for the kernel.

(Incidentally, I'm not so sure that all these problems really were solved 
by ioctl on device nodes.  I bet you could find plenty of cases where 
ioctl races with disconnect if you looked.)

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Samstag, 10. März 2007 20:19 schrieb Alan Stern:
> On Fri, 9 Mar 2007, Oliver Neukum wrote:
> 
> > Am Freitag, 9. März 2007 21:08 schrieb Alan Stern:
> > > After some more thought, I basically agree with what Oliver wrote
> > > originally.  sysfs_dirent is indeed the logical place to store the kref
> > > pointer.  However it needs to be used during open and release, not during
> > 
> > OK.
> > 
> > > read, write, and poll.  Another point, which Oliver didn't think of, is
> > > that the kref pointer needs to be passed to the driver as an argument in
> > > the show() and store() method calls.
> > 
> > Why? What's wrong with simply calling kref_get/put?
> 
> It's the same old problem: the race between unbind and sysfs I/O.  What
> good does holding a reference to the private data structure do if the
> show/store method gets called after the driver has been unbound from the
> device?  dev_get_drvdata() will no longer provide a valid pointer to the
> private data, so the method will have no way to access it.  Hence the
> method needs another argument.

It does half the job. You can make sure the driver is not asked to access
freed memory.
It is true that a driver will have to mark that device "disconnected"
and return errors if that device's attributes are referenced, but this can
be done internally.

Yes, this is a bit more complicated.
{rant mode}
Who came up with the idea of making life simpler by adding a code path?
All these problems were already solved for device nodes. Ioctl is ugly, but
at least a known code path.
{rant off}

> (BTW, the sysfs core would actually need more than a kref.  It would also 
> need a pointer to a release routine -- the kref contains only the atomic

Yes, this is implied.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Samstag, 10. März 2007 20:19 schrieb Alan Stern:
 On Fri, 9 Mar 2007, Oliver Neukum wrote:
 
  Am Freitag, 9. März 2007 21:08 schrieb Alan Stern:
   After some more thought, I basically agree with what Oliver wrote
   originally.  sysfs_dirent is indeed the logical place to store the kref
   pointer.  However it needs to be used during open and release, not during
  
  OK.
  
   read, write, and poll.  Another point, which Oliver didn't think of, is
   that the kref pointer needs to be passed to the driver as an argument in
   the show() and store() method calls.
  
  Why? What's wrong with simply calling kref_get/put?
 
 It's the same old problem: the race between unbind and sysfs I/O.  What
 good does holding a reference to the private data structure do if the
 show/store method gets called after the driver has been unbound from the
 device?  dev_get_drvdata() will no longer provide a valid pointer to the
 private data, so the method will have no way to access it.  Hence the
 method needs another argument.

It does half the job. You can make sure the driver is not asked to access
freed memory.
It is true that a driver will have to mark that device disconnected
and return errors if that device's attributes are referenced, but this can
be done internally.

Yes, this is a bit more complicated.
{rant mode}
Who came up with the idea of making life simpler by adding a code path?
All these problems were already solved for device nodes. Ioctl is ugly, but
at least a known code path.
{rant off}

 (BTW, the sysfs core would actually need more than a kref.  It would also 
 need a pointer to a release routine -- the kref contains only the atomic

Yes, this is implied.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

   Why? What's wrong with simply calling kref_get/put?
  
  It's the same old problem: the race between unbind and sysfs I/O.  What
  good does holding a reference to the private data structure do if the
  show/store method gets called after the driver has been unbound from the
  device?  dev_get_drvdata() will no longer provide a valid pointer to the
  private data, so the method will have no way to access it.  Hence the
  method needs another argument.
 
 It does half the job. You can make sure the driver is not asked to access
 freed memory.
 It is true that a driver will have to mark that device disconnected
 and return errors if that device's attributes are referenced, but this can
 be done internally.

No, you're missing the point.  Let's say driver A's disconnect() is
called, so the driver marks its private data structure as disconnected
and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
device, and it does its own dev_set_drvdata().  Then a user still holding
an open sysfs file reference for driver A calls a show() or store()  
method.  The method will do dev_get_drvdata(), receiving the pointer to
driver B's private data.  Now you're in trouble, because A's method will
think it owns B's private data!

 Yes, this is a bit more complicated.
 {rant mode}
 Who came up with the idea of making life simpler by adding a code path?
 All these problems were already solved for device nodes. Ioctl is ugly, but
 at least a known code path.
 {rant off}

I'll let Greg give the complete answer.  :-)  Bear in mind, however, that
the aim was probably to make life simpler for userspace -- which does not
mean making life simpler for the kernel.

(Incidentally, I'm not so sure that all these problems really were solved 
by ioctl on device nodes.  I bet you could find plenty of cases where 
ioctl races with disconnect if you looked.)

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 15:57 schrieb Alan Stern:probably nece
 On Mon, 12 Mar 2007, Oliver Neukum wrote:
 
Why? What's wrong with simply calling kref_get/put?
   
   It's the same old problem: the race between unbind and sysfs I/O.  What
   good does holding a reference to the private data structure do if the
   show/store method gets called after the driver has been unbound from the
   device?  dev_get_drvdata() will no longer provide a valid pointer to the
   private data, so the method will have no way to access it.  Hence the
   method needs another argument.
  
  It does half the job. You can make sure the driver is not asked to access
  freed memory.
  It is true that a driver will have to mark that device disconnected
  and return errors if that device's attributes are referenced, but this can
  be done internally.
 
 No, you're missing the point.  Let's say driver A's disconnect() is
 called, so the driver marks its private data structure as disconnected
 and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
 device, and it does its own dev_set_drvdata().  Then a user still holding
 an open sysfs file reference for driver A calls a show() or store()  
 method.  The method will do dev_get_drvdata(), receiving the pointer to
 driver B's private data.  Now you're in trouble, because A's method will
 think it owns B's private data!

Yes, I was missing the point. In consequence, drivers must not use
dev_get_drvdata() to get their references to their private data. It's
probably necessary to store it in struct sysfs_buffer and include that
in the store/show callbacks.
(The same does apply to interfaces of course)

  Yes, this is a bit more complicated.
  {rant mode}
  Who came up with the idea of making life simpler by adding a code path?
  All these problems were already solved for device nodes. Ioctl is ugly, but
  at least a known code path.
  {rant off}
 
 I'll let Greg give the complete answer.  :-)  Bear in mind, however, that
 the aim was probably to make life simpler for userspace -- which does not
 mean making life simpler for the kernel.

That doesn't mean that the method needed to be thrown out.
Sysfs could simply pass through the syscalls for a device, like
it is done in character devices. I am tempted to recommend
such radical surgery.

 (Incidentally, I'm not so sure that all these problems really were solved 
 by ioctl on device nodes.  I bet you could find plenty of cases where 
 ioctl races with disconnect if you looked.)

I will look. Death to all race conditions.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Dmitry Torokhov

On 3/12/07, Oliver Neukum [EMAIL PROTECTED] wrote:

Am Montag, 12. März 2007 15:57 schrieb Alan Stern:probably nece
 On Mon, 12 Mar 2007, Oliver Neukum wrote:

Why? What's wrong with simply calling kref_get/put?
  
   It's the same old problem: the race between unbind and sysfs I/O.  What
   good does holding a reference to the private data structure do if the
   show/store method gets called after the driver has been unbound from the
   device?  dev_get_drvdata() will no longer provide a valid pointer to the
   private data, so the method will have no way to access it.  Hence the
   method needs another argument.
 
  It does half the job. You can make sure the driver is not asked to access
  freed memory.
  It is true that a driver will have to mark that device disconnected
  and return errors if that device's attributes are referenced, but this can
  be done internally.

 No, you're missing the point.  Let's say driver A's disconnect() is
 called, so the driver marks its private data structure as disconnected
 and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
 device, and it does its own dev_set_drvdata().  Then a user still holding
 an open sysfs file reference for driver A calls a show() or store()
 method.  The method will do dev_get_drvdata(), receiving the pointer to
 driver B's private data.  Now you're in trouble, because A's method will
 think it owns B's private data!

Yes, I was missing the point. In consequence, drivers must not use
dev_get_drvdata() to get their references to their private data. It's
probably necessary to store it in struct sysfs_buffer and include that
in the store/show callbacks.
(The same does apply to interfaces of course)



Or drivers coudl verify that they still bound to the device they are
about to operate on (psmouse does this by taking a lock on device and
then checking if driver bound is the same address as psmouse). But I'd
rather get rid of all this clutter if we could sever sysfs access
after removing corresponding attributes.

--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 16:42 schrieb Dmitry Torokhov:
 On 3/12/07, Oliver Neukum [EMAIL PROTECTED] wrote:
  Am Montag, 12. März 2007 15:57 schrieb Alan Stern
   No, you're missing the point.  Let's say driver A's disconnect() is
   called, so the driver marks its private data structure as disconnected
   and does dev_set_drvdata(NULL).  Then driver B is probed and bound to the
   device, and it does its own dev_set_drvdata().  Then a user still holding
   an open sysfs file reference for driver A calls a show() or store()
   method.  The method will do dev_get_drvdata(), receiving the pointer to
   driver B's private data.  Now you're in trouble, because A's method will
   think it owns B's private data!
 
  Yes, I was missing the point. In consequence, drivers must not use
  dev_get_drvdata() to get their references to their private data. It's
  probably necessary to store it in struct sysfs_buffer and include that
  in the store/show callbacks.
  (The same does apply to interfaces of course)
 
 
 Or drivers coudl verify that they still bound to the device they are
 about to operate on (psmouse does this by taking a lock on device and
 then checking if driver bound is the same address as psmouse). But I'd
 rather get rid of all this clutter if we could sever sysfs access
 after removing corresponding attributes.

No, the call has to fail if the driver is rebound to the device.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

   Yes, I was missing the point. In consequence, drivers must not use
   dev_get_drvdata() to get their references to their private data. It's

You do realize how foolish that sounds?  Why do you think 
dev_get_drvdata() was written in the first place?

   probably necessary to store it in struct sysfs_buffer and include that
   in the store/show callbacks.
   (The same does apply to interfaces of course)
  
  
  Or drivers coudl verify that they still bound to the device they are
  about to operate on (psmouse does this by taking a lock on device and
  then checking if driver bound is the same address as psmouse). But I'd
  rather get rid of all this clutter if we could sever sysfs access
  after removing corresponding attributes.
 
 No, the call has to fail if the driver is rebound to the device.

I'm with Dmitry; the whole thing becomes much, much simpler if we put back
your patch and prevent sysfs access after unregistering an attribute 
file.  No API changes are needed, no driver changes are needed, no radical 
core changes are needed,...  All we would have to do is fix the one SCSI 
method to make it use a workqueue.

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 17:21 schrieb Alan Stern:
 On Mon, 12 Mar 2007, Oliver Neukum wrote:
 
Yes, I was missing the point. In consequence, drivers must not use
dev_get_drvdata() to get their references to their private data. It's
 
 You do realize how foolish that sounds?  Why do you think 
 dev_get_drvdata() was written in the first place?

It's still useful in disconnect/suspend/resume/etc...
If everything were alright with the design, we wouldn't be discussing
it now, would we?

 
 I'm with Dmitry; the whole thing becomes much, much simpler if we put back
 your patch and prevent sysfs access after unregistering an attribute 
 file.  No API changes are needed, no driver changes are needed, no radical 
 core changes are needed,...  All we would have to do is fix the one SCSI 
 method to make it use a workqueue.

Try. I don't like reverting my own code. But I predict he'll tell you that a
driver's bond with a device should be represented in a data structure
that is to be refcounted.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

  I'm with Dmitry; the whole thing becomes much, much simpler if we put back
  your patch and prevent sysfs access after unregistering an attribute 
  file.  No API changes are needed, no driver changes are needed, no radical 
  core changes are needed,...  All we would have to do is fix the one SCSI 
  method to make it use a workqueue.
 
 Try.

I did.  Didn't you see this message from Saturday:

http://marc.theaimsgroup.com/?l=linux-kernelm=117355959020831w=2

I sent it to Linus as well as to all of you.  No replies received so far 
from anybody.

  I don't like reverting my own code. But I predict he'll tell you that a
 driver's bond with a device should be represented in a data structure
 that is to be refcounted.

:-)

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 20:31 schrieb Alan Stern:
 On Mon, 12 Mar 2007, Oliver Neukum wrote:
 
   I'm with Dmitry; the whole thing becomes much, much simpler if we put back
   your patch and prevent sysfs access after unregistering an attribute 
   file.  No API changes are needed, no driver changes are needed, no 
   radical 
   core changes are needed,...  All we would have to do is fix the one SCSI 
   method to make it use a workqueue.
  
  Try.
 
 I did.  Didn't you see this message from Saturday:
 
 http://marc.theaimsgroup.com/?l=linux-kernelm=117355959020831w=2

Yes. In this case, silence is partial agreement. However, convincing me
is futile if Linus rejects the approach.

I wrote the original patch. But this problem must be solved. If the
first attempt is rejected, I'll try another.

 I sent it to Linus as well as to all of you.  No replies received so far 
 from anybody.
 
   I don't like reverting my own code. But I predict he'll tell you that a
  driver's bond with a device should be represented in a data structure
  that is to be refcounted.
 
 :-)

Coming to think about it, he might be right there.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Oliver Neukum wrote:

I don't like reverting my own code. But I predict he'll tell you that a
   driver's bond with a device should be represented in a data structure
   that is to be refcounted.
  
  :-)
 
 Coming to think about it, he might be right there.

There still would be a synchronization problem.  Refcounts don't solve
races; they only solve lifetime problems.  And you would still have to
change the sysfs API, plus all the other stuff...

Do you think Linus would listen if all three of us (plus maybe Greg) tried 
to convince him?

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Oliver Neukum
Am Montag, 12. März 2007 21:03 schrieb Alan Stern:
 On Mon, 12 Mar 2007, Oliver Neukum wrote:
 
 I don't like reverting my own code. But I predict he'll tell you that a
driver's bond with a device should be represented in a data structure
that is to be refcounted.
   
   :-)
  
  Coming to think about it, he might be right there.
 
 There still would be a synchronization problem.  Refcounts don't solve
 races; they only solve lifetime problems.  And you would still have to
 change the sysfs API, plus all the other stuff...
 
 Do you think Linus would listen if all three of us (plus maybe Greg) tried 
 to convince him?

No. He'd tell you that a crap API should be changed.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Dmitry Torokhov

On 3/12/07, Alan Stern [EMAIL PROTECTED] wrote:

On Mon, 12 Mar 2007, Oliver Neukum wrote:

I don't like reverting my own code. But I predict he'll tell you that a
   driver's bond with a device should be represented in a data structure
   that is to be refcounted.
 
  :-)

 Coming to think about it, he might be right there.

There still would be a synchronization problem.  Refcounts don't solve
races; they only solve lifetime problems.  And you would still have to
change the sysfs API, plus all the other stuff...

Do you think Linus would listen if all three of us (plus maybe Greg) tried
to convince him?



If we'd accompany the argument with the patch that changes scsi to use
wq to perform deletion so we don't have deadlock regression in the
kernel he might be more perceptive... He is right about lifetime
issues but this is not strictly lifetime issue as you correctly point
out. Plus, refcounting also bloats the kernel so I don't relly want to
use refcount for every integer I happen to export through sysfs if I
can simply revoke access.

--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Alan Stern
On Mon, 12 Mar 2007, Dmitry Torokhov wrote:

  Do you think Linus would listen if all three of us (plus maybe Greg) tried
  to convince him?
 
 
 If we'd accompany the argument with the patch that changes scsi to use
 wq to perform deletion so we don't have deadlock regression in the
 kernel he might be more perceptive...

I wrote that patch over the weekend but forgot to bring it in to work.  
I'll post it tonight or tomorrow.

  He is right about lifetime
 issues but this is not strictly lifetime issue as you correctly point
 out. Plus, refcounting also bloats the kernel so I don't relly want to
 use refcount for every integer I happen to export through sysfs if I
 can simply revoke access.

Agreed.

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-12 Thread Richard Purdie
On Mon, 2007-03-12 at 16:31 -0400, Dmitry Torokhov wrote:
 On 3/12/07, Alan Stern [EMAIL PROTECTED] wrote:
  On Mon, 12 Mar 2007, Oliver Neukum wrote:
  I don't like reverting my own code. But I predict he'll tell you 
 that a
 driver's bond with a device should be represented in a data structure
 that is to be refcounted.
 
  There still would be a synchronization problem.  Refcounts don't solve
  races; they only solve lifetime problems.  And you would still have to
  change the sysfs API, plus all the other stuff...
 
  Do you think Linus would listen if all three of us (plus maybe Greg) tried
  to convince him?
 
 
 If we'd accompany the argument with the patch that changes scsi to use
 wq to perform deletion so we don't have deadlock regression in the
 kernel he might be more perceptive... He is right about lifetime
 issues but this is not strictly lifetime issue as you correctly point
 out. Plus, refcounting also bloats the kernel so I don't relly want to
 use refcount for every integer I happen to export through sysfs if I
 can simply revoke access.

For what its worth, I think it makes sense if the driver no longer has
to worry about sysfs attributes after they've been removed. This is
something the core should look after, not each and every driver.

http://marc.theaimsgroup.com/?l=linux-kernelm=117355959020831w=2

makes a lot of sense, particularly that No driver callbacks occur after
unregistration. When writing the backlight class code, I remember
checking into this, concluding that seemed to be the design of sysfs and
thinking it a sane design.

The alternative is to force each and every driver to do its own
refcounting. My experience with locking in the extremely simple
backlight class shows nobody reads the documentation or writes the code
correctly. With that, I've given up and added suitable locking to the
core even if not every driver needs it. In doing so, I made a net
removal of a few hundred lines of broken ticking timebomb style code.
I dread to think what would happen if every driver had to deal with
sysfs refcounting.

So count me as a vote for handling this in the sysfs core, not the
drivers.

Richard


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-10 Thread Alan Stern
On Fri, 9 Mar 2007, Oliver Neukum wrote:

> Am Freitag, 9. März 2007 21:08 schrieb Alan Stern:
> > After some more thought, I basically agree with what Oliver wrote
> > originally.  sysfs_dirent is indeed the logical place to store the kref
> > pointer.  However it needs to be used during open and release, not during
> 
> OK.
> 
> > read, write, and poll.  Another point, which Oliver didn't think of, is
> > that the kref pointer needs to be passed to the driver as an argument in
> > the show() and store() method calls.
> 
> Why? What's wrong with simply calling kref_get/put?

It's the same old problem: the race between unbind and sysfs I/O.  What
good does holding a reference to the private data structure do if the
show/store method gets called after the driver has been unbound from the
device?  dev_get_drvdata() will no longer provide a valid pointer to the
private data, so the method will have no way to access it.  Hence the
method needs another argument.

(BTW, the sysfs core would actually need more than a kref.  It would also 
need a pointer to a release routine -- the kref contains only the atomic 
counter.  The more you think about it, the more complicated this approach 
becomes.)

> > Finally, there's added complexity in each driver which wants to use the 
> > new facility.  The module_exit routine will need to be smart enough to 
> > block until all the private data structures have been released.  
> > usb-storage does something like that now; it's kind of ugly (although it 
> > could be improved if appropriate support were added to the core kernel).
> 
> If we up the module count for every bound device, all device attributes
> should be gone before we ever get that far.

Not quite right.  However, since every open sysfs file holds a module
reference, if the driver's module_exit has been called then there can be
no open sysfs files, hence no private data still pinned.  Thus this isn't
a problem at all.


But never mind all the above.  I'm going to post another message on this
thread in which I argue that Oliver's original approach was a good one and
should not have been reverted.  The specific problem identified by Hugh
Dickins can be fixed in the way Dmitry first suggested, by doing the real
operation from a workqueue routine.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-10 Thread Alan Stern
On Fri, 9 Mar 2007, Oliver Neukum wrote:

 Am Freitag, 9. März 2007 21:08 schrieb Alan Stern:
  After some more thought, I basically agree with what Oliver wrote
  originally.  sysfs_dirent is indeed the logical place to store the kref
  pointer.  However it needs to be used during open and release, not during
 
 OK.
 
  read, write, and poll.  Another point, which Oliver didn't think of, is
  that the kref pointer needs to be passed to the driver as an argument in
  the show() and store() method calls.
 
 Why? What's wrong with simply calling kref_get/put?

It's the same old problem: the race between unbind and sysfs I/O.  What
good does holding a reference to the private data structure do if the
show/store method gets called after the driver has been unbound from the
device?  dev_get_drvdata() will no longer provide a valid pointer to the
private data, so the method will have no way to access it.  Hence the
method needs another argument.

(BTW, the sysfs core would actually need more than a kref.  It would also 
need a pointer to a release routine -- the kref contains only the atomic 
counter.  The more you think about it, the more complicated this approach 
becomes.)

  Finally, there's added complexity in each driver which wants to use the 
  new facility.  The module_exit routine will need to be smart enough to 
  block until all the private data structures have been released.  
  usb-storage does something like that now; it's kind of ugly (although it 
  could be improved if appropriate support were added to the core kernel).
 
 If we up the module count for every bound device, all device attributes
 should be gone before we ever get that far.

Not quite right.  However, since every open sysfs file holds a module
reference, if the driver's module_exit has been called then there can be
no open sysfs files, hence no private data still pinned.  Thus this isn't
a problem at all.


But never mind all the above.  I'm going to post another message on this
thread in which I argue that Oliver's original approach was a good one and
should not have been reverted.  The specific problem identified by Hugh
Dickins can be fixed in the way Dmitry first suggested, by doing the real
operation from a workqueue routine.

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 21:08 schrieb Alan Stern:
> After some more thought, I basically agree with what Oliver wrote
> originally.  sysfs_dirent is indeed the logical place to store the kref
> pointer.  However it needs to be used during open and release, not during

OK.

> read, write, and poll.  Another point, which Oliver didn't think of, is
> that the kref pointer needs to be passed to the driver as an argument in
> the show() and store() method calls.

Why? What's wrong with simply calling kref_get/put?
 
> Finally, there's added complexity in each driver which wants to use the 
> new facility.  The module_exit routine will need to be smart enough to 
> block until all the private data structures have been released.  
> usb-storage does something like that now; it's kind of ugly (although it 
> could be improved if appropriate support were added to the core kernel).

If we up the module count for every bound device, all device attributes
should be gone before we ever get that far.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 21:27 schrieb Alan Stern:
> On Fri, 9 Mar 2007, Oliver Neukum wrote:
> 
> > > Adding a new release() callback would solve the problem by creating 
> > > another.  Drivers need to release their data as soon as possible after
> > > they unbind from a device, not when the device itself goes away.  Think
> > 
> > Wait, the callback from closing the file in sysfs is the earliest we can 
> > safely
> > free the data structure. How do you want to free earlier?
> 
> It is _not_ the earliest we can safely free the data structure.
> 
> Dmitry's callback occurs when _all_ the sysfs attributes have been
> released -- including ones that don't have anything to do with the
> driver's private data structure.  Think of the bInterfaceClass attribute,
> for example.

Ok, yes I see. It is by far too late.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Oliver Neukum wrote:

> > Adding a new release() callback would solve the problem by creating 
> > another.  Drivers need to release their data as soon as possible after
> > they unbind from a device, not when the device itself goes away.  Think
> 
> Wait, the callback from closing the file in sysfs is the earliest we can 
> safely
> free the data structure. How do you want to free earlier?

It is _not_ the earliest we can safely free the data structure.

Dmitry's callback occurs when _all_ the sysfs attributes have been
released -- including ones that don't have anything to do with the
driver's private data structure.  Think of the bInterfaceClass attribute,
for example.

But even aside from that, Dmitry's suggestion is wrong.  He wants to add a
second release() method to the driver, which can be called from the
subsystem's release() method -- which doesn't run until the device is
unregistered, possibly long after the driver has been unbound from it.  
Then how could the subsystem even know which drivers need their second
release method called?


> > Oliver, your idea won't work either.  Think about what would happen if 
> > someone did
> > 
> > rmmod driver_module  
> -EBUSY, as currently.

Yeah, I was wrong about that.


> > It might be better to keep your earlier patch and fix the deadlock you
> > mentioned earlier, the one that occurs when unbinding a driver through
> > sysfs.  How exactly does that deadlock work?
> 
> http://lkml.org/lkml/2007/3/6/364
> http://lkml.org/lkml/2007/3/6/528

I get the picture, thanks.

Alan Stern


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Alan Stern wrote:

> Oliver, your idea won't work either.  Think about what would happen if 
> someone did
> 
>   rmmod driver_module  
> The rmmod process would never actually read the attribute, so until it 
> exited the private data structure would have a positive refcount.  But 
> rmmod can't exit until the driver has been unloaded from memory, and it 
> can't be unloaded while its data structure is still allocated.  Thus we 
> would end up with deadlock; rmmod would hang forever.

I take this back.  Redirecting stdin to the attribute file would increase 
the module's refcount and cause rmmod to exit immediately with an error.

After some more thought, I basically agree with what Oliver wrote
originally.  sysfs_dirent is indeed the logical place to store the kref
pointer.  However it needs to be used during open and release, not during
read, write, and poll.  Another point, which Oliver didn't think of, is
that the kref pointer needs to be passed to the driver as an argument in
the show() and store() method calls.

Implementing this will be difficult.  One possibility is to change the 
definition of sysfs_ops, adding the new struct kref * argument to the 
prototypes.  This will involve changing _lots_ of source files, adding an 
unused argument to many functions, which isn't attractive.

The other possibility is to test at runtime whether the kref pointer is 
NULL, and if it is, don't pass it.  This would work, but it isn't 
type-safe.

Finally, there's added complexity in each driver which wants to use the 
new facility.  The module_exit routine will need to be smart enough to 
block until all the private data structures have been released.  
usb-storage does something like that now; it's kind of ugly (although it 
could be improved if appropriate support were added to the core kernel).

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 20:32 schrieb Alan Stern:
> On Fri, 9 Mar 2007, Dmitry Torokhov wrote:
> 
> > On 3/9/07, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> > > Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:
> > >
> > > > I think we already have all refcounting that is needed. What is
> > > > missing is subsystem-provided ->release() hooks for drivers to release
> > > > driver-specific resources when a device finally goes away.
> > >
> > > This is an interesting idea. Is it nice to pass through release()
> > > but not open() ?
> > >
> > 
> > Not sure if I follow... Generally speaking open is not a mandatory
> > operation; however every object in driver model has a release method.
> > What I am saying is that certain drivers need to have their disconnect
> > method split in 2 parts - one that shuts down the device and second is
> > releases resources that might be accesses through sysfs (and other
> > kernel parts). That second part will have to be called from
> > subsystem's core ->release() method se we need a release() hook.
> 
> Dmitry, you're not viewing this correctly.
> 
> Adding a new release() callback would solve the problem by creating 
> another.  Drivers need to release their data as soon as possible after
> they unbind from a device, not when the device itself goes away.  Think

Wait, the callback from closing the file in sysfs is the earliest we can safely
free the data structure. How do you want to free earlier?

> about what would happen if you tried to rmmod a driver.  The rmmod process 
> would block until the device was unregistered.
> 
> Oliver, your idea won't work either.  Think about what would happen if 
> someone did
> 
>   rmmod driver_module  The rmmod process would never actually read the attribute, so until it 
> exited the private data structure would have a positive refcount.  But 
> rmmod can't exit until the driver has been unloaded from memory, and it 
> can't be unloaded while its data structure is still allocated.  Thus we 
> would end up with deadlock; rmmod would hang forever.
> 
> It might be better to keep your earlier patch and fix the deadlock you
> mentioned earlier, the one that occurs when unbinding a driver through
> sysfs.  How exactly does that deadlock work?

http://lkml.org/lkml/2007/3/6/364
http://lkml.org/lkml/2007/3/6/528

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Dmitry Torokhov wrote:

> On 3/9/07, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> > Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:
> >
> > > I think we already have all refcounting that is needed. What is
> > > missing is subsystem-provided ->release() hooks for drivers to release
> > > driver-specific resources when a device finally goes away.
> >
> > This is an interesting idea. Is it nice to pass through release()
> > but not open() ?
> >
> 
> Not sure if I follow... Generally speaking open is not a mandatory
> operation; however every object in driver model has a release method.
> What I am saying is that certain drivers need to have their disconnect
> method split in 2 parts - one that shuts down the device and second is
> releases resources that might be accesses through sysfs (and other
> kernel parts). That second part will have to be called from
> subsystem's core ->release() method se we need a release() hook.

Dmitry, you're not viewing this correctly.

Adding a new release() callback would solve the problem by creating 
another.  Drivers need to release their data as soon as possible after
they unbind from a device, not when the device itself goes away.  Think
about what would happen if you tried to rmmod a driver.  The rmmod process 
would block until the device was unregistered.

Oliver, your idea won't work either.  Think about what would happen if 
someone did

rmmod driver_module http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Dmitry Torokhov

On 3/9/07, Oliver Neukum <[EMAIL PROTECTED]> wrote:

Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:

> I think we already have all refcounting that is needed. What is
> missing is subsystem-provided ->release() hooks for drivers to release
> driver-specific resources when a device finally goes away.

This is an interesting idea. Is it nice to pass through release()
but not open() ?



Not sure if I follow... Generally speaking open is not a mandatory
operation; however every object in driver model has a release method.
What I am saying is that certain drivers need to have their disconnect
method split in 2 parts - one that shuts down the device and second is
releases resources that might be accesses through sysfs (and other
kernel parts). That second part will have to be called from
subsystem's core ->release() method se we need a release() hook.

--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:

> I think we already have all refcounting that is needed. What is
> missing is subsystem-provided ->release() hooks for drivers to release
> driver-specific resources when a device finally goes away.

This is an interesting idea. Is it nice to pass through release()
but not open() ?

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Dmitry Torokhov

On 3/9/07, Oliver Neukum <[EMAIL PROTECTED]> wrote:

Am Freitag, 9. März 2007 17:32 schrieb Alan Stern:
> On Fri, 9 Mar 2007, Oliver Neukum wrote:
>
> > Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
> > > On Thu, 8 Mar 2007, Oliver Neukum wrote:
> > >
> > > > Hi,
> > > >
> > > > after a lightning bolt from high above I've been looking into 
refcounting
> > > > the data structures drivers use to provide the data used to refill sysfs
> > > > buffers. I've come to the following conclusion.
> > > >
> > > > 1. struct sysfs_buffer must have a struct kref * and probably a 
destructor
> > > > pointer
> > > > 2. drivers must be able to pass these pointers through an extended
> > > > device_create_file()
> > > > 3. Drivers must use refcounting if they want to use attributes
> > > > 4. read/write/poll must do refcounting
> > > >
> > > > I am not sure where to store the pointers. struct sysfs_dirent() looks
> > > > like the obvious choice. Comments?
> > >
> > > Can you explain the reasoning that led to these conclusions?  And what
> > > exactly was your lightning bolt?
> >
> > The old race between disconnect and IO to attribute via sysfs again.
> > If I cannot disassociate the drivers from the buffers in the buffers, 
drivers
> > must not deallocate the data necessary to answer sysfs callbacks while
> > a buffer exists.
>
> Why wouldn't you be able to dissociate a driver from a buffer?  That was
> the whole point of adding .orphan to sysfs_buffer and creating
> sysfs_buffer_collection -- it was supposed to solve exactly this race.

It did solve the race but deadlocked when unbinding devices through sysfs.
Linux therefore asked for the patch to be reverted and wants the isue solved
with refcounting.



I think we already have all refcounting that is needed. What is
missing is subsystem-provided ->release() hooks for drivers to release
driver-specific resources when a device finally goes away.

--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 17:32 schrieb Alan Stern:
> On Fri, 9 Mar 2007, Oliver Neukum wrote:
> 
> > Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
> > > On Thu, 8 Mar 2007, Oliver Neukum wrote:
> > > 
> > > > Hi,
> > > > 
> > > > after a lightning bolt from high above I've been looking into 
> > > > refcounting
> > > > the data structures drivers use to provide the data used to refill sysfs
> > > > buffers. I've come to the following conclusion.
> > > > 
> > > > 1. struct sysfs_buffer must have a struct kref * and probably a 
> > > > destructor
> > > > pointer
> > > > 2. drivers must be able to pass these pointers through an extended
> > > > device_create_file()
> > > > 3. Drivers must use refcounting if they want to use attributes
> > > > 4. read/write/poll must do refcounting
> > > > 
> > > > I am not sure where to store the pointers. struct sysfs_dirent() looks
> > > > like the obvious choice. Comments?
> > > 
> > > Can you explain the reasoning that led to these conclusions?  And what 
> > > exactly was your lightning bolt?
> > 
> > The old race between disconnect and IO to attribute via sysfs again.
> > If I cannot disassociate the drivers from the buffers in the buffers, 
> > drivers
> > must not deallocate the data necessary to answer sysfs callbacks while
> > a buffer exists.
> 
> Why wouldn't you be able to dissociate a driver from a buffer?  That was 
> the whole point of adding .orphan to sysfs_buffer and creating 
> sysfs_buffer_collection -- it was supposed to solve exactly this race.

It did solve the race but deadlocked when unbinding devices through sysfs.
Linux therefore asked for the patch to be reverted and wants the isue solved
with refcounting.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Oliver Neukum wrote:

> Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
> > On Thu, 8 Mar 2007, Oliver Neukum wrote:
> > 
> > > Hi,
> > > 
> > > after a lightning bolt from high above I've been looking into refcounting
> > > the data structures drivers use to provide the data used to refill sysfs
> > > buffers. I've come to the following conclusion.
> > > 
> > > 1. struct sysfs_buffer must have a struct kref * and probably a destructor
> > > pointer
> > > 2. drivers must be able to pass these pointers through an extended
> > > device_create_file()
> > > 3. Drivers must use refcounting if they want to use attributes
> > > 4. read/write/poll must do refcounting
> > > 
> > > I am not sure where to store the pointers. struct sysfs_dirent() looks
> > > like the obvious choice. Comments?
> > 
> > Can you explain the reasoning that led to these conclusions?  And what 
> > exactly was your lightning bolt?
> 
> The old race between disconnect and IO to attribute via sysfs again.
> If I cannot disassociate the drivers from the buffers in the buffers, drivers
> must not deallocate the data necessary to answer sysfs callbacks while
> a buffer exists.

Why wouldn't you be able to dissociate a driver from a buffer?  That was 
the whole point of adding .orphan to sysfs_buffer and creating 
sysfs_buffer_collection -- it was supposed to solve exactly this race.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Oliver Neukum wrote:

 Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
  On Thu, 8 Mar 2007, Oliver Neukum wrote:
  
   Hi,
   
   after a lightning bolt from high above I've been looking into refcounting
   the data structures drivers use to provide the data used to refill sysfs
   buffers. I've come to the following conclusion.
   
   1. struct sysfs_buffer must have a struct kref * and probably a destructor
   pointer
   2. drivers must be able to pass these pointers through an extended
   device_create_file()
   3. Drivers must use refcounting if they want to use attributes
   4. read/write/poll must do refcounting
   
   I am not sure where to store the pointers. struct sysfs_dirent() looks
   like the obvious choice. Comments?
  
  Can you explain the reasoning that led to these conclusions?  And what 
  exactly was your lightning bolt?
 
 The old race between disconnect and IO to attribute via sysfs again.
 If I cannot disassociate the drivers from the buffers in the buffers, drivers
 must not deallocate the data necessary to answer sysfs callbacks while
 a buffer exists.

Why wouldn't you be able to dissociate a driver from a buffer?  That was 
the whole point of adding .orphan to sysfs_buffer and creating 
sysfs_buffer_collection -- it was supposed to solve exactly this race.

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 17:32 schrieb Alan Stern:
 On Fri, 9 Mar 2007, Oliver Neukum wrote:
 
  Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
   On Thu, 8 Mar 2007, Oliver Neukum wrote:
   
Hi,

after a lightning bolt from high above I've been looking into 
refcounting
the data structures drivers use to provide the data used to refill sysfs
buffers. I've come to the following conclusion.

1. struct sysfs_buffer must have a struct kref * and probably a 
destructor
pointer
2. drivers must be able to pass these pointers through an extended
device_create_file()
3. Drivers must use refcounting if they want to use attributes
4. read/write/poll must do refcounting

I am not sure where to store the pointers. struct sysfs_dirent() looks
like the obvious choice. Comments?
   
   Can you explain the reasoning that led to these conclusions?  And what 
   exactly was your lightning bolt?
  
  The old race between disconnect and IO to attribute via sysfs again.
  If I cannot disassociate the drivers from the buffers in the buffers, 
  drivers
  must not deallocate the data necessary to answer sysfs callbacks while
  a buffer exists.
 
 Why wouldn't you be able to dissociate a driver from a buffer?  That was 
 the whole point of adding .orphan to sysfs_buffer and creating 
 sysfs_buffer_collection -- it was supposed to solve exactly this race.

It did solve the race but deadlocked when unbinding devices through sysfs.
Linux therefore asked for the patch to be reverted and wants the isue solved
with refcounting.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Dmitry Torokhov

On 3/9/07, Oliver Neukum [EMAIL PROTECTED] wrote:

Am Freitag, 9. März 2007 17:32 schrieb Alan Stern:
 On Fri, 9 Mar 2007, Oliver Neukum wrote:

  Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
   On Thu, 8 Mar 2007, Oliver Neukum wrote:
  
Hi,
   
after a lightning bolt from high above I've been looking into 
refcounting
the data structures drivers use to provide the data used to refill sysfs
buffers. I've come to the following conclusion.
   
1. struct sysfs_buffer must have a struct kref * and probably a 
destructor
pointer
2. drivers must be able to pass these pointers through an extended
device_create_file()
3. Drivers must use refcounting if they want to use attributes
4. read/write/poll must do refcounting
   
I am not sure where to store the pointers. struct sysfs_dirent() looks
like the obvious choice. Comments?
  
   Can you explain the reasoning that led to these conclusions?  And what
   exactly was your lightning bolt?
 
  The old race between disconnect and IO to attribute via sysfs again.
  If I cannot disassociate the drivers from the buffers in the buffers, 
drivers
  must not deallocate the data necessary to answer sysfs callbacks while
  a buffer exists.

 Why wouldn't you be able to dissociate a driver from a buffer?  That was
 the whole point of adding .orphan to sysfs_buffer and creating
 sysfs_buffer_collection -- it was supposed to solve exactly this race.

It did solve the race but deadlocked when unbinding devices through sysfs.
Linux therefore asked for the patch to be reverted and wants the isue solved
with refcounting.



I think we already have all refcounting that is needed. What is
missing is subsystem-provided -release() hooks for drivers to release
driver-specific resources when a device finally goes away.

--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:

 I think we already have all refcounting that is needed. What is
 missing is subsystem-provided -release() hooks for drivers to release
 driver-specific resources when a device finally goes away.

This is an interesting idea. Is it nice to pass through release()
but not open() ?

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Dmitry Torokhov

On 3/9/07, Oliver Neukum [EMAIL PROTECTED] wrote:

Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:

 I think we already have all refcounting that is needed. What is
 missing is subsystem-provided -release() hooks for drivers to release
 driver-specific resources when a device finally goes away.

This is an interesting idea. Is it nice to pass through release()
but not open() ?



Not sure if I follow... Generally speaking open is not a mandatory
operation; however every object in driver model has a release method.
What I am saying is that certain drivers need to have their disconnect
method split in 2 parts - one that shuts down the device and second is
releases resources that might be accesses through sysfs (and other
kernel parts). That second part will have to be called from
subsystem's core -release() method se we need a release() hook.

--
Dmitry
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Dmitry Torokhov wrote:

 On 3/9/07, Oliver Neukum [EMAIL PROTECTED] wrote:
  Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:
 
   I think we already have all refcounting that is needed. What is
   missing is subsystem-provided -release() hooks for drivers to release
   driver-specific resources when a device finally goes away.
 
  This is an interesting idea. Is it nice to pass through release()
  but not open() ?
 
 
 Not sure if I follow... Generally speaking open is not a mandatory
 operation; however every object in driver model has a release method.
 What I am saying is that certain drivers need to have their disconnect
 method split in 2 parts - one that shuts down the device and second is
 releases resources that might be accesses through sysfs (and other
 kernel parts). That second part will have to be called from
 subsystem's core -release() method se we need a release() hook.

Dmitry, you're not viewing this correctly.

Adding a new release() callback would solve the problem by creating 
another.  Drivers need to release their data as soon as possible after
they unbind from a device, not when the device itself goes away.  Think
about what would happen if you tried to rmmod a driver.  The rmmod process 
would block until the device was unregistered.

Oliver, your idea won't work either.  Think about what would happen if 
someone did

rmmod driver_module /sys/devices/.../attribute_file

The rmmod process would never actually read the attribute, so until it 
exited the private data structure would have a positive refcount.  But 
rmmod can't exit until the driver has been unloaded from memory, and it 
can't be unloaded while its data structure is still allocated.  Thus we 
would end up with deadlock; rmmod would hang forever.

It might be better to keep your earlier patch and fix the deadlock you
mentioned earlier, the one that occurs when unbinding a driver through
sysfs.  How exactly does that deadlock work?

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 20:32 schrieb Alan Stern:
 On Fri, 9 Mar 2007, Dmitry Torokhov wrote:
 
  On 3/9/07, Oliver Neukum [EMAIL PROTECTED] wrote:
   Am Freitag, 9. März 2007 18:02 schrieb Dmitry Torokhov:
  
I think we already have all refcounting that is needed. What is
missing is subsystem-provided -release() hooks for drivers to release
driver-specific resources when a device finally goes away.
  
   This is an interesting idea. Is it nice to pass through release()
   but not open() ?
  
  
  Not sure if I follow... Generally speaking open is not a mandatory
  operation; however every object in driver model has a release method.
  What I am saying is that certain drivers need to have their disconnect
  method split in 2 parts - one that shuts down the device and second is
  releases resources that might be accesses through sysfs (and other
  kernel parts). That second part will have to be called from
  subsystem's core -release() method se we need a release() hook.
 
 Dmitry, you're not viewing this correctly.
 
 Adding a new release() callback would solve the problem by creating 
 another.  Drivers need to release their data as soon as possible after
 they unbind from a device, not when the device itself goes away.  Think

Wait, the callback from closing the file in sysfs is the earliest we can safely
free the data structure. How do you want to free earlier?

 about what would happen if you tried to rmmod a driver.  The rmmod process 
 would block until the device was unregistered.
 
 Oliver, your idea won't work either.  Think about what would happen if 
 someone did
 
   rmmod driver_module /sys/devices/.../attribute_file

-EBUSY, as currently.
 
 The rmmod process would never actually read the attribute, so until it 
 exited the private data structure would have a positive refcount.  But 
 rmmod can't exit until the driver has been unloaded from memory, and it 
 can't be unloaded while its data structure is still allocated.  Thus we 
 would end up with deadlock; rmmod would hang forever.
 
 It might be better to keep your earlier patch and fix the deadlock you
 mentioned earlier, the one that occurs when unbinding a driver through
 sysfs.  How exactly does that deadlock work?

http://lkml.org/lkml/2007/3/6/364
http://lkml.org/lkml/2007/3/6/528

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Alan Stern wrote:

 Oliver, your idea won't work either.  Think about what would happen if 
 someone did
 
   rmmod driver_module /sys/devices/.../attribute_file
 
 The rmmod process would never actually read the attribute, so until it 
 exited the private data structure would have a positive refcount.  But 
 rmmod can't exit until the driver has been unloaded from memory, and it 
 can't be unloaded while its data structure is still allocated.  Thus we 
 would end up with deadlock; rmmod would hang forever.

I take this back.  Redirecting stdin to the attribute file would increase 
the module's refcount and cause rmmod to exit immediately with an error.

After some more thought, I basically agree with what Oliver wrote
originally.  sysfs_dirent is indeed the logical place to store the kref
pointer.  However it needs to be used during open and release, not during
read, write, and poll.  Another point, which Oliver didn't think of, is
that the kref pointer needs to be passed to the driver as an argument in
the show() and store() method calls.

Implementing this will be difficult.  One possibility is to change the 
definition of sysfs_ops, adding the new struct kref * argument to the 
prototypes.  This will involve changing _lots_ of source files, adding an 
unused argument to many functions, which isn't attractive.

The other possibility is to test at runtime whether the kref pointer is 
NULL, and if it is, don't pass it.  This would work, but it isn't 
type-safe.

Finally, there's added complexity in each driver which wants to use the 
new facility.  The module_exit routine will need to be smart enough to 
block until all the private data structures have been released.  
usb-storage does something like that now; it's kind of ugly (although it 
could be improved if appropriate support were added to the core kernel).

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Alan Stern
On Fri, 9 Mar 2007, Oliver Neukum wrote:

  Adding a new release() callback would solve the problem by creating 
  another.  Drivers need to release their data as soon as possible after
  they unbind from a device, not when the device itself goes away.  Think
 
 Wait, the callback from closing the file in sysfs is the earliest we can 
 safely
 free the data structure. How do you want to free earlier?

It is _not_ the earliest we can safely free the data structure.

Dmitry's callback occurs when _all_ the sysfs attributes have been
released -- including ones that don't have anything to do with the
driver's private data structure.  Think of the bInterfaceClass attribute,
for example.

But even aside from that, Dmitry's suggestion is wrong.  He wants to add a
second release() method to the driver, which can be called from the
subsystem's release() method -- which doesn't run until the device is
unregistered, possibly long after the driver has been unbound from it.  
Then how could the subsystem even know which drivers need their second
release method called?


  Oliver, your idea won't work either.  Think about what would happen if 
  someone did
  
  rmmod driver_module /sys/devices/.../attribute_file
 
 -EBUSY, as currently.

Yeah, I was wrong about that.


  It might be better to keep your earlier patch and fix the deadlock you
  mentioned earlier, the one that occurs when unbinding a driver through
  sysfs.  How exactly does that deadlock work?
 
 http://lkml.org/lkml/2007/3/6/364
 http://lkml.org/lkml/2007/3/6/528

I get the picture, thanks.

Alan Stern


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 21:27 schrieb Alan Stern:
 On Fri, 9 Mar 2007, Oliver Neukum wrote:
 
   Adding a new release() callback would solve the problem by creating 
   another.  Drivers need to release their data as soon as possible after
   they unbind from a device, not when the device itself goes away.  Think
  
  Wait, the callback from closing the file in sysfs is the earliest we can 
  safely
  free the data structure. How do you want to free earlier?
 
 It is _not_ the earliest we can safely free the data structure.
 
 Dmitry's callback occurs when _all_ the sysfs attributes have been
 released -- including ones that don't have anything to do with the
 driver's private data structure.  Think of the bInterfaceClass attribute,
 for example.

Ok, yes I see. It is by far too late.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-09 Thread Oliver Neukum
Am Freitag, 9. März 2007 21:08 schrieb Alan Stern:
 After some more thought, I basically agree with what Oliver wrote
 originally.  sysfs_dirent is indeed the logical place to store the kref
 pointer.  However it needs to be used during open and release, not during

OK.

 read, write, and poll.  Another point, which Oliver didn't think of, is
 that the kref pointer needs to be passed to the driver as an argument in
 the show() and store() method calls.

Why? What's wrong with simply calling kref_get/put?
 
 Finally, there's added complexity in each driver which wants to use the 
 new facility.  The module_exit routine will need to be smart enough to 
 block until all the private data structures have been released.  
 usb-storage does something like that now; it's kind of ugly (although it 
 could be improved if appropriate support were added to the core kernel).

If we up the module count for every bound device, all device attributes
should be gone before we ever get that far.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-08 Thread Oliver Neukum
Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
> On Thu, 8 Mar 2007, Oliver Neukum wrote:
> 
> > Hi,
> > 
> > after a lightning bolt from high above I've been looking into refcounting
> > the data structures drivers use to provide the data used to refill sysfs
> > buffers. I've come to the following conclusion.
> > 
> > 1. struct sysfs_buffer must have a struct kref * and probably a destructor
> > pointer
> > 2. drivers must be able to pass these pointers through an extended
> > device_create_file()
> > 3. Drivers must use refcounting if they want to use attributes
> > 4. read/write/poll must do refcounting
> > 
> > I am not sure where to store the pointers. struct sysfs_dirent() looks
> > like the obvious choice. Comments?
> 
> Can you explain the reasoning that led to these conclusions?  And what 
> exactly was your lightning bolt?

The old race between disconnect and IO to attribute via sysfs again.
If I cannot disassociate the drivers from the buffers in the buffers, drivers
must not deallocate the data necessary to answer sysfs callbacks while
a buffer exists. Reading from a buffer must up a refcount in the driver's
data structures. The question becomes how to get a pointer to the buffer.
And it cannot live in the dentry as the dentry can go away while files
are still open. This leaves the inode or the buffer.

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-08 Thread Alan Stern
On Thu, 8 Mar 2007, Oliver Neukum wrote:

> Hi,
> 
> after a lightning bolt from high above I've been looking into refcounting
> the data structures drivers use to provide the data used to refill sysfs
> buffers. I've come to the following conclusion.
> 
> 1. struct sysfs_buffer must have a struct kref * and probably a destructor
> pointer
> 2. drivers must be able to pass these pointers through an extended
> device_create_file()
> 3. Drivers must use refcounting if they want to use attributes
> 4. read/write/poll must do refcounting
> 
> I am not sure where to store the pointers. struct sysfs_dirent() looks
> like the obvious choice. Comments?

Can you explain the reasoning that led to these conclusions?  And what 
exactly was your lightning bolt?

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


refcounting drivers' data structures used in sysfs buffers

2007-03-08 Thread Oliver Neukum
Hi,

after a lightning bolt from high above I've been looking into refcounting
the data structures drivers use to provide the data used to refill sysfs
buffers. I've come to the following conclusion.

1. struct sysfs_buffer must have a struct kref * and probably a destructor
pointer
2. drivers must be able to pass these pointers through an extended
device_create_file()
3. Drivers must use refcounting if they want to use attributes
4. read/write/poll must do refcounting

I am not sure where to store the pointers. struct sysfs_dirent() looks
like the obvious choice. Comments?

Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-08 Thread Alan Stern
On Thu, 8 Mar 2007, Oliver Neukum wrote:

 Hi,
 
 after a lightning bolt from high above I've been looking into refcounting
 the data structures drivers use to provide the data used to refill sysfs
 buffers. I've come to the following conclusion.
 
 1. struct sysfs_buffer must have a struct kref * and probably a destructor
 pointer
 2. drivers must be able to pass these pointers through an extended
 device_create_file()
 3. Drivers must use refcounting if they want to use attributes
 4. read/write/poll must do refcounting
 
 I am not sure where to store the pointers. struct sysfs_dirent() looks
 like the obvious choice. Comments?

Can you explain the reasoning that led to these conclusions?  And what 
exactly was your lightning bolt?

Alan Stern

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: refcounting drivers' data structures used in sysfs buffers

2007-03-08 Thread Oliver Neukum
Am Donnerstag, 8. März 2007 17:02 schrieb Alan Stern:
 On Thu, 8 Mar 2007, Oliver Neukum wrote:
 
  Hi,
  
  after a lightning bolt from high above I've been looking into refcounting
  the data structures drivers use to provide the data used to refill sysfs
  buffers. I've come to the following conclusion.
  
  1. struct sysfs_buffer must have a struct kref * and probably a destructor
  pointer
  2. drivers must be able to pass these pointers through an extended
  device_create_file()
  3. Drivers must use refcounting if they want to use attributes
  4. read/write/poll must do refcounting
  
  I am not sure where to store the pointers. struct sysfs_dirent() looks
  like the obvious choice. Comments?
 
 Can you explain the reasoning that led to these conclusions?  And what 
 exactly was your lightning bolt?

The old race between disconnect and IO to attribute via sysfs again.
If I cannot disassociate the drivers from the buffers in the buffers, drivers
must not deallocate the data necessary to answer sysfs callbacks while
a buffer exists. Reading from a buffer must up a refcount in the driver's
data structures. The question becomes how to get a pointer to the buffer.
And it cannot live in the dentry as the dentry can go away while files
are still open. This leaves the inode or the buffer.

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


refcounting drivers' data structures used in sysfs buffers

2007-03-08 Thread Oliver Neukum
Hi,

after a lightning bolt from high above I've been looking into refcounting
the data structures drivers use to provide the data used to refill sysfs
buffers. I've come to the following conclusion.

1. struct sysfs_buffer must have a struct kref * and probably a destructor
pointer
2. drivers must be able to pass these pointers through an extended
device_create_file()
3. Drivers must use refcounting if they want to use attributes
4. read/write/poll must do refcounting

I am not sure where to store the pointers. struct sysfs_dirent() looks
like the obvious choice. Comments?

Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/