This patch adds the CAN core functionality but no protocols or drivers.

No protocol implementations are included here.  They come as separate

patches.  Protocol numbers are already in include/linux/can.h.
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>

Signed-off-by: Urs Thuermann <urs.thuermann@volkswagen.de>
---

 include/linux/can.h       |  113 +++++

 include/linux/can/core.h  |   78 +++

 include/linux/can/error.h |   93 ++++

 net/Kconfig               |    1

 net/Makefile              |    1

 net/can/Kconfig           |   25 +

 net/can/Makefile          |    6

 net/can/af_can.c          | 1002 ++++++++++++++++++++++++++++++++++++++++++++++

 net/can/af_can.h          |  121 +++++

 net/can/proc.c            |  531 ++++++++++++++++++++++++

 10 files changed, 1971 insertions(+)
Index: net-2.6.24/include/linux/can.h

===================================================================

--- /dev/null	1970-01-01 00:00:00.000000000 +0000

+++ net-2.6.24/include/linux/can.h	2007-09-17 10:27:09.000000000 +0200

@@ -0,0 +1,113 @@

+/*

+ * linux/can.h

+ *

+ * Definitions for CAN networklayer (socket addr / CAN frame / CAN filter)

+ *

+ * Authors: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>

+ *          Urs Thuermann   <urs.thuermann@volkswagen.de>

+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research

+ * All rights reserved.

+ *

+ * Send feedback to <socketcan-users@lists.berlios.de>

+ *

+ */

+

+#ifndef CAN_H

+#define CAN_H

+

+#include <linux/types.h>

+#include <linux/socket.h>

+

+/* controller area network (CAN) kernel definitions */

+

+/* special address description flags for the CAN_ID */

+#define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */

+#define CAN_RTR_FLAG 0x40000000U /* remote transmission request */

+#define CAN_ERR_FLAG 0x20000000U /* error frame */

+

+/* valid bits in CAN ID for frame formats */

+#define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */

+#define ...
[ message continues ]

Looks pretty good, please see below for a few comments (mostly minor

nitpicking, a few things that look like real bugs). Nothing that
Is this file used only from within the kernel? If so you could use

the nicer-to-look-at u8/u16/u32 types instead of the double underscored
The callers of the unregister function don't check the return code,

and they can't handle errors anyways since they use it in the

module unload path, so making it void seems more appropriate
Same here, none of the callers check the return value and since
It seems most of the things declared in that file are only used within
This seems to be only used in af_can.c, so it could be static.

__read_mostly also seems to be approriate. There are a few

more that look like they could be __read_mostly below, but
Both of these printks seem to be user-triggerable, so they should
What prevents the module from getting unloaded again (and using
EPERM doesn't seem like the best fit, but I don't have a better
So the intention here is to send the packet to the non-loopback device
On the receive path you use RCU, so this should be

hlist_for_each_entry_rcu(), no? The bottem half disabling during

addition/removal also seems unnecessary, but I might be missing
If I'm not mistaken this comment is outdated and should refer to
-

These functions have been declared returning void originally, but in

the review process with Thomas we changed them since they can fail.

Even if the current users, i.e. raw.c and bcm.c don't use the return

value, others might do.  I therefore prefer to keep the int return

value.  Rather we should check whether we can do something useful with

the return value in raw.c and bcm.c.  At least we can put a WARN_ON
af_can.h declares the interface between af_can.c and proc.c.  There
Can't be static since it's used in proc.c.  But __read_mostly might

make sense.
What exactly is the effect of __read_mostly?  Is that in a separate
Hm, I don't think DBG() would be right here, since the messages show

problems in the installation to the admin.  OTOH, I see that a user

can flood the log by opening sockets with invalid proto numbers.  Rate

limiting might solve this, or we should print the message only once
When the module is unloaded it calls can_proto_unregister() which
We have chosen EPERM because a user shouldn't be allowed to open a raw

CAN socket and then send arbitrary frames to other non-CAN interfaces.

This is also because the raw and bcm protocols can be configured to
CAN is a broadcast message network, so every frame should be (usually)

sent to all receivers, on remote hosts and to all local sockets.  If

the driver for the interface is not able to loop back the frame for

local delivery, the can_send() function will do this as a fallback.

For real CAN devices it is preferred that the driver does loopback.

For vcan it makes no difference where loopback is done.  The module

paramenter for vcan is therefore only useful to test and debug the CAN
find_dev_rcv_lists() is called in one place from can_rcv() with RCU

lock held, as you write.  The other two calls to find_dev_rcv_lists()

are from can_rx_register/unregister() functions which change the

receive lists.  Therefore, we can't only use RCU but need protection

against simultanous writes.  We do this with the spin_lo...
[ message continues ]

IMO this is a perfectly normal condition (not finding a module).
Yes, you do request_module, load the module, get the cp pointer

from proto_tab, the module is unloaded again. cp points to
I'm not saying you should use *only* RCU, you need the lock

for additions/removal of course, but since the receive path

doesn't take that lock and relies on RCU, you need to use

the _rcu list walking variant to avoid races with concurrent
Yes. register_netdevice() takes the rtnl mutex, so it can only
I don't think you will get *exactly* one second, but you also have
Thanks :)

