Re: [PATCH 5/7] CAN: Add virtual CAN netdevice driver

Previous thread: [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #9 by Urs Thuermann on Tuesday, October 2, 2007 - 6:10 am. (1 message)

Next thread: [PATCH 4/7] CAN: Add broadcast manager (bcm) protocol by Urs Thuermann on Tuesday, October 2, 2007 - 6:10 am. (1 message)
From: Urs Thuermann
Date: Tuesday, October 2, 2007 - 6:10 am

This patch adds the virtual CAN bus (vcan) network driver.
The vcan device is just a loopback device for CAN frames, no
real CAN hardware is involved.

Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
Signed-off-by: Urs Thuermann <urs.thuermann@volkswagen.de>

---
 drivers/net/Makefile     |    1 
 drivers/net/can/Kconfig  |   25 +++++
 drivers/net/can/Makefile |    5 +
 drivers/net/can/vcan.c   |  207 +++++++++++++++++++++++++++++++++++++++++++++++
 net/can/Kconfig          |    3 
 5 files changed, 241 insertions(+)

Index: net-2.6.24/drivers/net/Makefile
===================================================================
--- net-2.6.24.orig/drivers/net/Makefile	2007-09-20 23:46:01.000000000 +0200
+++ net-2.6.24/drivers/net/Makefile	2007-10-02 12:03:30.000000000 +0200
@@ -12,6 +12,7 @@
 obj-$(CONFIG_CHELSIO_T1) += chelsio/
 obj-$(CONFIG_CHELSIO_T3) += cxgb3/
 obj-$(CONFIG_EHEA) += ehea/
+obj-$(CONFIG_CAN) += can/
 obj-$(CONFIG_BONDING) += bonding/
 obj-$(CONFIG_ATL1) += atl1/
 obj-$(CONFIG_GIANFAR) += gianfar_driver.o
Index: net-2.6.24/drivers/net/can/Kconfig
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ net-2.6.24/drivers/net/can/Kconfig	2007-10-02 12:03:30.000000000 +0200
@@ -0,0 +1,25 @@
+menu "CAN Device Drivers"
+	depends on CAN
+
+config CAN_VCAN
+	tristate "Virtual Local CAN Interface (vcan)"
+ 	depends on CAN
+	default N
+ 	---help---
+	  Similar to the network loopback devices, vcan offers a
+	  virtual local CAN interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called vcan.
+
+config CAN_DEBUG_DEVICES
+	bool "CAN devices debugging messages"
+	depends on CAN
+	default N
+	---help---
+	  Say Y here if you want the CAN device drivers to produce a bunch of
+	  debug messages to the system log.  Select this if you are having
+	  a problem with CAN support and want to see more of what is going
+	  on.
+
+endmenu
Index: ...
From: Arnaldo Carvalho de Melo
Date: Tuesday, October 2, 2007 - 7:20 am

Can debug be a boolean? Like its counterpart on DCCP:

net/dccp/proto.c:

module_param(dccp_debug, bool, 0444);

Where we also use a namespace prefix, for those of us who use ctags or

Is this comment still valid? If so can this move happen now? If not I


Thinking out loud: I guess these days we can try to reduce the clutter
on the source code for things like "hey, I entered function foo" using
simple systemtap scripts, that could even be shipped with the kernel
-

From: Oliver Hartkopp
Date: Tuesday, October 2, 2007 - 8:07 am

Bringing all the current available CAN network device drivers into 
Kernel style qualitiy is a TODO for the time after the PF_CAN core is 
mainlined.

When more than this single vcan CAN netdev driver is part of the Kernel 
it makes sense to put several things (like the common configuration 
interface and commonly used library funtions for CAN drivers) into 
linux/can/dev.h. And at that time this currently local DEBUG definition 
should go there togehther with the other stuff.

Please think of all the comments, if we created a single dev.h file with 
a single DEBUG definition used by a single vcan.c file ;-)

Oliver

-

From: Arnaldo Carvalho de Melo
Date: Tuesday, October 2, 2007 - 9:46 am

