* [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
@ 2008-08-28 16:36 Tejun Heo
2008-08-28 16:47 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-08-28 16:36 UTC (permalink / raw)
To: Andrew Morton, Greg KH; +Cc: Linux Kernel Mailing List
Add cdev->release() so that cdev can be considered in more involved
object lifetime management. cdev_alloc() used a separate ktype for
auto-free release(). This patch converts it to use cdev->release() so
that there's no need for separate ktype and cdev_init() can be used
for auto-free variant too.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
This one is also for CUSE. Oops, forgot lkml. Resending. Thanks.
fs/char_dev.c | 30 +++++++++++++-----------------
include/linux/cdev.h | 1 +
2 files changed, 14 insertions(+), 17 deletions(-)
diff --git a/fs/char_dev.c b/fs/char_dev.c
index 3cb7cda..3d50199 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -478,26 +478,22 @@ void cdev_del(struct cdev *p)
}
-static void cdev_default_release(struct kobject *kobj)
+static void cdev_release(struct kobject *kobj)
{
struct cdev *p = container_of(kobj, struct cdev, kobj);
cdev_purge(p);
+ if (p->release)
+ p->release(p);
}
-static void cdev_dynamic_release(struct kobject *kobj)
-{
- struct cdev *p = container_of(kobj, struct cdev, kobj);
- cdev_purge(p);
- kfree(p);
-}
-
-static struct kobj_type ktype_cdev_default = {
- .release = cdev_default_release,
+static struct kobj_type cdev_ktype = {
+ .release = cdev_release,
};
-static struct kobj_type ktype_cdev_dynamic = {
- .release = cdev_dynamic_release,
-};
+static void cdev_alloc_release(struct cdev *cdev)
+{
+ kfree(cdev);
+}
/**
* cdev_alloc() - allocate a cdev structure
@@ -506,10 +502,10 @@ static struct kobj_type ktype_cdev_dynamic = {
*/
struct cdev *cdev_alloc(void)
{
- struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
+ struct cdev *p = kmalloc(sizeof(struct cdev), GFP_KERNEL);
if (p) {
- INIT_LIST_HEAD(&p->list);
- kobject_init(&p->kobj, &ktype_cdev_dynamic);
+ cdev_init(p, NULL);
+ p->release = cdev_alloc_release;
}
return p;
}
@@ -526,7 +522,7 @@ void cdev_init(struct cdev *cdev, const struct file_operations *fops)
{
memset(cdev, 0, sizeof *cdev);
INIT_LIST_HEAD(&cdev->list);
- kobject_init(&cdev->kobj, &ktype_cdev_default);
+ kobject_init(&cdev->kobj, &cdev_ktype);
cdev->ops = fops;
}
diff --git a/include/linux/cdev.h b/include/linux/cdev.h
index fb45919..9c3b17e 100644
--- a/include/linux/cdev.h
+++ b/include/linux/cdev.h
@@ -16,6 +16,7 @@ struct cdev {
struct list_head list;
dev_t dev;
unsigned int count;
+ void (*release)(struct cdev *);
};
void cdev_init(struct cdev *, const struct file_operations *);
--
1.5.4.5
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 16:36 [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it Tejun Heo
@ 2008-08-28 16:47 ` Greg KH
2008-08-28 16:56 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2008-08-28 16:47 UTC (permalink / raw)
To: Tejun Heo; +Cc: Andrew Morton, Linux Kernel Mailing List
On Thu, Aug 28, 2008 at 06:36:56PM +0200, Tejun Heo wrote:
> Add cdev->release() so that cdev can be considered in more involved
> object lifetime management. cdev_alloc() used a separate ktype for
> auto-free release(). This patch converts it to use cdev->release() so
> that there's no need for separate ktype and cdev_init() can be used
> for auto-free variant too.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
Ick, I really don't want struct cdev to be used for lifecycle
management, as it is only for major:minor stuff. Why do you want to
make this change?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 16:47 ` Greg KH
@ 2008-08-28 16:56 ` Tejun Heo
2008-08-28 17:38 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-08-28 16:56 UTC (permalink / raw)
To: Greg KH; +Cc: Andrew Morton, Linux Kernel Mailing List
Greg KH wrote:
> Ick, I really don't want struct cdev to be used for lifecycle
> management, as it is only for major:minor stuff. Why do you want to
> make this change?
Well, as cdev can be referenced from userspace, ->release is required
for most purposes. The reason why devices have been getting by without
it is because most chardevs are created on module load and destroyed on
module unload and in the meantime cdev refcount virtually equals module
refcnt, but I'm fairly sure we have cases where cdev can be destroyed
for other reasons then module unloading and it's very likely those cases
are buggy in the current code (backing structure gone bug cdev still
hanging around).
As CUSE can create and destroy devices regardless of module reference
count, it falls in the second category and needs cdev->release() to make
sure the backing structure doesn't go away till cdev is released.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 16:56 ` Tejun Heo
@ 2008-08-28 17:38 ` Greg KH
2008-08-28 17:44 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2008-08-28 17:38 UTC (permalink / raw)
To: Tejun Heo; +Cc: Andrew Morton, Linux Kernel Mailing List
On Thu, Aug 28, 2008 at 06:56:48PM +0200, Tejun Heo wrote:
> Greg KH wrote:
> > Ick, I really don't want struct cdev to be used for lifecycle
> > management, as it is only for major:minor stuff. Why do you want to
> > make this change?
>
> Well, as cdev can be referenced from userspace, ->release is required
> for most purposes. The reason why devices have been getting by without
> it is because most chardevs are created on module load and destroyed on
> module unload and in the meantime cdev refcount virtually equals module
> refcnt, but I'm fairly sure we have cases where cdev can be destroyed
> for other reasons then module unloading and it's very likely those cases
> are buggy in the current code (backing structure gone bug cdev still
> hanging around).
Hm, I thought Al covered that when he created the cdev interface, I
would be a bit supprised if this was the case.
> As CUSE can create and destroy devices regardless of module reference
> count, it falls in the second category and needs cdev->release() to make
> sure the backing structure doesn't go away till cdev is released.
But CUSE should be it's own module, right? And it would "own" the cdev,
so the module and cdev count should be fine and matching. The userspace
code could go away but the CUSE code should handle that with a different
reference count. This is the way that hardware drivers handle the
issue.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 17:38 ` Greg KH
@ 2008-08-28 17:44 ` Tejun Heo
2008-08-28 17:48 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-08-28 17:44 UTC (permalink / raw)
To: Greg KH; +Cc: Andrew Morton, Linux Kernel Mailing List
Greg KH wrote:
> On Thu, Aug 28, 2008 at 06:56:48PM +0200, Tejun Heo wrote:
>> Greg KH wrote:
>>> Ick, I really don't want struct cdev to be used for lifecycle
>>> management, as it is only for major:minor stuff. Why do you want to
>>> make this change?
>> Well, as cdev can be referenced from userspace, ->release is required
>> for most purposes. The reason why devices have been getting by without
>> it is because most chardevs are created on module load and destroyed on
>> module unload and in the meantime cdev refcount virtually equals module
>> refcnt, but I'm fairly sure we have cases where cdev can be destroyed
>> for other reasons then module unloading and it's very likely those cases
>> are buggy in the current code (backing structure gone bug cdev still
>> hanging around).
>
> Hm, I thought Al covered that when he created the cdev interface, I
> would be a bit supprised if this was the case.
Hmmm.... I've never actually audited the code so... it could be that no
chardev is created and destroyed that way, I guess.
>> As CUSE can create and destroy devices regardless of module reference
>> count, it falls in the second category and needs cdev->release() to make
>> sure the backing structure doesn't go away till cdev is released.
>
> But CUSE should be it's own module, right? And it would "own" the cdev,
> so the module and cdev count should be fine and matching. The userspace
> code could go away but the CUSE code should handle that with a different
> reference count. This is the way that hardware drivers handle the
> issue.
The problem is not the device to talk to CUSE (/dev/cuse as in
/dev/fuse), for which module refcount and device refcount match fine.
But the whole point of CUSE is allowing CUSE clients to create arbitrary
character devices, so in addition to /dev/cuse which clients use to talk
to CUSE, CUSE hosts character devices for its clients and they come and
go dynamically and thus requires proper lifetime management.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 17:44 ` Tejun Heo
@ 2008-08-28 17:48 ` Greg KH
2008-08-28 17:55 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2008-08-28 17:48 UTC (permalink / raw)
To: Tejun Heo; +Cc: Andrew Morton, Linux Kernel Mailing List
On Thu, Aug 28, 2008 at 07:44:55PM +0200, Tejun Heo wrote:
> Greg KH wrote:
> > On Thu, Aug 28, 2008 at 06:56:48PM +0200, Tejun Heo wrote:
> >> Greg KH wrote:
> >>> Ick, I really don't want struct cdev to be used for lifecycle
> >>> management, as it is only for major:minor stuff. Why do you want to
> >>> make this change?
> >> Well, as cdev can be referenced from userspace, ->release is required
> >> for most purposes. The reason why devices have been getting by without
> >> it is because most chardevs are created on module load and destroyed on
> >> module unload and in the meantime cdev refcount virtually equals module
> >> refcnt, but I'm fairly sure we have cases where cdev can be destroyed
> >> for other reasons then module unloading and it's very likely those cases
> >> are buggy in the current code (backing structure gone bug cdev still
> >> hanging around).
> >
> > Hm, I thought Al covered that when he created the cdev interface, I
> > would be a bit supprised if this was the case.
>
> Hmmm.... I've never actually audited the code so... it could be that no
> chardev is created and destroyed that way, I guess.
>
> >> As CUSE can create and destroy devices regardless of module reference
> >> count, it falls in the second category and needs cdev->release() to make
> >> sure the backing structure doesn't go away till cdev is released.
> >
> > But CUSE should be it's own module, right? And it would "own" the cdev,
> > so the module and cdev count should be fine and matching. The userspace
> > code could go away but the CUSE code should handle that with a different
> > reference count. This is the way that hardware drivers handle the
> > issue.
>
> The problem is not the device to talk to CUSE (/dev/cuse as in
> /dev/fuse), for which module refcount and device refcount match fine.
> But the whole point of CUSE is allowing CUSE clients to create arbitrary
> character devices, so in addition to /dev/cuse which clients use to talk
> to CUSE, CUSE hosts character devices for its clients and they come and
> go dynamically and thus requires proper lifetime management.
That's fine, it's just like a USB device that uses a cdev, right? Or a
PCI device, or any other type of device that can come and go independant
of the cdev or module lifespan.
So, you tie the cdev lifespan to the module lifespan with the
MODULE_OWNER in the file operations. As the cuse.ko module will own the
cdev, it can handle that reference, and it can not be removed as long as
userspace has the cdev open, right?
It really isn't any different from any other type of removable device
(i.e. any type of device...)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 17:48 ` Greg KH
@ 2008-08-28 17:55 ` Tejun Heo
2008-08-28 18:17 ` Greg KH
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-08-28 17:55 UTC (permalink / raw)
To: Greg KH; +Cc: Andrew Morton, Linux Kernel Mailing List
Greg KH wrote:
>> The problem is not the device to talk to CUSE (/dev/cuse as in
>> /dev/fuse), for which module refcount and device refcount match fine.
>> But the whole point of CUSE is allowing CUSE clients to create arbitrary
>> character devices, so in addition to /dev/cuse which clients use to talk
>> to CUSE, CUSE hosts character devices for its clients and they come and
>> go dynamically and thus requires proper lifetime management.
>
> That's fine, it's just like a USB device that uses a cdev, right? Or a
> PCI device, or any other type of device that can come and go independant
> of the cdev or module lifespan.
>
> So, you tie the cdev lifespan to the module lifespan with the
> MODULE_OWNER in the file operations. As the cuse.ko module will own the
> cdev, it can handle that reference, and it can not be removed as long as
> userspace has the cdev open, right?
>
> It really isn't any different from any other type of removable device
> (i.e. any type of device...)
Ah.. right, but taking cdev refcount out of the picture requires adding
'severing' operation on cdev f_ops, which certainly is doable but isn't
it just cleaner to make cdev follow the usual lifetime management rules?
An object which is referenced counted requires ->release if it's gonna
be used in any non-simplistic way.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 17:55 ` Tejun Heo
@ 2008-08-28 18:17 ` Greg KH
2008-11-13 8:58 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Greg KH @ 2008-08-28 18:17 UTC (permalink / raw)
To: Tejun Heo; +Cc: Andrew Morton, Linux Kernel Mailing List
On Thu, Aug 28, 2008 at 07:55:39PM +0200, Tejun Heo wrote:
> Greg KH wrote:
> >> The problem is not the device to talk to CUSE (/dev/cuse as in
> >> /dev/fuse), for which module refcount and device refcount match fine.
> >> But the whole point of CUSE is allowing CUSE clients to create arbitrary
> >> character devices, so in addition to /dev/cuse which clients use to talk
> >> to CUSE, CUSE hosts character devices for its clients and they come and
> >> go dynamically and thus requires proper lifetime management.
> >
> > That's fine, it's just like a USB device that uses a cdev, right? Or a
> > PCI device, or any other type of device that can come and go independant
> > of the cdev or module lifespan.
> >
> > So, you tie the cdev lifespan to the module lifespan with the
> > MODULE_OWNER in the file operations. As the cuse.ko module will own the
> > cdev, it can handle that reference, and it can not be removed as long as
> > userspace has the cdev open, right?
> >
> > It really isn't any different from any other type of removable device
> > (i.e. any type of device...)
>
> Ah.. right, but taking cdev refcount out of the picture requires adding
> 'severing' operation on cdev f_ops, which certainly is doable but isn't
> it just cleaner to make cdev follow the usual lifetime management rules?
> An object which is referenced counted requires ->release if it's gonna
> be used in any non-simplistic way.
Yes, but as you "normally" tie the cdev to the module itself, that
handles the lifetime rules.
Now I really dont' like the current cdev interface, it's a bit too
complex as it needed to support the old-style interfaces without any
build time changes, but I think your change isn't needed as somehow all
the current drivers that support dynamic devices don't need it.
Actually, the kobject in cdev shouldn't be an kobject, it's not used for
registering with sysfs at all, it should just be a kref. I sweep the
tree for code that sets the name of the cdev every few months as people
don't seem to realize this :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-08-28 18:17 ` Greg KH
@ 2008-11-13 8:58 ` Tejun Heo
[not found] ` <492136F5.8010903@kernel.org>
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-11-13 8:58 UTC (permalink / raw)
To: Greg KH; +Cc: Andrew Morton, Linux Kernel Mailing List
Hello, Greg.
I'm now trying to convert the cdev thing for CUSE and it just ain't
feel right.
Greg KH wrote:
>> Ah.. right, but taking cdev refcount out of the picture requires adding
>> 'severing' operation on cdev f_ops, which certainly is doable but isn't
>> it just cleaner to make cdev follow the usual lifetime management rules?
>> An object which is referenced counted requires ->release if it's gonna
>> be used in any non-simplistic way.
>
> Yes, but as you "normally" tie the cdev to the module itself, that
> handles the lifetime rules.
That "normally" is from the days when only a single or fixed number of
devices are associated with single module or driver.
> Now I really dont' like the current cdev interface, it's a bit too
> complex as it needed to support the old-style interfaces without any
> build time changes, but I think your change isn't needed as somehow all
> the current drivers that support dynamic devices don't need it.
Yes, it requires all drivers to have global device table and check
whether the device is still available at ->open. For most, drivers
usually have certain fixed number of devices which can directly be
indexed with minor. For CUSE, it gotta be a hash table or a b-tree.
I don't really see any point in not adding ->release. Time has
changed and everybody is playing with reference counts and ->release
methods. Plus, cdev_alloc() interface is half-baked anyway (no free
function for cases where cdev_add() fails, drivers call cdev_del() in
those cases risking unregistering other driver's map). It's perfectly
okay to keep it around for compatibility but there just is no reason
to cling to it.
> Actually, the kobject in cdev shouldn't be an kobject, it's not used for
> registering with sysfs at all, it should just be a kref. I sweep the
> tree for code that sets the name of the cdev every few months as people
> don't seem to realize this :)
Heh... CUSE did that too. Removing it.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
[not found] ` <49221CE8.1050402@kernel.org>
@ 2008-11-18 1:40 ` Tejun Heo
2008-11-18 13:44 ` Boaz Harrosh
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-11-18 1:40 UTC (permalink / raw)
To: Greg KH, Linux Kernel
(lkml was missing, adding it back and quoting whole body for reference)
Tejun Heo wrote:
> Greg KH wrote:
>> On Mon, Nov 17, 2008 at 06:18:45PM +0900, Tejun Heo wrote:
>>> (this is a private message)
>>>
>>> PING. :-)
>> Heh.
>>
>> I've lost this out of my tree, as I didn't know you still wanted this
>> applied. If you need it for your char-fuse code, then I don't have an
>> objection to it, and you can take it through your tree.
>>
>> Or do you want me to take it through mine?
>>
>> Your choice :)
>
> I'll push it through Miklos' tree w/ your ACK then.
>
> Thanks. :-)
>
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-11-18 1:40 ` Tejun Heo
@ 2008-11-18 13:44 ` Boaz Harrosh
2008-11-18 15:58 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Boaz Harrosh @ 2008-11-18 13:44 UTC (permalink / raw)
To: Tejun Heo; +Cc: Greg KH, Linux Kernel
Tejun Heo wrote:
> (lkml was missing, adding it back and quoting whole body for reference)
>
> Tejun Heo wrote:
>> Greg KH wrote:
>>> On Mon, Nov 17, 2008 at 06:18:45PM +0900, Tejun Heo wrote:
>>>> (this is a private message)
>>>>
>>>> PING. :-)
>>> Heh.
>>>
>>> I've lost this out of my tree, as I didn't know you still wanted this
>>> applied. If you need it for your char-fuse code, then I don't have an
>>> objection to it, and you can take it through your tree.
>>>
>>> Or do you want me to take it through mine?
>>>
>>> Your choice :)
>> I'll push it through Miklos' tree w/ your ACK then.
>>
>> Thanks. :-)
>>
>
>
Tejun Hi.
I just saw this thread for the first time and it left me confused.
What was the final verdict. Is this patch going in at the end?
Which incarnation of it? is there a public git tree I can try?
The reason I ask is because I have just the same principal work in one of
my test trees. What I have is a Filesystem, osdfs, that is mounted
on an OSD scsi-device, which is a char-device. Now the osdfs when mounting
an OSD device does not use __open, like user mode it needs some kernel
reference counting to keep the char-device up. On the other hand
the actual teardown and unmap of the char-device is done from the scsi-ml
remove vector. So just like in sd, sr and other scsi ULDs I need to unmap
the device but keep the memory allocated and available until the last reference.
All this is usually done using the Release() of the block-device. But for me
I only have a char-device. Currently what I had to do is keep another kref
to govern the device's lifecycle and sync every thing together. A Release() at
the char-dev would let me reuse what's there and let me clean all that code up.
While Investigating the problem and compering what was done on the block-device
side, I've seen more then a few places that private reference counting could be
dropped completely, and the char-dev could be used. Off my head some of these places
are:
- UBI used by UBIFS
- sg.c which does not have a Kernel user but needs it's char device until
scsi-Remove and/or __close()
and other places as well.
Thanks for doing this
Boaz
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-11-18 13:44 ` Boaz Harrosh
@ 2008-11-18 15:58 ` Tejun Heo
2008-11-19 17:59 ` Miklos Szeredi
2008-11-20 5:54 ` Greg KH
0 siblings, 2 replies; 16+ messages in thread
From: Tejun Heo @ 2008-11-18 15:58 UTC (permalink / raw)
To: Boaz Harrosh; +Cc: Greg KH, Linux Kernel, Miklos Szeredi
Hello,
Boaz Harrosh wrote:
> I just saw this thread for the first time and it left me confused.
> What was the final verdict. Is this patch going in at the end?
> Which incarnation of it? is there a public git tree I can try?
Yes it is and as posted and no there isn't no public git tree yet, I was
planning on pushing it through Miklos' tree.
> The reason I ask is because I have just the same principal work in one of
> my test trees. What I have is a Filesystem, osdfs, that is mounted
> on an OSD scsi-device, which is a char-device. Now the osdfs when mounting
> an OSD device does not use __open, like user mode it needs some kernel
> reference counting to keep the char-device up. On the other hand
> the actual teardown and unmap of the char-device is done from the scsi-ml
> remove vector. So just like in sd, sr and other scsi ULDs I need to unmap
> the device but keep the memory allocated and available until the last reference.
> All this is usually done using the Release() of the block-device. But for me
> I only have a char-device. Currently what I had to do is keep another kref
> to govern the device's lifecycle and sync every thing together. A Release() at
> the char-dev would let me reuse what's there and let me clean all that code up.
>
> While Investigating the problem and compering what was done on the block-device
> side, I've seen more then a few places that private reference counting could be
> dropped completely, and the char-dev could be used. Off my head some of these places
> are:
> - UBI used by UBIFS
> - sg.c which does not have a Kernel user but needs it's char device until
> scsi-Remove and/or __close()
>
> and other places as well.
Hmmm... if you can use this change, I think you can push it through
whatever tree you push patches through, things can be taken care of when
merging or we can put it in Greg's tree and ask Miklos to pull from the
tree first but Greg doesn't keep a stable tree. Hmm... if your patches
are gonna go through some public tree, I think we can ask Miklos pull
from the tree too. Guys, what do you think?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-11-18 15:58 ` Tejun Heo
@ 2008-11-19 17:59 ` Miklos Szeredi
2008-11-20 5:54 ` Greg KH
1 sibling, 0 replies; 16+ messages in thread
From: Miklos Szeredi @ 2008-11-19 17:59 UTC (permalink / raw)
To: tj; +Cc: bharrosh, greg, linux-kernel, miklos
On Wed, 19 Nov 2008, Tejun Heo wrote:
> Hmmm... if you can use this change, I think you can push it through
> whatever tree you push patches through, things can be taken care of when
> merging or we can put it in Greg's tree and ask Miklos to pull from the
> tree first but Greg doesn't keep a stable tree. Hmm... if your patches
> are gonna go through some public tree, I think we can ask Miklos pull
> from the tree too. Guys, what do you think?
Sure, since this is not fuse specific, it better not go through my
tree anyway.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-11-18 15:58 ` Tejun Heo
2008-11-19 17:59 ` Miklos Szeredi
@ 2008-11-20 5:54 ` Greg KH
2008-11-20 6:28 ` Tejun Heo
1 sibling, 1 reply; 16+ messages in thread
From: Greg KH @ 2008-11-20 5:54 UTC (permalink / raw)
To: Tejun Heo; +Cc: Boaz Harrosh, Linux Kernel, Miklos Szeredi
On Wed, Nov 19, 2008 at 12:58:26AM +0900, Tejun Heo wrote:
> Hello,
>
> Boaz Harrosh wrote:
> > I just saw this thread for the first time and it left me confused.
> > What was the final verdict. Is this patch going in at the end?
> > Which incarnation of it? is there a public git tree I can try?
>
> Yes it is and as posted and no there isn't no public git tree yet, I was
> planning on pushing it through Miklos' tree.
>
> > The reason I ask is because I have just the same principal work in one of
> > my test trees. What I have is a Filesystem, osdfs, that is mounted
> > on an OSD scsi-device, which is a char-device. Now the osdfs when mounting
> > an OSD device does not use __open, like user mode it needs some kernel
> > reference counting to keep the char-device up. On the other hand
> > the actual teardown and unmap of the char-device is done from the scsi-ml
> > remove vector. So just like in sd, sr and other scsi ULDs I need to unmap
> > the device but keep the memory allocated and available until the last reference.
> > All this is usually done using the Release() of the block-device. But for me
> > I only have a char-device. Currently what I had to do is keep another kref
> > to govern the device's lifecycle and sync every thing together. A Release() at
> > the char-dev would let me reuse what's there and let me clean all that code up.
> >
> > While Investigating the problem and compering what was done on the block-device
> > side, I've seen more then a few places that private reference counting could be
> > dropped completely, and the char-dev could be used. Off my head some of these places
> > are:
> > - UBI used by UBIFS
> > - sg.c which does not have a Kernel user but needs it's char device until
> > scsi-Remove and/or __close()
> >
> > and other places as well.
>
> Hmmm... if you can use this change, I think you can push it through
> whatever tree you push patches through, things can be taken care of when
> merging or we can put it in Greg's tree and ask Miklos to pull from the
> tree first but Greg doesn't keep a stable tree.
I don't keep a stable tree? What do you mean? It's a quilt tree, and
it does get included in -next, so I think it does work :)
Send it to me again and I'll push it through my tree if no one else
objects.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-11-20 5:54 ` Greg KH
@ 2008-11-20 6:28 ` Tejun Heo
2008-11-20 11:45 ` Tejun Heo
0 siblings, 1 reply; 16+ messages in thread
From: Tejun Heo @ 2008-11-20 6:28 UTC (permalink / raw)
To: Greg KH; +Cc: Boaz Harrosh, Linux Kernel, Miklos Szeredi
Greg KH wrote:
>> Hmmm... if you can use this change, I think you can push it through
>> whatever tree you push patches through, things can be taken care of when
>> merging or we can put it in Greg's tree and ask Miklos to pull from the
>> tree first but Greg doesn't keep a stable tree.
>
> I don't keep a stable tree? What do you mean? It's a quilt tree, and
> it does get included in -next, so I think it does work :)
Heh... I meant stable (in the sense that it doesn't get rebased) git
tree which Miklos can pull from.
> Send it to me again and I'll push it through my tree if no one else
> objects.
Yeap, will resend it and let you and Miklos sort it out.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it
2008-11-20 6:28 ` Tejun Heo
@ 2008-11-20 11:45 ` Tejun Heo
0 siblings, 0 replies; 16+ messages in thread
From: Tejun Heo @ 2008-11-20 11:45 UTC (permalink / raw)
To: Greg KH; +Cc: Boaz Harrosh, Linux Kernel, Miklos Szeredi
Add cdev->release() so that cdev can be considered in more involved
object lifetime management. cdev_alloc() used a separate ktype for
auto-free release(). This patch converts it to use cdev->release() so
that there's no need for separate ktype and cdev_init() can be used
for auto-free variant too.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
fs/char_dev.c | 30 +++++++++++++-----------------
include/linux/cdev.h | 1 +
2 files changed, 14 insertions(+), 17 deletions(-)
Index: work/fs/char_dev.c
===================================================================
--- work.orig/fs/char_dev.c
+++ work/fs/char_dev.c
@@ -482,26 +482,22 @@ void cdev_del(struct cdev *p)
}
-static void cdev_default_release(struct kobject *kobj)
+static void cdev_release(struct kobject *kobj)
{
struct cdev *p = container_of(kobj, struct cdev, kobj);
cdev_purge(p);
+ if (p->release)
+ p->release(p);
}
-static void cdev_dynamic_release(struct kobject *kobj)
-{
- struct cdev *p = container_of(kobj, struct cdev, kobj);
- cdev_purge(p);
- kfree(p);
-}
-
-static struct kobj_type ktype_cdev_default = {
- .release = cdev_default_release,
+static struct kobj_type cdev_ktype = {
+ .release = cdev_release,
};
-static struct kobj_type ktype_cdev_dynamic = {
- .release = cdev_dynamic_release,
-};
+static void cdev_alloc_release(struct cdev *cdev)
+{
+ kfree(cdev);
+}
/**
* cdev_alloc() - allocate a cdev structure
@@ -510,10 +506,10 @@ static struct kobj_type ktype_cdev_dynam
*/
struct cdev *cdev_alloc(void)
{
- struct cdev *p = kzalloc(sizeof(struct cdev), GFP_KERNEL);
+ struct cdev *p = kmalloc(sizeof(struct cdev), GFP_KERNEL);
if (p) {
- INIT_LIST_HEAD(&p->list);
- kobject_init(&p->kobj, &ktype_cdev_dynamic);
+ cdev_init(p, NULL);
+ p->release = cdev_alloc_release;
}
return p;
}
@@ -530,7 +526,7 @@ void cdev_init(struct cdev *cdev, const
{
memset(cdev, 0, sizeof *cdev);
INIT_LIST_HEAD(&cdev->list);
- kobject_init(&cdev->kobj, &ktype_cdev_default);
+ kobject_init(&cdev->kobj, &cdev_ktype);
cdev->ops = fops;
}
Index: work/include/linux/cdev.h
===================================================================
--- work.orig/include/linux/cdev.h
+++ work/include/linux/cdev.h
@@ -16,6 +16,7 @@ struct cdev {
struct list_head list;
dev_t dev;
unsigned int count;
+ void (*release)(struct cdev *);
};
void cdev_init(struct cdev *, const struct file_operations *);
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2008-11-20 11:46 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-28 16:36 [PATCH RESEND] char_dev: add cdev->release() and convert cdev_alloc() to use it Tejun Heo
2008-08-28 16:47 ` Greg KH
2008-08-28 16:56 ` Tejun Heo
2008-08-28 17:38 ` Greg KH
2008-08-28 17:44 ` Tejun Heo
2008-08-28 17:48 ` Greg KH
2008-08-28 17:55 ` Tejun Heo
2008-08-28 18:17 ` Greg KH
2008-11-13 8:58 ` Tejun Heo
[not found] ` <492136F5.8010903@kernel.org>
[not found] ` <20081117171717.GB31306@kroah.com>
[not found] ` <49221CE8.1050402@kernel.org>
2008-11-18 1:40 ` Tejun Heo
2008-11-18 13:44 ` Boaz Harrosh
2008-11-18 15:58 ` Tejun Heo
2008-11-19 17:59 ` Miklos Szeredi
2008-11-20 5:54 ` Greg KH
2008-11-20 6:28 ` Tejun Heo
2008-11-20 11:45 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox