Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
One more thing... On 4/21/05, Evgeniy Polyakov <[EMAIL PROTECTED]> wrote: > On Thu, 2005-04-21 at 02:07 -0500, Dmitry Torokhov wrote: > > > w1-master-drop-attrs.patch > >Get rid of unneeded master device attributes: > >- 'pointer' and 'attempts' are meaningless for userspace; > >- information provided by 'slaves' and 'slave_count' can be gathered > > from other sysfs bits; > >- w1_slave_found has to be rearranged now that slave_count field is gone. > > attempts is usefull for broken lines. It simply increments with every search i.e. every 10 secondsby default and does not provide indication of the quality of the wire. -- 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: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
Hi Evgeniy,
On 4/21/05, Evgeniy Polyakov <[EMAIL PROTECTED]> wrote:
> On Thu, 2005-04-21 at 02:07 -0500, Dmitry Torokhov wrote:
> > Hi,
>
> Hello, Dmitry.
>
> > I happened to take a look into drivers/w1 and found there bunch of thigs
> > that IMO should be changed:
> >
> > - custom-made refcounting is racy
>
> Why do you think so?
> Did you find exactly the place which races against something?
>
> > - lifetime rules need to be better enforced
>
> Hmm, I misunderstand you.
>
Consider thie following:
451 while (atomic_read(>refcnt)) {
452 printk(KERN_INFO "Waiting for %s to become free:
refcnt=%d.\n",
453 sl->name, atomic_read(>refcnt));
454
455 if (msleep_interruptible(1000))
456 flush_signals(current);
457 }
458
459 sysfs_remove_bin_file (>dev.kobj, >attr_bin);
460 device_remove_file(>dev, >attr_name);
461 device_remove_file(>dev, >attr_val);
462 device_unregister(>dev);
463 w1_family_put(sl->family);
.. And caller does kfree(sl);
Now, if application opens slave's sysfs attribute while other thread
exited the loop and is about to remove attributes, then you will kfree
object that is in use and who knows what will happen. This is example
of both refcounting being racey and lifetime rules being violated.
> > - family framework is insufficient for many advanced w1 devices
>
> No, family framework is just indication which family is used.
> Feel free to implement additional methods for various devices
> and store them in driver's private areas like ipaq does.
>
OK, that is what I am aying. But why do you need that attribute with
variable name and a bin attribute that is not really bin but just a
dump for all kind of data (looks like debug one).
> > - custom-made hotplug notification over netlink should be removed in favor
> > of standard hotplug notification
>
> It is not hotplug, and your changes broke it completely.
> I'm waiting for connector to be included or discarded, so I can move
> w1 on top of it's interface or move connector's bits into the w1
> subsystem.
>
You will not be able to cram all 1-wire devices into unified
interface. You will need to build classes on top of it and you might
use connector (I am not sure) bit not on w1 bus level.
...
> > w1-slave-attr-group.patc
> >Add 2 default attributes "family" and "serial" to slave devices, every
> >1-Wire slave has them. Use attribute_group to handle. The rest of slave
> >attributes are left as is - will be dealt with later.
>
> Serial number is already there in bus_id, family is there too.
> Why do you need separate files?
Yeah, could probably drop them.
...
> > w1-drop-owner.patch
> >Drop owner field from w1_master and w1_slave structures. Just having it
> >there does not magically fixes lifetime rules.
>
> They do not even pretend, I still do not understand what is "lifetime
> rules"?
So there is no point in having them, right?
>
> > w1-bus-ops.patch
> >Cleanup bus operations code:
> >- have bus operatiions accept w1_master instead of unsigned long and
> > drop data field from w1_bus_master so the structure can be statically
> > allocated by driver implementing it;
> >- rename w1_bus_master to w1_bus_ops to avoid confusion with w1_master;
> >- separate master registering and allocation so drivers can setup proper
> > link between private data and master and set useable master's name.
>
> I strongly object against such changes.
> 1. w1 was designed in the way that w1 bus master drivers do not
> know about other w1 world. It is very simple and very low-level
> abstraction,
> that only understands how to do low-level functions. It is not needed
> do know about w1_master structure and even about it's existence.
Well, it does need to know about w1_bus_master structure, which is
pretty much the same. And it allows having static bus_ops allocation
and removes need for casting from unsigned longs...
> 2. All renaming are superfluous, I'm not against it, but completely do
> not
> understand it's merits.
Because now it represents operations only, data field has been
dropped. I my head hurt when I see w1_master and w1_bus_muster
together as 2 separate objects both representing the same piece of
hardware.
> 3. You broke netlink allocation routing - it may fail and it is not
> fatal.
Because it is going away in later patcehs ;)
>
> > w1-fold-w1-int.patch
> >Fold w1_int.c into w1.c - there is no point in artificially separating
> >code for master devices between 2 files.
>
> w1_int.c was created to store external interface implementation,
> why do you want to move it into w1 core code?
> It will only soil the code...
Because I do not understand why code creating master devices is
separate from code creating master device's attributes.
>
> > w1-drop-netlink.patch
> >Drop custom-made hotplug over netlink notification from
Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
On Thu, 2005-04-21 at 02:07 -0500, Dmitry Torokhov wrote: > Hi, Hello, Dmitry. > I happened to take a look into drivers/w1 and found there bunch of thigs > that IMO should be changed: > > - custom-made refcounting is racy Why do you think so? Did you find exactly the place which races against something? > - lifetime rules need to be better enforced Hmm, I misunderstand you. > - family framework is insufficient for many advanced w1 devices No, family framework is just indication which family is used. Feel free to implement additional methods for various devices and store them in driver's private areas like ipaq does. > - custom-made hotplug notification over netlink should be removed in favor > of standard hotplug notification It is not hotplug, and your changes broke it completely. I'm waiting for connector to be included or discarded, so I can move w1 on top of it's interface or move connector's bits into the w1 subsystem. > - sysfs attributes have unnecessary prefixes (like w1_master) or not needed > at all (w1_master_pointer) Yep, some of them can be suspicious from the first point of view. > Please consider series of patches below. Unfortunately I do not have any W1 > equipment so it was only compile-tested. Please also note that lifetime and > locking rules were changed on object-by-object base so mid-series some stuff > may appear broken but as far as I can see the end result shoudl work pretty > well. I can test your changes next week, but I already have too many objections to apply the whole patch set. > w1-whitespace.patch >Whitespace fixes. > > w1-formatting.patch >Some formatting changes to bring the code in line with CodingStyle >guidelines. Both above look ok, although they will not apply after patches already sent to Greg but not yet in the tree are applied. > w1-master-attr-group.patch >Use attribute_group to create master device attributes to guarantee >proper cleanup in case of failure. Also, hide most of attribute define >ugliness in macros. Yep, I like it. > w1-slave-attr-group.patc >Add 2 default attributes "family" and "serial" to slave devices, every >1-Wire slave has them. Use attribute_group to handle. The rest of slave >attributes are left as is - will be dealt with later. Serial number is already there in bus_id, family is there too. Why do you need separate files? > w1-lists-cleanup.patch >List handling cleanup. Most of the list_for_each_safe users don't need >*_safe variant, *_entry variant is better suited in most places. Also, >checking retrieved list element for null is a bit pointless... Yep, it is correct. > w1-drop-owner.patch >Drop owner field from w1_master and w1_slave structures. Just having it >there does not magically fixes lifetime rules. They do not even pretend, I still do not understand what is "lifetime rules"? > w1-bus-ops.patch >Cleanup bus operations code: >- have bus operatiions accept w1_master instead of unsigned long and > drop data field from w1_bus_master so the structure can be statically > allocated by driver implementing it; >- rename w1_bus_master to w1_bus_ops to avoid confusion with w1_master; >- separate master registering and allocation so drivers can setup proper > link between private data and master and set useable master's name. I strongly object against such changes. 1. w1 was designed in the way that w1 bus master drivers do not know about other w1 world. It is very simple and very low-level abstraction, that only understands how to do low-level functions. It is not needed do know about w1_master structure and even about it's existence. 2. All renaming are superfluous, I'm not against it, but completely do not understand it's merits. 3. You broke netlink allocation routing - it may fail and it is not fatal. > w1-fold-w1-int.patch >Fold w1_int.c into w1.c - there is no point in artificially separating >code for master devices between 2 files. w1_int.c was created to store external interface implementation, why do you want to move it into w1 core code? It will only soil the code... > w1-drop-netlink.patch >Drop custom-made hotplug over netlink notification from w1 core. >Standard hotplug mechanism should work just fine (patch will follow). netlink notification was not created for hotplug. Also I'm against w1 hotplug support, since hotplug is quite rarely used in embedded platforms where the majority of w1 devices live. I mean not to completely forget this idea, but implement it in the way it will not hurt existing model. > w1-drop-control-thread.patch >Drop control thread from w1 core, whatever it does can also be done in >the context of w1_remove_master_device. Also, pin the module when >registering new master device to make sure that w1 core is not unloaded >until last device is gone. This simplifies logic a lot. Why do you think master can be removed in safe context only? I have
Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
On Thu, 2005-04-21 at 02:07 -0500, Dmitry Torokhov wrote: Hi, Hello, Dmitry. I happened to take a look into drivers/w1 and found there bunch of thigs that IMO should be changed: - custom-made refcounting is racy Why do you think so? Did you find exactly the place which races against something? - lifetime rules need to be better enforced Hmm, I misunderstand you. - family framework is insufficient for many advanced w1 devices No, family framework is just indication which family is used. Feel free to implement additional methods for various devices and store them in driver's private areas like ipaq does. - custom-made hotplug notification over netlink should be removed in favor of standard hotplug notification It is not hotplug, and your changes broke it completely. I'm waiting for connector to be included or discarded, so I can move w1 on top of it's interface or move connector's bits into the w1 subsystem. - sysfs attributes have unnecessary prefixes (like w1_master) or not needed at all (w1_master_pointer) Yep, some of them can be suspicious from the first point of view. Please consider series of patches below. Unfortunately I do not have any W1 equipment so it was only compile-tested. Please also note that lifetime and locking rules were changed on object-by-object base so mid-series some stuff may appear broken but as far as I can see the end result shoudl work pretty well. I can test your changes next week, but I already have too many objections to apply the whole patch set. w1-whitespace.patch Whitespace fixes. w1-formatting.patch Some formatting changes to bring the code in line with CodingStyle guidelines. Both above look ok, although they will not apply after patches already sent to Greg but not yet in the tree are applied. w1-master-attr-group.patch Use attribute_group to create master device attributes to guarantee proper cleanup in case of failure. Also, hide most of attribute define ugliness in macros. Yep, I like it. w1-slave-attr-group.patc Add 2 default attributes family and serial to slave devices, every 1-Wire slave has them. Use attribute_group to handle. The rest of slave attributes are left as is - will be dealt with later. Serial number is already there in bus_id, family is there too. Why do you need separate files? w1-lists-cleanup.patch List handling cleanup. Most of the list_for_each_safe users don't need *_safe variant, *_entry variant is better suited in most places. Also, checking retrieved list element for null is a bit pointless... Yep, it is correct. w1-drop-owner.patch Drop owner field from w1_master and w1_slave structures. Just having it there does not magically fixes lifetime rules. They do not even pretend, I still do not understand what is lifetime rules? w1-bus-ops.patch Cleanup bus operations code: - have bus operatiions accept w1_master instead of unsigned long and drop data field from w1_bus_master so the structure can be statically allocated by driver implementing it; - rename w1_bus_master to w1_bus_ops to avoid confusion with w1_master; - separate master registering and allocation so drivers can setup proper link between private data and master and set useable master's name. I strongly object against such changes. 1. w1 was designed in the way that w1 bus master drivers do not know about other w1 world. It is very simple and very low-level abstraction, that only understands how to do low-level functions. It is not needed do know about w1_master structure and even about it's existence. 2. All renaming are superfluous, I'm not against it, but completely do not understand it's merits. 3. You broke netlink allocation routing - it may fail and it is not fatal. w1-fold-w1-int.patch Fold w1_int.c into w1.c - there is no point in artificially separating code for master devices between 2 files. w1_int.c was created to store external interface implementation, why do you want to move it into w1 core code? It will only soil the code... w1-drop-netlink.patch Drop custom-made hotplug over netlink notification from w1 core. Standard hotplug mechanism should work just fine (patch will follow). netlink notification was not created for hotplug. Also I'm against w1 hotplug support, since hotplug is quite rarely used in embedded platforms where the majority of w1 devices live. I mean not to completely forget this idea, but implement it in the way it will not hurt existing model. w1-drop-control-thread.patch Drop control thread from w1 core, whatever it does can also be done in the context of w1_remove_master_device. Also, pin the module when registering new master device to make sure that w1 core is not unloaded until last device is gone. This simplifies logic a lot. Why do you think master can be removed in safe context only? I have feature requests for both adding/removing and exporting master
Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
Hi Evgeniy,
On 4/21/05, Evgeniy Polyakov [EMAIL PROTECTED] wrote:
On Thu, 2005-04-21 at 02:07 -0500, Dmitry Torokhov wrote:
Hi,
Hello, Dmitry.
I happened to take a look into drivers/w1 and found there bunch of thigs
that IMO should be changed:
- custom-made refcounting is racy
Why do you think so?
Did you find exactly the place which races against something?
- lifetime rules need to be better enforced
Hmm, I misunderstand you.
Consider thie following:
451 while (atomic_read(sl-refcnt)) {
452 printk(KERN_INFO Waiting for %s to become free:
refcnt=%d.\n,
453 sl-name, atomic_read(sl-refcnt));
454
455 if (msleep_interruptible(1000))
456 flush_signals(current);
457 }
458
459 sysfs_remove_bin_file (sl-dev.kobj, sl-attr_bin);
460 device_remove_file(sl-dev, sl-attr_name);
461 device_remove_file(sl-dev, sl-attr_val);
462 device_unregister(sl-dev);
463 w1_family_put(sl-family);
.. And caller does kfree(sl);
Now, if application opens slave's sysfs attribute while other thread
exited the loop and is about to remove attributes, then you will kfree
object that is in use and who knows what will happen. This is example
of both refcounting being racey and lifetime rules being violated.
- family framework is insufficient for many advanced w1 devices
No, family framework is just indication which family is used.
Feel free to implement additional methods for various devices
and store them in driver's private areas like ipaq does.
OK, that is what I am aying. But why do you need that attribute with
variable name and a bin attribute that is not really bin but just a
dump for all kind of data (looks like debug one).
- custom-made hotplug notification over netlink should be removed in favor
of standard hotplug notification
It is not hotplug, and your changes broke it completely.
I'm waiting for connector to be included or discarded, so I can move
w1 on top of it's interface or move connector's bits into the w1
subsystem.
You will not be able to cram all 1-wire devices into unified
interface. You will need to build classes on top of it and you might
use connector (I am not sure) bit not on w1 bus level.
...
w1-slave-attr-group.patc
Add 2 default attributes family and serial to slave devices, every
1-Wire slave has them. Use attribute_group to handle. The rest of slave
attributes are left as is - will be dealt with later.
Serial number is already there in bus_id, family is there too.
Why do you need separate files?
Yeah, could probably drop them.
...
w1-drop-owner.patch
Drop owner field from w1_master and w1_slave structures. Just having it
there does not magically fixes lifetime rules.
They do not even pretend, I still do not understand what is lifetime
rules?
So there is no point in having them, right?
w1-bus-ops.patch
Cleanup bus operations code:
- have bus operatiions accept w1_master instead of unsigned long and
drop data field from w1_bus_master so the structure can be statically
allocated by driver implementing it;
- rename w1_bus_master to w1_bus_ops to avoid confusion with w1_master;
- separate master registering and allocation so drivers can setup proper
link between private data and master and set useable master's name.
I strongly object against such changes.
1. w1 was designed in the way that w1 bus master drivers do not
know about other w1 world. It is very simple and very low-level
abstraction,
that only understands how to do low-level functions. It is not needed
do know about w1_master structure and even about it's existence.
Well, it does need to know about w1_bus_master structure, which is
pretty much the same. And it allows having static bus_ops allocation
and removes need for casting from unsigned longs...
2. All renaming are superfluous, I'm not against it, but completely do
not
understand it's merits.
Because now it represents operations only, data field has been
dropped. I my head hurt when I see w1_master and w1_bus_muster
together as 2 separate objects both representing the same piece of
hardware.
3. You broke netlink allocation routing - it may fail and it is not
fatal.
Because it is going away in later patcehs ;)
w1-fold-w1-int.patch
Fold w1_int.c into w1.c - there is no point in artificially separating
code for master devices between 2 files.
w1_int.c was created to store external interface implementation,
why do you want to move it into w1 core code?
It will only soil the code...
Because I do not understand why code creating master devices is
separate from code creating master device's attributes.
w1-drop-netlink.patch
Drop custom-made hotplug over netlink notification from w1 core.
Standard hotplug mechanism should work just fine (patch will follow).
netlink
Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes
One more thing... On 4/21/05, Evgeniy Polyakov [EMAIL PROTECTED] wrote: On Thu, 2005-04-21 at 02:07 -0500, Dmitry Torokhov wrote: w1-master-drop-attrs.patch Get rid of unneeded master device attributes: - 'pointer' and 'attempts' are meaningless for userspace; - information provided by 'slaves' and 'slave_count' can be gathered from other sysfs bits; - w1_slave_found has to be rearranged now that slave_count field is gone. attempts is usefull for broken lines. It simply increments with every search i.e. every 10 secondsby default and does not provide indication of the quality of the wire. -- 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/
