[Home]

Summary:DAHLIN-00037: [patch] Add TDMoE Multi-Frame support
Reporter:Joseph Benden (jbenden)Labels:
Date Opened:2008-09-15 04:17:23Date Closed:2010-05-21 13:05:04
Priority:MajorRegression?No
Status:Closed/CompleteComponents:dahdi_dynamic
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dahdi-dynamic-20080915-3.diff
( 1) dahdi-dynamic-20081025-1.diff
( 2) dahdi-tools-20080915-1.diff
Description:A patch against SVN rev 4904, to add TDMoE Multi-Frame support as described in the article at the following URL:

http://www.thrallingpenguin.com/articles/tdmoe-mf.htm

TDMoE-MF is known to be implemented in hardware solutions from Redfone Communications.

This patch additionally implements RCU within dahdi_dynamic to decrease lock contention, latency, and context switching. Because of the use of RCU locking, all prior known issues with loading and unloading of the modules are resolved, providing the spans are shutdown with "dahdi_cfg -s".

It also contains an attempt, which works, at fixing a kernel change with skb_linearize(). The use of kernel version number does not work with SuSE SLES 10, as it appears they have backported the 2.6.18 change in to their 2.6.16 version.
Comments:By: Tzafrir Cohen (tzafrir) 2008-09-15 04:45:28

Speaking about the shutdown...

The dahdi init script (dahdi.init in the tools directory) has a remmed-out call to the following function at the "stop" operation (before unloading modules).

shutdown_dynamic() {
       if ! grep -q ' ZTD/' /proc/* 2>/dev/null; then return; fi

       # we should only get here if we have dynamic spans. Right?
       $DAHDI_CFG_CMD -s
}


I never actually tested it on any system and hoped someone will, eventually. What do you think about it?

hmm... it should be /proc/dahdi/*, I guess.
By: Joseph Benden (jbenden) 2008-09-15 04:53:37

The TDMoE-MF driver creates a proc entry, /proc/dahdi/dynamic-ethmf, that could be used to determine if the shutdown is safe and to see statistics about the driver.

I have not tested the original TDMoE driver; however, the crash usually happened within dahdi_dynamic. It would be possible to convert dahdi_dynamic_eth to use RCU locking too, but that would have to wait for a bit.

In the mean-time, the init script could test for the presence of /proc/dahdi/dynamic-ethmf and go ahead and do the shutdown, otherwise just move on.
By: Joseph Benden (jbenden) 2008-09-23 15:59:29

Any comments, suggestions, or ideas?
By: Tzafrir Cohen (tzafrir) 2008-10-22 07:31:13

When I build it on my amd64 system, I get the following warnings:

drivers/dahdi/dahdi_dynamic_ethmf.c: In function ‘ztdethmf_transmit’:
drivers/dahdi/dahdi_dynamic_ethmf.c:490: warning: integer overflow in expression
drivers/dahdi/dahdi_dynamic_ethmf.c:490: warning: integer overflow in expression

Line 490 is:

 zh->subaddr = htons((0x8000 | (spans_ready & 0xFF)));

In addition, when building with sparse (make C=1 / make C=2), I get the following:

drivers/dahdi/dahdi_dynamic_ethmf.c:533:2: warning: obsolete struct initializer, use C99 syntax
drivers/dahdi/dahdi_dynamic_ethmf.c:534:2: warning: obsolete struct initializer, use C99 syntax
drivers/dahdi/dahdi_dynamic_ethmf.c:535:2: warning: obsolete struct initializer, use C99 syntax
drivers/dahdi/dahdi_dynamic_ethmf.c:662:2: warning: obsolete struct initializer, use C99 syntax
drivers/dahdi/dahdi_dynamic_ethmf.c:417:11: error: bad constant expression
drivers/dahdi/dahdi_dynamic_ethmf.c:417:29: error: bad constant expression

obsolete initializer:

Use:
struct something foo {
 .bar = 1,
 /* baz is explicitly initialized to a 0 value) */
}

Instead of:

struct something foo {
 bar: 1,
 baz: 0,
}

(Note: this is common in the whole dahdi tree)

bad constant exporession is the array size spans_ready in line 417:

       if ((spans_ready = ethmf_trx_spans_ready(z->addr_hash, &ready_spans))) {
               int pad[spans_ready], rbs[spans_ready];
By: Joseph Benden (jbenden) 2008-10-25 13:27:47

I have uploaded another patch that fixes the sparse messages. The patch is also against the latest trunk.

I am compiling on an Intel 64-bit chip and do not receive any messages about the integer overflow, but have made some changes to hopefully resolve the warnings.
By: Tzafrir Cohen (tzafrir) 2008-10-26 11:47:23

Warnings are gone
By: Joseph Benden (jbenden) 2008-11-04 19:52:10.000-0600

Any comments, suggestions, or ideas?
By: Tzafrir Cohen (tzafrir) 2009-10-11 16:06:02

Patch fails to apply on trunk. It is quite complex.

Why is there a cast to (char* __user) in the ioctls? Anyway, can't this and other small issues be in a separate patch?
By: Leif Madsen (lmadsen) 2009-11-10 08:53:03.000-0600

Just pinging this issue to see if we're still planning on doing anything here? If not, I could just close this as suspended for now if no further progress is going to be made. Thanks!
By: Shaun Ruffell (sruffell) 2009-11-10 09:26:47.000-0600

I'm planning on getting this merged into trunk by 2.3.0.  Not sure how far out that is....a month or two at least.
By: Leif Madsen (lmadsen) 2009-11-10 10:46:50.000-0600

Setting to Ready for Testing as sruffell is planning on getting this merged, so lets acknowledge that some additional testing would be ideal :)
By: Tom Farnham (tomfarnham) 2009-12-23 09:29:39.000-0600