Don't get to defensive. You know a lot more than me about the code you
worked for that many years. Its just that it looked suspicious for a
casual reviewer. :-)

- Arnaldo
-

From: Oliver Hartkopp
Date: Tuesday, October 2, 2007 - 2:02 pm

'debug' should remain an integer to be able to specifiy debug-levels or 

Even if i don't have any general objections to rename this 'debug' to 
'vcan_debug', it looks like an 'overnamed' module parameter for me. Is 

Yes. This is definitely a boolean candidate. We'll change that.

Thanks,
Oliver

-

From: Arnaldo Carvalho de Melo
Date: Tuesday, October 2, 2007 - 2:43 pm

[acme@mica linux-2.6.23-rc9-rt1]$ find . -name "*.c" | xargs grep
'module_param(.\+debug,' | wc -l
112
[acme@mica linux-2.6.23-rc9-rt1]$ find . -name "*.c" | xargs grep
'module_param(debug,' | wc -l
233
[acme@mica linux-2.6.23-rc9-rt1]$

I think that helping ctags to find the definition for the debug variable
to see, for instance, if it is a bitmask or a boolean without having to
chose from tons of 'debug' variables is a good thing.

- Arnaldo
-

From: David Miller
Date: Tuesday, October 2, 2007 - 2:50 pm

From: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>

I completely agree.
-

From: Oliver Hartkopp
Date: Wednesday, October 3, 2007 - 12:06 am

OK. No problem if it's helpful. We'll change debug to vcan_debug.

Thanks.

Oliver
-

From: Stephen Hemminger
Date: Tuesday, October 2, 2007 - 2:52 pm

On Tue, 02 Oct 2007 23:02:53 +0200

Please consider using netif_msg_xxx() and module parameter to set
default message level, like other real network drivers already do.

-- 
Stephen Hemminger <shemminger@linux-foundation.org>
-

From: David Miller
Date: Tuesday, October 2, 2007 - 3:04 pm

From: Stephen Hemminger <shemminger@linux-foundation.org>

I keep seeing this recommendation, but the two supposedly most mature
and actively used drivers in the tree, tg3 and e1000 and e1000e, all
do not use this scheme.

In fact there are tons of drivers that even hook up the ethtool
msg_level setting function and never even use the value.

If people aren't using netif_msg_xxx() and the ethtool msg_level
facilities properly, it's because there is a severe dearth of good
example drivers to learn about it from.
-

From: Oliver Hartkopp
Date: Wednesday, October 3, 2007 - 10:47 am

The currently available CAN netdevice drivers do not have a common debug 
concept neither any runtime control mechanism for this debugging. So 
netif_msg_xxx() is definitely worth to look at instead of creating any 
new stuff in this direction, before posting any 'real' CAN network 
driver here.

Thanks very much for that hint!

Oliver
-

From: Urs Thuermann
Date: Thursday, October 4, 2007 - 4:52 am

debug used to a bit mask, like it still is in core.h.  You can see
this in the test

        debug & 1 ? ... : ...


I think ctags should be able to handle multiple identical static
symbols.  Isn't it?  I find it somewhat clumsy to write

        modprobe vcan vcan_debug=1

I think it would be nice to change the module_param() macro so that
you can name the module argument and the corresponding variable
independently, like

        module_param(can_debug, "debug", bool, 0444);

OK, forget that last paragraph.  I've looked at the definition of
module_param() and have seen that we have module_param_named().  I



I've never heard of systemtap before.  I've ust looked at its overview
web page which sounds promising.  I think I'll check it out when time
permits...


urs
-

Previous thread: [PATCH 0/7] CAN: Add new PF_CAN protocol family, try #9 by Urs Thuermann on Tuesday, October 2, 2007 - 6:10 am. (1 message)

Next thread: [PATCH 4/7] CAN: Add broadcast manager (bcm) protocol by Urs Thuermann on Tuesday, October 2, 2007 - 6:10 am. (1 message)