Summary:	DAHLIN-00001: [patch] automagic configuration with sysfs and udev
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2006-07-28 20:45:27	Date Closed:	2016-09-12 06:21:20
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) autoztcfg.diff ( 1) autoztcfg-1.2.diff ( 2) Makefile-kernel26_patch_1_4_rev3121 ( 3) Makefile-kernel26_patch_1_4_rev3211 ( 4) zaptel-base-c_patch_1_4_rev3121 ( 5) zaptel-base-c_patch_1_4_rev3211 ( 6) zaptel-h_patch_1_4_rev3121 ( 7) zaptel-h_patch_1_4_rev3211 ( 8) zaptel-sysfs.c ( 9) zaptel-sysfs.h (10) zaptel-sysfs-private.h (11) zaptel-sysfs-private-v2.h (12) zaptel-sysfs-v2.c (13) zaptel-sysfs-v2.h
Description:	Expose information through udev. (ab)use this for e.g. avoiding the need to explicitly run dahdi_cfg in the common cases.
Comments:	By: Tzafrir Cohen (tzafrir) 2006-07-30 03:18:23 Uploaded a version for 1.2 (1.2.6, actually) that is tested. By: Denis Smirnov (mithraen) 2006-08-11 11:14:01 I think this patch is useful. By: jmls (jmls) 2006-11-01 06:39:41.000-0600 ping. Developers, care to comment ? By: jmls (jmls) 2007-01-07 03:08:33.000-0600 any comments from anybody ? Please :( By: Tzafrir Cohen (tzafrir) 2007-01-25 00:37:46.000-0600 I noticed that zapscan, which is part of asterisk-gui essecially enforces ks signalling for analog ports, and is run on every run of the zaptel init script. So why not just configure analog ports automatically and save the need to run ztcfg for them in the first place? By: Tzafrir Cohen (tzafrir) 2007-10-12 07:26:12 This one belongs in Zaptel. Anyway, one obvious problem: ztcfg is still needed to set the tonezone, right? By: Tzafrir Cohen (tzafrir) 2007-10-17 16:34:24 Changed back to "new", as patch probably needs updating. By: meneault (meneault) 2007-10-22 07:48:06 IMHO I would say that what could be done in user-space should be done in user-space. I think that most of zaptel drivers are already overloaded from things that should have been done in userspace. It results in a less readable, less maintainable code. As you pointed out yourself, ztcfg does more than actually setting the channel signalling type, and in the future it could even do more. In the future, analog drivers may support other signalling types, for instance Ground Start -- How would you choose beetween them? Only a user can do that. And AFAIK every 'asterisk for dumbs' distribution already comes with scripts that handle this heuristic pretty well. However leveraging the ioctl function is not a bad idea so moving  ZT_CHANCONFIG into a separate chunk of code for modularity (could be good -- but in this case you could make it inline). By: Brandon Kruse (bkruse) 2007-10-22 09:12:41 Tzafrir: If we had a tool to run to see if the cards have changed. Zapscan forces ks, because ks can work with ks and ls. This is useful and good code, but probably not in the right place.... -bk By: Tzafrir Cohen (tzafrir) 2007-10-22 11:54:47 Usually I'm all for doing things in userspace. However in this case the work to do is simply unnecessary. If you think that this patch is bad due to "doing in userspace", please provide a scenario where it actually gets in the way or handling things in userspace. For example, let's consider a strange user trying to force a specific channel to be loopstart rather than kewlstart. With this patch in place the user can still use the approach taken by the current init.d script: write a zaptel.conf with just the channels the user wants to override and run ztcfg at the zaptel init.d script. bkruse mentioned zapscan that is run automatically at the init.d script of asterisknow. In the case of our user, zapscan will always override his channels in zaptel.conf and he will not be able to use loopstart signalling. Anyway, the proper fix is to run a partial ztcfg from udev for each span separately once I get proper device model support. This is the proper place to move things to userspace. Right now userspace simply has not enough data. By: Brandon Kruse (bkruse) 2007-10-22 12:25:58 I completely agree. However, very soon, the user will be able to have much more control over zaptel.conf. Zapscan will NO LONGER write directly zaptel.conf, instead, it will write to zapscan.conf in /etc/asterisk and then the GUI will write out a zaptel.conf which can be edited if needed, and NOT overwritten every startup (yes, it is a pain :/ ) I am open for any better suggestions on handling this also. -bk By: Brandon Kruse (bkruse) 2007-10-22 12:26:38 Actually, why do we not just add some logic to zapscan and check zaptel.conf to see if the channel definitions exist, or give it a --force mode for overriding such. Just some food for thought -bk By: Tzafrir Cohen (tzafrir) 2007-10-22 12:34:49 zapscan is a binary that is not part of Zaptel. By: Brandon Kruse (bkruse) 2007-10-22 13:26:06 Correct. -bk By: meneault (meneault) 2007-10-23 02:09:36 Tzafrir: About 'zapscan', from what I understand this is just a limitation of the boot process of a specific distribution that don't give the user the possibility to override the 'Asterisk For Now' heuristics. That's not a matter of zaptel from my point of view and as bkruse said the work to allow this is underway. As I previously mentioned configuring a channel is not only a matter of configuring its channel type, and the current implementation that need configuring before use is the safer approach because it tells the user 'warning you didn't configure anything'. Guessing a proper signalling from the sigcap of a channel is not deterministic it is a matter of choosing. In kernel when choosing is required then it is up to the user-space to do so. And I do agree that the udev way would be simply the cleanest way, a clean approach would be: - before registering a span the driver should determine its channel type. (this may need a fix for instance for wctdm which determine it afterwards) - add environment variables to an uevent (from zaptel-base during zt_register). uevent may be the one when channel is registered (ZT_CHANCAP for instance which would give signalling capabilities of the channel). - then any distribution may do its own script to respond to such udev events fetching ZT_CHANCAP to give a common start to any zap device. For instance building-up an automatic zaptel-auto.conf, then merging it with user's zaptel.conf. At the end of the boot a simple call to ztcfg with zaptel-merged.conf and everything should be fine. Note: calling ztcfg directly to a response to udev event could be possible but may rise some race conditions so better run ztcfg at the end of the boot anyway. Another possibility would be to make this information available directly to sysfs (or procfs) but I am not sure that information is still necessary after the boot process because zaptel already expose some information on procfs (after configuration of a channel it puts its signalling type). By: Tzafrir Cohen (tzafrir) 2007-10-23 12:44:38 meneault: take a look at zaptel/team/tzafrir/sysfs . What other information do you need exposed? uevents are not yet in place. By: meneault (meneault) 2007-10-24 01:37:41 I have looked at your implementation and its nice (code is clear and file less than 1000 lines...thanks -- zaptel needs a lot of redesign at least zaptel-base should be split in 5 or 6 files, ok that's not of our business right now). Apparently you made a separate bus in sysfs to work with virtual devices, well thats a clean approach however I think that using the class approach of zaptel could be better fitted -- I am not an expert of linux device model but I think classes have been created so as to abstract a behavior, and its class devices to  abstract devices from a logical point. I think that buses/devices are reserved for real hardware buses and devices. Zaptel base already creates a class called "zaptel" in sysfs and you could simply modify its class device nodes instead of duplicating some information in a virtual bus. I do agree that classes are tricky to handle when dealing with all kernel versions from 2.6.1 to 2.6.16 but this would be the better approach IMHO. Now, about the necessary information to be exported (personally I don't need any yet), If what you need is auto configuration of channels a simple a look at ZT_GET_PARAMS ioctl and ztcfg makes me think you should add: - channel's master number - channel's current law (take a look at ZT_GET_PARAMS ioctl current law is detemined by pointer checking on xlaw) - channel's idlebits (Yes some information may be related to digital signalling but it does not hurt to expose that information). And then you could probably make good heuristics from user-space to properly get a channel configured. Another Note: you do not lock on channel access nor in span access, it may not hurt much because these are only read accesses but take in account that sysfs information may be out of meaning then. By: meneault (meneault) 2007-10-27 05:52:43 In my previous message I told you how to better you sample, I have written something from scratch that keep backward compatibility with existing version  of zaptel and add support for any 2.6.* kernel (up to now of course) (code tested on a 2.6.9 kernel and on a 2.6.19 kernel). note: It could even work on a 2.5.69 kernel I think but I have not checked for, please take note that current implementation enables CONFIG_ZAP_UDEV when we use a 2.6.1 or latter kernel but I think it may not work on a 2.6.1 (because class_simple was added on 2.6.2 AFAIK). My code moves the sysfs code of zaptel in a separate file zaptel-sysfs.c and it adds: - uevent environment variables support (even if not so useful) - a real good start to add the all the sysfs information we need  in a clean an easy manner. - take a look at zaptel-sysfs.c I have been really verbose in comments  I talk about the sysfs /sys/zaptel tree. - I have already added a lot of usefull information:  each channel has got a sysfs directory containing:  - attributes name/channo/chanpos/sig/sigcap/master->channo/xlaw  - symlink to its span  each span has got a sysfs directory containing:  - attributes name/desc/spanno/channels/alarms  - a symlink for each of its channels - Oh yes I forgot to mention I added proper locking when dealing with attributes printing. I deliver the code: - zaptel-sysfs.c (sysfs main code) - zaptel-sysfs-private.h (sysfs handy macros and defines to handle any 2.6 version of the kernel) - zaptel-sysfs.h (public header file for zaptel sysfs exported methods) - zaptel-base-c_patch_1_4_rev3211 (patch for zaptel-base.c) - zaptel-h_patch_1_4_rev3211      (patch for zaptel.h) - Makefile-kernel26_patch_1_4_rev3211  (patch for the Makefile) I developed the code under branch 1.4 of zaptel revision 3211, however the changes in zaptel code is minimal so the patch should apply on most zaptel 1.4 revisions. For the 1.2 branch I don't know. I added license on the new files but feel free to modify them at your needs. If you have any questions ask me, you can modify them at will because I don't plan to work on it these days since they work. To print a new member of a span or a channel is a matter of two lines of codes so It won't be too difficult to add something in. Moreover As I said I have been really verbose (maybe too much) so you won't get lost. By: Tzafrir Cohen (tzafrir) 2007-10-27 08:02:57 The devices classes interface is being deprecated by the kernel developers. This is why I have avoided it. By: meneault (meneault) 2007-10-29 06:46:36 You're completely right about class_device(s), they are now deprecated but they are still valid to use (even in 2.6.24 kernel which was released 4 days ago AFAIK). I already said that it was tricky to handle it for the 2.6.* kernels and it is, linux device model is a new feature of 2.6 so it is not surprising. Now, struct device should be used instead of class_device. However the changes required are not so hard to get I think. Apparently the idea was first announced in the 2.6.16 kernel, but the main migration occurred in the 2.6.19 to 2.6.21 kernels. However I am not sure we should make a rule that would migrate the class_device to device since 2.6.19 kernels but rather wait until 2.6.24 because some patches have been added to the driver core later on to fix some issues about this migration (2.6.22 and 2.6.23 may be safe but I have not checked for). I will give another shot to handle all this migration: - I will fix identifiers issues (e.g. CLASS_DEVICE by DEVICE) - I will replace class_device by device from 2.6.24 kernels - Fix another issue about uevent API change from 2.6.23-rc8. Then It will be up to you and the managers to decide what should be best. My opinion is: As every span is linked to only one device (AFAIK), each span is already on a "real" bus so there is no need to add a new virtual bus. IMHO using class and class_device under 2.6.2* kernel is good as: 1- we don't break the linux device model 2- we don't break zaptel implementations and user spaces programs using it   (including startup scripts and every udev version) 3- we don't overload the sysfs tree duplicating information on it. (The virtual bus way will break point 3 at least (and 1 IHMO only)). I think that respecting 1 and 3 is really important if someday we wanted the zaptel driver to be integrated in the kernel. Then using class and device over 2.6.2* kernels is good as: 1- we can keep the same layout as with class_device (everything would be still fetched under the "zaptel" class with the same identifiers if we want to) 2- this will reflect the real hardware/driver relationships. (Point 1 & 2 cannot be achieved without duplicating information using the virtual bus method). Note: - this would be good to have zaptel be integrated into the mainline kernel, this would avoid us all this handling about kernel versions! Warning: - my patches are not for 3211 revision of 1.4 branch but for the 3121 (sorry about that). By: Tzafrir Cohen (tzafrir) 2007-10-29 07:24:03 Let's follow-up on the asterisk-dev mailing list or on #asterisk-dev on irc.freenode.net? I appreciate your work, and have some comments on it, but this place is sub-optimal for discussions. By: meneault (meneault) 2007-10-30 09:04:12 Ok for asterisk-dev mailing list. Anyway I should post my second shot that handle issues related to class_device/device migration. So here you have: - zaptel-h_patch_1_4_rev3121 - zaptel-base-c_patch_1_4_rev3121 - Makefile-kernel26_patch_1_4_rev3121 - zaptel-sysfs-v2.c   (please rename it to zaptel-sysfs.c before compiling) - zaptel-sysfs-private-v2.h (please rename it to zaptel-sysfs-private.h) - zaptel-sysfs-v2.h  (please rename it to zaptel-sysfs.h) Please note that: - I arbitrarily set the class_device to device migration since linux 2.6.24. - It was tested on 2.6.9 kernel (with wcfxo and wctdm for the spans/channels)  and on a 2.6.19 kernel (with ztdummy for the span) - Even if it wasn't tested on a 2.6.24 I forced the migration to device on my 2.6.19 kernel and it worked well (as well as it should on a 2.6.19). - Only thing not tested the API uevent change that happened on 2.6.24. By: Brandon Kruse (bkruse) 2008-01-27 23:32:18.000-0600 Ping: What is the status on this now. Just for reference, 'ztscan' which scans both analog and digital is not in the zaptel 1.4 branch. -bk By: Steve Murphy (murf) 2008-06-21 15:49:40 Anything I can do to help move this bug to closing? I could, for instance, update patches to DAHDI, etc. What does everyone involved want to see happen with this bug report? Seems to me that that a bit of work and thought went into this report, and it'd be a shame to arbitrarily close it due to inactivity. Let's finish what was begun? By: meneault (meneault) 2008-06-22 11:14:01 Hi, Unfortunately I don't have much time to spend on the issue now. The issue went much beyond than simple automatic channel configuration, It was more a sysfs code update to better deal with 2.6 kernels and a uniform API for modules to expose information to userspace. I only remember that is was a great headache to deal with all 2.6 sysfs linux kernel API incompatibilities but I found a way to deal with them in an uniform manner (I think). However the resulting header handling these "magic" could look nasty at first sight and I agree with this... I can help but not too often and for small tasks only... By: Leif Madsen (lmadsen) 2008-11-21 09:25:26.000-0600 I'd like to ping this issue again as per murf to see what is necessary to move this bug to closing. Even if meneault can't move this issue forward, perhaps someone who was part of this discussion can chime in and layout what needs to be done to get this issue resolved. By: Leif Madsen (lmadsen) 2009-05-19 13:01:14 Moved back to 'new' as there appears to be no one able to move this forward at the moment. By: Shaun Ruffell (sruffell) 2011-12-08 14:29:23.643-0600 This is (slowly) moving forward with the 2.6.0 release. By: Tzafrir Cohen (tzafrir) 2016-09-12 06:21:20.588-0500 Issue has mostly been resolved by the sysfs work and tools using it. As an example, a DAHDI system with empty configuration could mostly just work (with a slight tweak of the existing scripts).