/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c: In function ‘find_echocan’:
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1125: warning: format not a string literal and no format arguments
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c: In function ‘dahdi_ppp_xmit’:
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1755: warning: comparison of distinct pointer types lacks a cast
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1810: error: implicit declaration of function ‘print_debug_writebuf’
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1813: warning: comparison of distinct pointer types lacks a cast
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c: In function ‘dahdi_chan_ioctl’:
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:5020: error: expected ‘;’ before ‘}’ token


Adding the ';' to the end of line 5020 fixes that.

It then comes up with:

/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c: In function ‘find_echocan’:
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1125: warning: format not a string literal and no format arguments
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c: In function ‘dahdi_ppp_xmit’:
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1755: warning: comparison of distinct pointer types lacks a cast
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1810: error: implicit declaration of function ‘print_debug_writebuf’
/usr/src/dahdi-linux-redfone-new/drivers/dahdi/dahdi-base.c:1813: warning: comparison of distinct pointer types lacks a cast


This is based off of revision 5111 and the second patch that was provided.
By: Joseph Benden (jbenden) 2010-01-13 12:37:21.000-0600

I have applied updates to the patch and have it tracking trunk. The patches were minor and were to simply keep the patch applying to changes made to trunk, after the initial patch was created.

It's available from SVN at the following location:

http://svn.asterisk.org/svn/dahdi/linux/team/jbenden/tdmoe-mf/

To answer a question from Tzafrir, "Why is there a cast to (char* __user) in the ioctls?" The answer stems from a request by you to ensure the code passes all "sparse" warnings and notices. These changes were required to do so.
By: Max Khon (max khon) 2010-01-16 04:54:27.000-0600

Is the use of RCU API really worth it? It is not very portable and in exactly this use-case I do not see any major benefits vs. using simple rwlocks.
By: Joseph Benden (jbenden) 2010-01-16 19:27:53.000-0600

The gains are indeed worth the use. It solves high lock usage and context switching. The use of RCU also solved a long standing bug of the dynamic subsystem crashing upon an unload. In general, the dynamic modules are some of the least maintained Zaptel/Dahdi code; for which I'm trying to help do this. :)

Seeing how RCU API is a part of the Linux kernel, I personally see no reason to be concerned with portability. It is well discussed within books, the kernel docs, research papers, and the Linux source itself. It is to my knowledge the available Dahdi drivers, from Digium, support only Linux; so I see no reason for concern with the usage of RCU API.
By: Mark Warren (markw) 2010-02-10 10:47:36.000-0600

The following checkout has been tested thoroughly;
http://svn.asterisk.org/svn/dahdi/linux/team/jbenden/tdmoe-mf/

It has been tested on numerous kernel versions & distros, Debian, Centos, SuSe, with both Redfone hardware and straight Asterisk back-to-back using dahdi_dummy as timing source.

Is there any reason now why this shouldn't be merged?
By: Shaun Ruffell (sruffell) 2010-02-10 11:18:31.000-0600

None that I can think of...it's on my list for things before the 2.3 release of DAHDI which is still looking like a couple of months away.
By: Shaun Ruffell (sruffell) 2010-02-10 11:20:46.000-0600

BTW...speaking of rwlocks vs RCU.  There is movement towards removing rwlocks from the kernel for various reasons: http://lwn.net/Articles/364583/


By: Leif Madsen (lmadsen) 2010-02-22 15:39:38.000-0600

Reassigned to sruffell as he seems to be taking the lead here for the 2.3.x release.
By: Digium Subversion (svnbot) 2010-02-25 13:10:03.000-0600

Repository: dahdi
Revision: 8103

U   linux/trunk/drivers/dahdi/Kbuild
U   linux/trunk/drivers/dahdi/Kconfig
U   linux/trunk/drivers/dahdi/dahdi_dynamic.c
U   linux/trunk/drivers/dahdi/dahdi_dynamic_eth.c
A   linux/trunk/drivers/dahdi/dahdi_dynamic_ethmf.c
U   linux/trunk/include/dahdi/kernel.h

------------------------------------------------------------------------
r8103 | sruffell | 2010-02-25 13:10:02 -0600 (Thu, 25 Feb 2010) | 26 lines

dahdi_dynamic: Add TDMoE Multi-Frame support.

Add TDMoE Multi-Frame support as described in the article at the following URL:

http://www.thrallingpenguin.com/articles/tdmoe-mf.htm

TDMoE-MF is known to be implemented in hardware solutions from Redfone
Communications.

This patch additionally implements RCU within dahdi_dynamic to decrease lock
contention, latency, and context switching. Because of the use of RCU locking,
all prior known issues with loading and unloading of the modules are resolved,
providing the spans are shutdown with "dahdi_cfg -s".

It also contains an attempt, which works, at fixing a kernel change with
skb_linearize(). The use of kernel version number does not work with SuSE SLES
10, as it appears they have backported the 2.6.18 change in to their 2.6.16
version.

This merges in the work Jbenden did at:
http://svn.digium.com/svn/dahdi/team/jbenden/tdmoe-mf@8102

(issue DAHLIN-37)
Patch by: JBenden
Reported by: JBenden
Tested by: JBenden
------------------------------------------------------------------------

http://svn.digium.com/view/dahdi?view=rev&revision=8103
By: Shaun Ruffell (sruffell) 2010-05-21 13:05:03

This was committed in 2.3.0.