Ira W. Snyder wrote: > On Mon, Feb 22, 2010 at 08:42:56PM +0100, Wolfgang Grandegger wrote: >> Ira W. Snyder wrote: >>> This patch series adds support for the Janz CMOD-IO carrier board, as well >>> as the Janz VMOD-ICAN3 Intelligent CAN controller. The CMOD-IO carrier >>> board is a PCI to MODULbus bridge, into which plug MODULbus daughterboards. >>> I only have access to two types of daughtercards, the VMOD-ICAN3 mentioned >>> above, and the VMOD-TTL GPIO controller. >>> >>> All of my boards have two VMOD-ICAN3 modules and one VMOD-TTL module. This >>> posting only contains drivers for the CMOD-IO carrier board and VMOD-ICAN3 >>> CAN interfaces. A driver for the VMOD-TTL GPIO module is under development, >>> and will be posted shortly. This module is even worse to program nicely >>> than the ICAN3 module. >>> >>> Since the RFCv2 posting, the CAN driver has been much more thoroughly >>> tested. CAN bus-off works correctly, as does the generation of error >>> frames. The bus-off and error frame code has been adapted from the SJA1000 >>> driver, as the ICAN3 firmware reports most of the status registers used by >>> the SJA1000 code. >> Sounds good and from my point of view the driver is more or less ready >> for mainline inclusion. If that is your primary goal and you feel it is >> mature and stable enough, please send a proper patch series as described >> here: >> >> http://svn.berlios.de/svnroot/repos/socketcan/trunk/README.submitting-patches. >> >> As an alternative, I could apply it to the SVN trunk for the time being. >> There, the requirements for acceptance are not that high. >> >> I briefly browsed the patches. Here some quick comments: >> >> - I do still not find __devinit, __devexit, and friends in your drivers >> as described here: >> >> http://lxr.linux.no/#linux+v2.6.32/Documentation/PCI/pci.txt#L177 >> >> They are also missing in janz-ican3.c. >> >> - You may need to declare some structures "__attribute__((packed))", >> >> - Don't include sja1000/sja1000.h. It's only for drivers in sja1000. >> I know that some other drivers use SJA1000 definitions as well, but >> that requires a general solution. >> > > Why not? I need some of the definitions for the SJA1000 error registers. > Is there any reason why it can't be include/linux/can/sja1000.h instead?
Yes, it does also contain private declarations and definitions which should not be used outside sja1000. > It seems stupid to duplicate the register definitions in each new driver > that comes along. Yes. As I said above, this needs a general solution splitting sja1000.h in public register definitions and private stuff for the sja1000 drivers. >> - Some time ago we agreed to use "_" for the Socket-CAN file names: >> s/janz-ican3/janz_ican3/ >> >> - You still use many hard-code numbers in the code. Please define >> values for most of them to make the code more readable. >> > > I missed a few of these in the version I sent. They'll be fixed for the > next version. > >> - There are still to much dev_dbg(). They should especially not be used >> in the xmit and recv path. >> >> - I see still a lot of duplicated code, especially for desc handling. >> Maybe some helper functions or combined i/o functions for send/recv >> could make the code more compact. >> >> - Checkpatch reports "lines too long". >> >> - s+<linux/janz.h>+<linux/mfd/janz.h>+ ? >> > > Ok. > >> - Check MODULE_LICENSE(). It does not match with your copyright notes. >> > > It will be changed to "GPL v2". I didn't know there was a difference > between "GPL" and "GPL v2" until I hunted down include/linux/module.h's > comments. I don't mind GPL v2 or later licensing, but I thought the > Linux kernel was GPL v2 only. I guess not. No, I think most code is under GPL v2 and *later*. If you have no particular reason, I would use that license (instead of the restricted v2). But that's your choice, of course. >> - About xmit flow control. What happens if you send messages quickly at >> 125 KB/s. You could use "cangen -g 0 can0" for that test. How many >> messages get dropped? >> > > I let the cangen command run for a while: > $ ifconfig -a > can0 Link encap:UNSPEC HWaddr > 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00 > UP RUNNING NOARP MTU:16 Metric:1 > RX packets:473455 errors:0 dropped:0 overruns:0 frame:0 > TX packets:473455 errors:0 dropped:1831983 overruns:0 carrier:0 As I expected, most packets get dropped because of missing xmit flow control. > collisions:0 txqueuelen:10 > RX bytes:2719863 (2.5 MiB) TX bytes:2719863 (2.5 MiB) > Interrupt:22 > When running cangen, the TX/RX rate is about 32KB/sec (258 kbit/sec) at > roughly 5800 packets/sec. Seems pretty low for the CAN devices > configured like this: > > 5: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN qlen > 10 > link/can > can state ERROR-ACTIVE restart-ms 0 > bitrate 1000000 sample-point 0.750 > tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1 > janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1 > clock 8000000 > 6: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN qlen > 10 > link/can > can state ERROR-ACTIVE restart-ms 0 > bitrate 1000000 sample-point 0.750 > tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1 > janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1 > clock 8000000 > > Any ideas on how I can go faster? The kernel appears to be spending ~63% > of its CPU time running cangen, and ~37% in softirq context, running > events/0 (the workqueue thread). cangen retries immediately if the send() returns ENOBUFS resulting in a high CPU load. It would be better to sleep some time or use poll/select. The softirq load is due to the fact that you are dropping packets at high rate and it's even worse at lower bitrates, I guess. Does it get better if you return with NETDEV_TX_BUSY (and do not free the packet). Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