-

Hi Patrick,
I have done allmost all changes to the code as you suggested.  The

changes to use the return value of can_rx_register() also fixed a

minor flax with failing bind() and setsockopt() on raw sockets.
But there are two things left I would like to ask/understand:
How would I use the module reference counter?  Somehow with

try_module_get()?  I have thought something like
        cp = proto_tab[protocol];

        if (!cp ...)

                return ...;
        if (!try_module_get(cp->prot->owner))

                return ...;
        sk = sk_alloc(...)
        module_put(...);

        return ret;
But here I see two problems:
1. Between the check !cp...  and referencing cp->prot->owner the

   module could get unloaded and the reference be invalid.  Is there

   some lock I can hold that prevents module unloading?  I haven't

   found something like this in include/linux/module.h
2. If the module gets unloaded after the first check and

   request_module() but before the call to try_module_get() the

   socket() syscall will return with error, although module auto
I have no objections to add the _rcu suffix for the code changing the

receive lists, but I don't see why it's necessary.  When I do a

spin_lock_bh() before writing, can't I be sure that there is no

interrupt routine running in parallel while I hold this spinlock?  If

so, there is no reader in parallel because the can_rcv() function runs

in a softirq.  I'd really like to understand why you think the writers

should also use the _rcu variant.  I'm sorry if I miss something

obvious here, but could you try to explain it to me?
urs

-

No, you need to add your own locking to prevent this, something

list this:
registration/unregistration:
take lock

change proto_tab[]

release lock
lookup:
take lock

cp = proto_tab[]

if (cp && !try_module_get(cp->owner))

	cp = NULL
Why do you want to prevent it? The admin unloaded the module,
I'm saying you need _rcu for the *read side*. All operations changing
spin_lock_bh only disables BHs locally, other CPUs can still process

softirqs. And since rcv_lists_lock is only used in process context,

the BH disabling is actually not even necessary.
-

Well, unloading a module doesn't usually cause to operation to fail

when auto loading is enabled.  It only wouldn't succeed when the

unload happens in the small window between test/request-module and

call to try_module_get().  This looks ugly to me.  But the lock you
Well, I finally (hopefully) got it and I have changed the code

accordingly.  Thanks for your explanation.
I will post our updated code again, probably today.  The issues still

left are
* module parameter for loopback, but we want to keep that.

* configure option for allowing normal users access to raw and bcm CAN

  sockets.  I'll check how easily an (embedded) system can be set up

  to run relevant/all processes with the CAP_NEW_RAW capability.  I

  would like to kill that configure option.

* seq_files for proc fs.  On my TODO list.
urs

-

Sounds good, thanks.

-

Thank you for your review.  I'll go through it and your other mail

this evening/tonight.  From a fast overview it looks like most issues

can be fixed/changed quite easily.
urs

-

Agreed. No objections to putting this in 2.6.24 from my side ..
-

Looks good to me!!!
>