Re: refcounting drivers' data structures used in sysfs buffers
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&m=117355959020831&w=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
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
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
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
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
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&m=117355959020831&w=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
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&m=117355959020831&w=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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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/
