Re: [PATCH 2/7] CAN: Add PF_CAN core module
David Miller [EMAIL PROTECTED] writes: I really frown upon these local debugging macros people tend to want to submit with their changes. It really craps up the tree, even though it might be useful to you. We have now removed the debug code completely. The code is quite stable and we won't need these remnants from early days anymore. So please remove this stuff or replace the debugging statements with some generic kernel debugging facility, there are several. I am not aware of any useful kernel facilities to replace our debug macros, i.e. printing of debug messages, controlled by a bit mask passed in as a module parameter, hexdumping of fames, etc. Of course, there are other things like kprobes but sometimes printk(KERN_DEBUG...) is more convenient and that's why people write their own DBG() macros. urs - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
From: Urs Thuermann [EMAIL PROTECTED] Date: 16 Nov 2007 15:33:08 +0100 I am not aware of any useful kernel facilities to replace our debug macros, i.e. printing of debug messages, controlled by a bit mask passed in as a module parameter, hexdumping of fames, etc. Of course, there are other things like kprobes but sometimes printk(KERN_DEBUG...) is more convenient and that's why people write their own DBG() macros. At the simplest level there is pr_debug() which replaces the usual print if DEBUG is defined that lots of people use. For device drivers we have something similar in dev_vdbg() et al. For bitmask based enabling yes it would be nice if there were something more. But you can also get this behavior by using DEBUG, the above interfaces, and adjusting the kernel log level to suit what you're trying to view. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Thu, 2007-11-15 at 08:40 +0100, Oliver Hartkopp wrote:
Stephen Hemminger wrote:
+#ifdef CONFIG_CAN_DEBUG_CORE
+extern void can_debug_skb(struct sk_buff *skb);
+extern void can_debug_cframe(const char *msg, struct can_frame *cframe);
+#define DBG(fmt, args...) (DBG_VAR 1 ? printk( \
+ KERN_DEBUG DBG_PREFIX : %s: fmt, \
+ __func__, ##args) : 0)
+#define DBG_FRAME(fmt, cf) (DBG_VAR 2 ? can_debug_cframe(fmt, cf) : 0)
+#define DBG_SKB(skb) (DBG_VAR 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(fmt, args...)
+#define DBG_FRAME(fmt, cf)
+#define DBG_SKB(skb)
+#endif
I would prefer the more frequently used macro style:
#define DBG(fmt, args...) \
do { if (DBG_VAR 1) printk(KERN_DEBUG DBG_PREFIX : %s: fmt, \
__func__, ##args); } while (0)
#define DBG_FRAME(fmt, cf) \
do { if (DBG_VAR 2) can_debug_cframe(fmt, cf); } while (0)
#define DBG_SKB(skb) \
do { if (DBG_VAR 4) can_debug_skb(skb); } while (0)
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Stephen Hemminger [EMAIL PROTECTED] writes:
+#ifdef CONFIG_CAN_DEBUG_CORE
+extern void can_debug_skb(struct sk_buff *skb);
+extern void can_debug_cframe(const char *msg, struct can_frame *cframe);
+#define DBG(fmt, args...) (DBG_VAR 1 ? printk( \
+ KERN_DEBUG DBG_PREFIX : %s: fmt, \
+ __func__, ##args) : 0)
+#define DBG_FRAME(fmt, cf) (DBG_VAR 2 ? can_debug_cframe(fmt, cf) : 0)
+#define DBG_SKB(skb) (DBG_VAR 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(fmt, args...)
+#define DBG_FRAME(fmt, cf)
+#define DBG_SKB(skb)
+#endif
This non-standard debugging seems like it needs a better interface.
Also, need paren's around (DBG_VAR 1) and don't use UPPERCASE for
variable names.
No additional parenthesis is needed here. ?: is the lowest precedence
operator above assignment and ,. Also, DBG_VAR is no variable name.
It's a macro that expands to a variable name like can_debug, raw_debug
or bcm_debug.
+HLIST_HEAD(rx_dev_list);
Please either make rx_dev_list static or call it can_rx_dev_list
to avoid name conflices.
+static struct dev_rcv_lists rx_alldev_list;
+static DEFINE_SPINLOCK(rcv_lists_lock);
+
+static struct kmem_cache *rcv_cache __read_mostly;
+
+/* table of registered CAN protocols */
+static struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
+static DEFINE_SPINLOCK(proto_tab_lock);
+
+struct timer_list stattimer; /* timer for statistics update */
+struct s_stats stats; /* packet statistics */
+struct s_pstats pstats; /* receive list statistics */
More global variables without prefix.
These variables are not exported with EXPORT_SYMBOL, so there should
be no name conflict. They cannot be made static because they are used
in af_can.c and proc.c. Nevertheless we can prefix them with can_ if
you still think it's necessary.
+static int can_proc_read_stats(char *page, char **start, off_t off,
+ int count, int *eof, void *data)
+{
+}
The read interface should use seq_file interface rather than
formatting into page buffer.
Why? For this simple function a page buffer is enough space and the
seq_file API would require more effort. IMHO, seq_files offer
advantages if the proc file shows some sequence of data generated in
an iteration through some loop (see below).
+static int can_proc_read_reset_stats(char *page, char **start, off_t off,
+int count, int *eof, void *data)
+{
+}
Why not have a write interface to do the reset?
I haven't looked into writable proc files yet. Will do so.
+static int can_proc_read_rcvlist(char *page, char **start, off_t off,
+int count, int *eof, void *data)
+{
+ /* double cast to prevent GCC warning */
+ int idx = (int)(long)data;
+}
This is were I would prefer sequence files. However, the seq file
interface doesn't allow me to pass additional info like the `data'
argument. This means I would have to write separate functions
instead.
Output from checkpatch:
WARNING: do not add new typedefs
#116: FILE: include/linux/can.h:41:
+typedef __u32 canid_t;
WARNING: do not add new typedefs
#124: FILE: include/linux/can.h:49:
+typedef __u32 can_err_mask_t;
These typedef were considered OK in previous discussions on the list.
ERROR: use tabs not spaces
#498: FILE: net/can/af_can.c:159:
+^I^I^I^I not implemented.\n, module_name);$
Fixed.
WARNING: braces {} are not necessary for single statement blocks
#1080: FILE: net/can/af_can.c:741:
+ if (!proto_tab[proto]) {
+ printk(KERN_ERR BUG: can: protocol %d is not registered\n,
+proto);
+ }
Hm, isn't it common to use braces for single statements if they span
more than one line?
Thanks for your review.
urs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Joe Perches [EMAIL PROTECTED] writes:
On Thu, 2007-11-15 at 08:40 +0100, Oliver Hartkopp wrote:
Stephen Hemminger wrote:
+#ifdef CONFIG_CAN_DEBUG_CORE
+extern void can_debug_skb(struct sk_buff *skb);
+extern void can_debug_cframe(const char *msg, struct can_frame *cframe);
+#define DBG(fmt, args...) (DBG_VAR 1 ? printk( \
+KERN_DEBUG DBG_PREFIX : %s:
fmt, \
+__func__, ##args) : 0)
+#define DBG_FRAME(fmt, cf) (DBG_VAR 2 ? can_debug_cframe(fmt, cf) : 0)
+#define DBG_SKB(skb) (DBG_VAR 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(fmt, args...)
+#define DBG_FRAME(fmt, cf)
+#define DBG_SKB(skb)
+#endif
I would prefer the more frequently used macro style:
#define DBG(fmt, args...) \
do { if (DBG_VAR 1) printk(KERN_DEBUG DBG_PREFIX : %s: fmt, \
__func__, ##args); } while (0)
#define DBG_FRAME(fmt, cf) \
do { if (DBG_VAR 2) can_debug_cframe(fmt, cf); } while (0)
#define DBG_SKB(skb) \
do { if (DBG_VAR 4) can_debug_skb(skb); } while (0)
I prefer our code because it is shorter (fits into one line) and can
be used anywhere where an expression is allowed compared to only where
a statement is allowed. Actually, I first had
#define DBG( ... ) ((debug 1) printk( ... ))
and so on, but that didn't work with can_debug_{cframe,sbk} since they
return void.
Admitted, the benefit of expr vs. statement is really negligible and
since this issue has come up several times I will change these macros
using do-while.
urs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
From: Urs Thuermann [EMAIL PROTECTED]
Date: 15 Nov 2007 12:51:34 +0100
I prefer our code because it is shorter (fits into one line) and can
be used anywhere where an expression is allowed compared to only where
a statement is allowed. Actually, I first had
#define DBG( ... ) ((debug 1) printk( ... ))
and so on, but that didn't work with can_debug_{cframe,sbk} since they
return void.
Admitted, the benefit of expr vs. statement is really negligible and
since this issue has come up several times I will change these macros
using do-while.
I really frown upon these local debugging macros people tend to want
to submit with their changes.
It really craps up the tree, even though it might be useful to you.
So please remove this stuff or replace the debugging statements
with some generic kernel debugging facility, there are several.
Thank you.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
+ +struct timer_list stattimer; /* timer for statistics update */ +struct s_stats stats; /* packet statistics */ +struct s_pstats pstats; /* receive list statistics */ More global variables without prefix. These variables are not exported with EXPORT_SYMBOL, so there should be no name conflict. They cannot be made static because they are used in af_can.c and proc.c. Nevertheless we can prefix them with can_ if you still think it's necessary. When this is build-in they will be in the global kernel namespace. So please add can_ prefix. Sam - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Thu, Nov 15, 2007 at 04:05:30AM -0800, David Miller wrote:
From: Urs Thuermann [EMAIL PROTECTED]
Date: 15 Nov 2007 12:51:34 +0100
I prefer our code because it is shorter (fits into one line) and can
be used anywhere where an expression is allowed compared to only where
a statement is allowed. Actually, I first had
#define DBG( ... ) ((debug 1) printk( ... ))
and so on, but that didn't work with can_debug_{cframe,sbk} since they
return void.
Admitted, the benefit of expr vs. statement is really negligible and
since this issue has come up several times I will change these macros
using do-while.
I really frown upon these local debugging macros people tend to want
to submit with their changes.
It really craps up the tree, even though it might be useful to you.
So please remove this stuff or replace the debugging statements
with some generic kernel debugging facility, there are several.
It would be usefull if someone could make a short intro to the preferred
ones and we could stuff it in Documentation/*
Had same comment but had nowhere to point the can guys at.
Sam
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Wed, 14 Nov 2007 13:13:41 +0100
Urs Thuermann [EMAIL PROTECTED] wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Urs Thuermann [EMAIL PROTECTED]
---
include/linux/can.h | 111 +
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 | 975
++
net/can/af_can.h | 120 +
net/can/proc.c| 532 +
10 files changed, 1942 insertions(+)
Index: net-2.6.25/include/linux/can/core.h
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ net-2.6.25/include/linux/can/core.h 2007-11-14 13:04:49.0
+0100
@@ -0,0 +1,78 @@
+/*
+ * linux/can/core.h
+ *
+ * Protoypes and definitions for CAN protocol modules using the PF_CAN core
+ *
+ * Authors: Oliver Hartkopp [EMAIL PROTECTED]
+ * Urs Thuermann [EMAIL PROTECTED]
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Send feedback to [EMAIL PROTECTED]
+ *
+ */
+
+#ifndef CAN_CORE_H
+#define CAN_CORE_H
+
+#include linux/can.h
+#include linux/skbuff.h
+#include linux/netdevice.h
+
+#define CAN_VERSION 20071027
+
+/* increment this number each time you change some user-space interface */
+#define CAN_ABI_VERSION 8
+
+#define CAN_VERSION_STRING rev CAN_VERSION abi CAN_ABI_VERSION
+
+#define DNAME(dev) ((dev) ? (dev)-name : any)
+
+/**
+ * struct can_proto - CAN protocol structure
+ * @type: type argument in socket() syscall, e.g. SOCK_DGRAM.
+ * @protocol: protocol number in socket() syscall.
+ * @capability: capability needed to open the socket, or -1 for no
restriction.
+ * @ops:pointer to struct proto_ops for sock-ops.
+ * @prot: pointer to struct proto structure.
+ */
+struct can_proto {
+ int type;
+ int protocol;
+ int capability;
+ struct proto_ops *ops;
+ struct proto *prot;
+};
+
+/* function prototypes for the CAN networklayer core (af_can.c) */
+
+extern int can_proto_register(struct can_proto *cp);
+extern void can_proto_unregister(struct can_proto *cp);
+
+extern int can_rx_register(struct net_device *dev, canid_t can_id,
+ canid_t mask,
+ void (*func)(struct sk_buff *, void *),
+ void *data, char *ident);
+
+extern void can_rx_unregister(struct net_device *dev, canid_t can_id,
+ canid_t mask,
+ void (*func)(struct sk_buff *, void *),
+ void *data);
+
+extern int can_send(struct sk_buff *skb, int loop);
+
+#ifdef CONFIG_CAN_DEBUG_CORE
+extern void can_debug_skb(struct sk_buff *skb);
+extern void can_debug_cframe(const char *msg, struct can_frame *cframe);
+#define DBG(fmt, args...) (DBG_VAR 1 ? printk( \
+ KERN_DEBUG DBG_PREFIX : %s: fmt, \
+ __func__, ##args) : 0)
+#define DBG_FRAME(fmt, cf) (DBG_VAR 2 ? can_debug_cframe(fmt, cf) : 0)
+#define DBG_SKB(skb) (DBG_VAR 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(fmt, args...)
+#define DBG_FRAME(fmt, cf)
+#define DBG_SKB(skb)
+#endif
This non-standard debugging seems like it needs a better interface.
Also, need paren's around (DBG_VAR 1) and don't use UPPERCASE for
variable names.
+
+#endif /* CAN_CORE_H */
Index: net-2.6.25/net/Kconfig
===
--- net-2.6.25.orig/net/Kconfig 2007-11-14 13:04:26.0 +0100
+++ net-2.6.25/net/Kconfig2007-11-14 13:04:49.0 +0100
@@ -218,6 +218,7 @@
endmenu
source net/ax25/Kconfig
+source net/can/Kconfig
source net/irda/Kconfig
source net/bluetooth/Kconfig
source net/rxrpc/Kconfig
Index: net-2.6.25/net/Makefile
===
--- net-2.6.25.orig/net/Makefile 2007-11-14 13:04:26.0 +0100
+++ net-2.6.25/net/Makefile 2007-11-14 13:04:49.0 +0100
@@ -34,6 +34,7 @@
obj-$(CONFIG_NETROM) += netrom/
obj-$(CONFIG_ROSE) += rose/
obj-$(CONFIG_AX25) += ax25/
+obj-$(CONFIG_CAN)+= can/
obj-$(CONFIG_IRDA) += irda/
obj-$(CONFIG_BT) += bluetooth/
obj-$(CONFIG_SUNRPC) += sunrpc/
Index: net-2.6.25/net/can/Kconfig
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Stephen Hemminger wrote:
+#ifdef CONFIG_CAN_DEBUG_CORE
+extern void can_debug_skb(struct sk_buff *skb);
+extern void can_debug_cframe(const char *msg, struct can_frame *cframe);
+#define DBG(fmt, args...) (DBG_VAR 1 ? printk( \
+KERN_DEBUG DBG_PREFIX : %s: fmt, \
+__func__, ##args) : 0)
+#define DBG_FRAME(fmt, cf) (DBG_VAR 2 ? can_debug_cframe(fmt, cf) : 0)
+#define DBG_SKB(skb) (DBG_VAR 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(fmt, args...)
+#define DBG_FRAME(fmt, cf)
+#define DBG_SKB(skb)
+#endif
This non-standard debugging seems like it needs a better interface.
Also, need paren's around (DBG_VAR 1) and don't use UPPERCASE for
variable names.
Of course we do not use UPPERCASE variable names ;-)
The DBG_VAR define points to a module specific named debug variable,
e.g. in can-raw :
+#ifdef CONFIG_CAN_DEBUG_CORE
+#define DBG_PREFIX can-raw
+#define DBG_VARraw_debug
+static int raw_debug;
+module_param_named(debug, raw_debug, int, S_IRUGO);
+MODULE_PARM_DESC(debug, debug print mask: 1:debug, 2:frames, 4:skbs);
+#endif
This has been introduced in try#10 to have module specific debug
variable names requested by Arnaldo and Dave.
Regarding the paren's: I love adding paren's, Urs doesn't. But in this
case, there should be no reason for them - or am i wrong here?
Output from checkpatch:
WARNING: do not add new typedefs
#116: FILE: include/linux/can.h:41:
+typedef __u32 canid_t;
WARNING: do not add new typedefs
#124: FILE: include/linux/can.h:49:
+typedef __u32 can_err_mask_t;
These definitions are structure definitions and pretty ok for that
reason - we had that discussion.
Please look into include/linux/can.h for details and if it looks ok for
you also then.
ERROR: use tabs not spaces
#498: FILE: net/can/af_can.c:159:
+^I^I^I^I not implemented.\n, module_name);$
Oops! Will be fixed.
WARNING: braces {} are not necessary for single statement blocks
#1080: FILE: net/can/af_can.c:741:
+ if (!proto_tab[proto]) {
+ printk(KERN_ERR BUG: can: protocol %d is not registered\n,
+proto);
+ }
Dito.
Thanks for the review, Stephen.
I'll go through your other remarks with Urs today.
Oliver.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Arnaldo Carvalho de Melo [EMAIL PROTECTED] writes:
+struct sockaddr_can {
+ sa_family_t can_family;
+ int can_ifindex;
+ union {
+ struct { canid_t rx_id, tx_id; } tp16;
+ struct { canid_t rx_id, tx_id; } tp20;
+ struct { canid_t rx_id, tx_id; } mcnet;
+ struct { canid_t rx_id, tx_id; } isotp;
+ } can_addr;
Again being curious, what is the value of this union of all its members
have the same definition? Backward source code compatibility?
As Oliver already wrote, different CAN transport protocols may use
different sockaddr structures. Therefore, we have made can_addr a
union. The four we have defined already, all look the same, but
other, future protocols may define a different structure.
+struct can_proto {
+ int type;
+ int protocol;
+ int capability;
+ struct proto_ops *ops;
+ struct proto *prot;
+};
+
+/* function prototypes for the CAN networklayer core (af_can.c) */
+
+extern int can_proto_register(struct can_proto *cp);
+extern void can_proto_unregister(struct can_proto *cp);
We have proto registering infrastructure for bluetooth, inet and now
CAN, have you looked at:
struct inet_protosw;
proto_{register,unregister}, etc?
Yes, I know inet_protosw and inet_{,un}register_protosw(). But we
can't use inet_register_protosw().
And can_proto_register() does use proto_register(). What exactly do
you want to suggest?
urs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Em Thu, Oct 04, 2007 at 01:51:47PM +0200, Urs Thuermann escreveu:
Arnaldo Carvalho de Melo [EMAIL PROTECTED] writes:
+struct sockaddr_can {
+ sa_family_t can_family;
+ int can_ifindex;
+ union {
+ struct { canid_t rx_id, tx_id; } tp16;
+ struct { canid_t rx_id, tx_id; } tp20;
+ struct { canid_t rx_id, tx_id; } mcnet;
+ struct { canid_t rx_id, tx_id; } isotp;
+ } can_addr;
Again being curious, what is the value of this union of all its members
have the same definition? Backward source code compatibility?
As Oliver already wrote, different CAN transport protocols may use
different sockaddr structures. Therefore, we have made can_addr a
union. The four we have defined already, all look the same, but
other, future protocols may define a different structure.
+struct can_proto {
+ int type;
+ int protocol;
+ int capability;
+ struct proto_ops *ops;
+ struct proto *prot;
+};
+
+/* function prototypes for the CAN networklayer core (af_can.c) */
+
+extern int can_proto_register(struct can_proto *cp);
+extern void can_proto_unregister(struct can_proto *cp);
We have proto registering infrastructure for bluetooth, inet and now
CAN, have you looked at:
struct inet_protosw;
proto_{register,unregister}, etc?
Yes, I know inet_protosw and inet_{,un}register_protosw(). But we
can't use inet_register_protosw().
And can_proto_register() does use proto_register(). What exactly do
you want to suggest?
Sorry, I was in a hurry and didn't completed my thoughts on how to share
more code and data structures.
My first reaction was: hey, struct can_proto has almost the same
definition as struct inet_protosw, and can_proto_register() looks like
inet_register_protosw().
can_proto_register() calls proto_register, inet_register_protosw
doesn't. But protocols such as DCCP and SCTP, call both
inet_register_protosw and proto_register. Perhaps we can make
inet_register_protosw behave like can_proto_register and do the
proto_register(inet_protosw-prot) for us.
Looking at inet_init in net/ipv4/af_inet.c we see that we do the same
for udp, tcp and raw too. There we also call proto_register +
inet_register_protosw.
See the possibilites for code sharing? Having just one way of
registering protocols would reduce complexity for new protocol writers
and for people that browse the code only when trying to fix some problem
and don't want to get lost in many ways of doing the same thing.
struct can_proto could be removed and struct inet_protosw could be
renamed to reflect the fact that it is, after all, not inet specific at
all.
So this is not something to fix on your implementation. It looks OK.
But we could use more hands on reducing complexity on the Linux network
protocol infrastructure and you would get free fixes and improvements
when people improve the inet protocols 8)
DCCP has been collecting dividends for quite a while for working like
that 8)
- Arnaldo
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Em Tue, Oct 02, 2007 at 03:10:08PM +0200, Urs Thuermann escreveu:
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 [EMAIL PROTECTED]
Signed-off-by: Urs Thuermann [EMAIL PROTECTED]
---
include/linux/can.h | 111 +
include/linux/can/core.h | 77 +++
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 | 970
++
net/can/af_can.h | 120 +
net/can/proc.c| 532 +
10 files changed, 1936 insertions(+)
Index: net-2.6.24/include/linux/can.h
===
--- /dev/null 1970-01-01 00:00:00.0 +
+++ net-2.6.24/include/linux/can.h2007-10-02 12:10:18.0 +0200
@@ -0,0 +1,111 @@
+/*
+ * linux/can.h
+ *
+ * Definitions for CAN network layer (socket addr / CAN frame / CAN filter)
+ *
+ * Authors: Oliver Hartkopp [EMAIL PROTECTED]
+ * Urs Thuermann [EMAIL PROTECTED]
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Send feedback to [EMAIL PROTECTED]
+ *
+ */
+
+#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 0x8000U /* EFF/SFF is set in the MSB */
+#define CAN_RTR_FLAG 0x4000U /* remote transmission request */
+#define CAN_ERR_FLAG 0x2000U /* error frame */
+
+/* valid bits in CAN ID for frame formats */
+#define CAN_SFF_MASK 0x07FFU /* standard frame format (SFF) */
+#define CAN_EFF_MASK 0x1FFFU /* extended frame format (EFF) */
+#define CAN_ERR_MASK 0x1FFFU /* omit EFF, RTR, ERR flags */
+
+/*
+ * Controller Area Network Identifier structure
+ *
+ * bit 0-28 : CAN identifier (11/29 bit)
+ * bit 29: error frame flag (0 = data frame, 1 = error frame)
+ * bit 30: remote transmission request flag (1 = rtr frame)
+ * bit 31: frame format flag (0 = standard 11 bit, 1 = extended 29 bit)
+ */
+typedef __u32 canid_t;
+
+/*
+ * Controller Area Network Error Frame Mask structure
+ *
+ * bit 0-28 : error class mask (see include/linux/can/error.h)
+ * bit 29-31 : set to zero
+ */
+typedef __u32 can_err_mask_t;
+
+/**
+ * struct can_frame - basic CAN frame structure
+ * @can_id: the CAN ID of the frame and CAN_*_FLAG flags, see above.
+ * @can_dlc: the data length field of the CAN frame
+ * @data:the CAN frame payload.
+ */
+struct can_frame {
+ canid_t can_id; /* 32 bit CAN_ID + EFF/RTR/ERR flags */
+ __u8can_dlc; /* data length code: 0 .. 8 */
+ __u8data[8] __attribute__((aligned(8)));
+};
+
+/* particular protocols of the protocol family PF_CAN */
+#define CAN_RAW 1 /* RAW sockets */
+#define CAN_BCM 2 /* Broadcast Manager */
+#define CAN_TP16 3 /* VAG Transport Protocol v1.6 */
+#define CAN_TP20 4 /* VAG Transport Protocol v2.0 */
+#define CAN_MCNET5 /* Bosch MCNet */
+#define CAN_ISOTP6 /* ISO 15765-2 Transport Protocol */
+#define CAN_NPROTO 7
+
+#define SOL_CAN_BASE 100
+
+/**
+ * struct sockaddr_can - the sockaddr structure for CAN sockets
+ * @can_family: address family number AF_CAN.
+ * @can_ifindex: CAN network interface index.
+ * @can_addr:transport protocol specific address, mostly CAN IDs.
+ */
+struct sockaddr_can {
+ sa_family_t can_family;
+ int can_ifindex;
+ union {
+ struct { canid_t rx_id, tx_id; } tp16;
+ struct { canid_t rx_id, tx_id; } tp20;
+ struct { canid_t rx_id, tx_id; } mcnet;
+ struct { canid_t rx_id, tx_id; } isotp;
+ } can_addr;
Again being curious, what is the value of this union of all its members
have the same definition? Backward source code compatibility?
+};
+
+/**
+ * struct can_filter - CAN ID based filter in can_register().
+ * @can_id: relevant bits of CAN ID which are not masked out.
+ * @can_mask: CAN mask (see description)
+ *
+ * Description:
+ * A filter matches, when
+ *
+ * received_can_id mask == can_id mask
+ *
+ * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
+ * filter for error frames (CAN_ERR_FLAG bit set in mask).
+ */
+struct can_filter {
+ canid_t can_id;
+ canid_t can_mask;
+};
+
+#define CAN_INV_FILTER 0x2000U /* to be set in can_filter.can_id */
+
+#endif /* CAN_H */
Index: net-2.6.24/include/linux/can/core.h
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Tue, 2007-09-25 at 14:09 -0700, David Miller wrote: Then please make all exported symbols marked EXPORT_SYMBOL_GPL to make sure that the other CAN protocol can not reuse your infrastructure. We don't want to force other CAN protocol implementations to be GPL also. AFAIR from discussions on LKML, it was mostly agreed upon that this decision is up to the authors of code. To a certain extent, yes. However, the core issue is whether anyone who uses the symbol is creating a derivative work. If it is pretty clear that this is the case, you really should mark the exported symbols GPL. In my opinion, in this case it is pretty clear that any use of these new symbols would be a derivative work and therefore they all should be marked GPL. Hmm, the code in question is dual licensed. So it's not that clear to me. But it's a legal grey area anyway if somebody loads non GPL code into the kernel, though IANAL and we can spend years on this discussion. I'm not inclined either way and we really should not make this a religious question whether that code goes in or not, especially not when we granted the mac80211 to export everything w/o _GPL suffix not too long ago. Thanks, tglx - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
From: Thomas Gleixner [EMAIL PROTECTED] Date: Fri, 28 Sep 2007 18:27:19 +0200 I'm not inclined either way and we really should not make this a religious question whether that code goes in or not, especially not when we granted the mac80211 to export everything w/o _GPL suffix not too long ago. This is because a wireless driver is a driver. It can exist outside of the kernel and it's mac80211 stack just like any other network device driver. ANd the interfaces in mac80211 are such that the driver doesn't explicitly go into the internals of the implementation. That's not true with CAN. With this CAN stuff, any driver you write for it is intimately integrated into the design and architecture of the CAN subsystem. Any such driver cannot stand on it's own. Look at how these drivers can get into the internals. If this code goes in without the _GPL() exports, that's fine, but it's setting incorrect expectations for people who think they can write binary-only drivers and link to these symbols. And it will be the CAN folks who are guilty of setting these false premises. Especially after I've explicitly warned about it here. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Fri, 2007-09-28 at 13:20 -0700, David Miller wrote: That's not true with CAN. With this CAN stuff, any driver you write for it is intimately integrated into the design and architecture of the CAN subsystem. Any such driver cannot stand on it's own. Look at how these drivers can get into the internals. I'm just concerned about protocols, which have been designed and implemented long ago outside of the kernel and are going to be wrapped with glue code to fit into the socket can implementation. That's hard to judge. If this code goes in without the _GPL() exports, that's fine, but it's setting incorrect expectations for people who think they can write binary-only drivers and link to these symbols. And it will be the CAN folks who are guilty of setting these false premises. Especially after I've explicitly warned about it here. Fair enough. tglx - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Em Tue, Sep 25, 2007 at 02:20:31PM +0200, Urs Thuermann escreveu:
+
+static void can_sock_destruct(struct sock *sk)
+{
+ DBG(called for sock %p\n, sk);
+
+ skb_queue_purge(sk-sk_receive_queue);
+ if (sk-sk_protinfo)
+ kfree(sk-sk_protinfo);
+}
Is it really needed to do this sk_protinfo check? You don't use it and
it is going to be removed (only user is one of the HAM radio protocols),
so it would be better to not add any more references to this struct sock
field.
- Arnaldo
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Arnaldo Carvalho de Melo [EMAIL PROTECTED] writes: + skb_queue_purge(sk-sk_receive_queue); + if (sk-sk_protinfo) + kfree(sk-sk_protinfo); +} Is it really needed to do this sk_protinfo check? Thanks for finding this. This is from 2.6.12 times or so. We have other CAN protocol (which we are not allowed to put under GPL) implemenatations which still use the protinfo field. But we should change those and I will delete this check. urs - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On 25 Sep 2007 15:24:33 +0200 Urs Thuermann [EMAIL PROTECTED] wrote: Arnaldo Carvalho de Melo [EMAIL PROTECTED] writes: + skb_queue_purge(sk-sk_receive_queue); + if (sk-sk_protinfo) + kfree(sk-sk_protinfo); +} Is it really needed to do this sk_protinfo check? Thanks for finding this. This is from 2.6.12 times or so. We have other CAN protocol (which we are not allowed to put under GPL) implemenatations which still use the protinfo field. But we should change those and I will delete this check. urs Then please make all exported symbols marked EXPORT_SYMBOL_GPL to make sure that the other CAN protocol can not reuse your infrastructure. -- Stephen Hemminger [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Stephen Hemminger [EMAIL PROTECTED] writes: Then please make all exported symbols marked EXPORT_SYMBOL_GPL to make sure that the other CAN protocol can not reuse your infrastructure. We don't want to force other CAN protocol implementations to be GPL also. AFAIR from discussions on LKML, it was mostly agreed upon that this decision is up to the authors of code. urs - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On 25 Sep 2007 23:00:15 +0200 Urs Thuermann [EMAIL PROTECTED] wrote: Stephen Hemminger [EMAIL PROTECTED] writes: Then please make all exported symbols marked EXPORT_SYMBOL_GPL to make sure that the other CAN protocol can not reuse your infrastructure. We don't want to force other CAN protocol implementations to be GPL also. AFAIR from discussions on LKML, it was mostly agreed upon that this decision is up to the authors of code. urs I just don't want proprietary drivers extensions in linux kernel. EXPORT_SYMBOL has no meaning in other OS environments. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
From: Urs Thuermann [EMAIL PROTECTED] Date: 25 Sep 2007 23:00:15 +0200 Stephen Hemminger [EMAIL PROTECTED] writes: Then please make all exported symbols marked EXPORT_SYMBOL_GPL to make sure that the other CAN protocol can not reuse your infrastructure. We don't want to force other CAN protocol implementations to be GPL also. AFAIR from discussions on LKML, it was mostly agreed upon that this decision is up to the authors of code. To a certain extent, yes. However, the core issue is whether anyone who uses the symbol is creating a derivative work. If it is pretty clear that this is the case, you really should mark the exported symbols GPL. In my opinion, in this case it is pretty clear that any use of these new symbols would be a derivative work and therefore they all should be marked GPL. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Joe Perches [EMAIL PROTECTED] writes:
I'd prefer something like this, which removes the unnecessary
kmalloc/kfree pairs or the equivalent conversions to functions.
I have changed this to a static buffer. Since this is only in
#ifdef CONFIG_CAN_DEBUG_CORE, it shouldn't hurt.
#define can_debug_cframe(cf, fmt, arg...) \
do { \
char buf[20]; \
int dlc = cf-can_dlc; \
if (dlc 8) \
dlc = 8; \
if (cf-can_id CAN_EFF_FLAG) \
sprintf(buf, %08X [%X] , cf-can_id CAN_EFF_MASK, dlc); \
else \
sprintf(buf, %03X [%X] , cf-can_id CAN_SFF_MASK, dlc); \
printk(KERN_DEBUG fmt, ##arg); \
print_hex_dump(buf, DUMP_PREFIX_NONE, cf-data, dlc); \
} while (0)
The line isn't printed atomically, i.e. it could happen that something
is printed between the fmt+args and the hexdump, i.e inbetween the
line.
and
#define can_debug_skb(skb) \
do { \
pr_debug(skbuff at %p, dev: %d, proto: %04x\n, \
skb, skb-dev ? skb-dev-ifindex : -1, \
ntohs(skb-protocol)); \
pr_debug(users: %d, dataref: %d, nr_frags: %d, \
h,d,t,e,l: %p %+d %+d %+d, %d\n, \
atomic_read(skb-users), \
atomic_read((skb_shinfo(skb)-dataref)), \
skb_shinfo(skb)-nr_frags, \
skb-head, skb-data - skb-head, \
skb-tail - skb-head, skb-end - skb-head, skb-len); \
print_hex_dump(KERN_DEBUG, skb_head: , DUMP_PREFIX_NONE, \
16, 1, skb-head, skb-end - skb-head); \
} while (0)
Here, the consecutive lines aren't printed atomically.
I think sprintf() to a buffer first and the do one printk() is
better. But I will use hex_dump_to_buffer().
urs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Urs Thuermann wrote: Patrick McHardy [EMAIL PROTECTED] writes: You drop the module reference again when leaving this function. So sock-ops might contain a stale pointer if the module is unloaded after this. You need to either keep the module reference while the socket is alive or remove stale references when unregistering the protocol. I don't think that can happen. Before we drop the module reference we call sk_alloc() which gets another module reference via its cp-prot argument. If sk_alloc() fails we return with error from can_create() I assume sock-ops won't be used after that. You're right, that should be enough. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Joe Perches [EMAIL PROTECTED] writes:
On Thu, 2007-09-20 at 20:43 +0200, Urs Thuermann wrote:
+#define DBG(...) (debug 1 ? \
+ (printk(KERN_DEBUG can-%s %s: , \
+ IDENT, __func__), printk(args)) : 0)
+#define DBG_FRAME(args...) (debug 2 ? can_debug_cframe(args) : 0)
+#define DBG_SKB(skb) (debug 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(args...)
+#define DBG_FRAME(args...)
+#define DBG_SKB(skb)
+#endif
Shouldn't these be like the more common
#define DBG(fmt, args...) \
When I wrote these macros we had one platform where we still used
linux-2.4 and gcc-2.95.3 and with that old gcc your macro def would
mean you can't use DBG with only one argument, like DBG(some string\n);
since gcc-2.95 would leave the , after __func__ followed by the
closing ). I didn't find a way with gcc-2.95 to make the format
string a separate macro argument (which I also wanted). OK, that's
history now and linux-2.6 needs gcc-3/4 anyway, so I will change this.
do { \
if (debug 1) \
printk(KERN_DEBUG can-%s %s: fmt, IDENT, __func__, ##args); \
} while (0)
I use do { ... } while(0) only for statements, not for expressions.
But I could have used the instead of ?: operator. I don't think
the do { ... } while(0) looks nicer or has any other advantage.
void can_debug_cframe(const char *msg, struct can_frame *cf, ...)
This prototype looks backwards to me.
You mean the order or `msg' and `cf'? You want to switch them so that
the variable args follow immediately the format string? Might make
sense, OTOH we wanted to have the message as the first argument.
I'd prefer something like this, which removes the unnecessary
kmalloc/kfree pairs or the equivalent conversions to functions.
#define can_debug_cframe(cf, fmt, arg...) \
do { \
char buf[20]; \
int dlc = cf-can_dlc; \
if (dlc 8) \
dlc = 8; \
if (cf-can_id CAN_EFF_FLAG) \
sprintf(buf, %08X [%X] , cf-can_id CAN_EFF_MASK, dlc); \
else \
sprintf(buf, %03X [%X] , cf-can_id CAN_SFF_MASK, dlc); \
printk(KERN_DEBUG fmt, ##arg); \
print_hex_dump(buf, DUMP_PREFIX_NONE, cf-data, dlc); \
} while (0)
Ah, I didn't know print_hex_dump(). That looks nicer. But as Thomas
mentioned, we shouldn't convert these functions into macros.
urs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Urs Thuermann wrote:
+static int can_create(struct net *net, struct socket *sock, int protocol)
+{
+ ...
+
+ spin_lock(proto_tab_lock);
+ cp = proto_tab[protocol];
+ if (cp !try_module_get(cp-prot-owner))
+ cp = NULL;
+ spin_unlock(proto_tab_lock);
+
+ /* check for success and correct type */
+ if (!cp || cp-type != sock-type) {
+ ret = -EPROTONOSUPPORT;
+ goto errout;
+ }
+
+ if (cp-capability = 0 !capable(cp-capability)) {
+ ret = -EPERM;
+ goto errout;
+ }
+
+ sock-ops = cp-ops;
You drop the module reference again when leaving this function.
So sock-ops might contain a stale pointer if the module is
unloaded after this. You need to either keep the module reference
while the socket is alive or remove stale references when
unregistering the protocol.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Fri, 2007-09-21 at 12:35 +0200, Urs Thuermann wrote:
I didn't find a way with gcc-2.95 to make the format
string a separate macro argument (which I also wanted).
The old 2.x GCC workaround was to use
#define DBG(fmt, arg) printk(fmt , ## arg)
adding a space before the last comma.
I use do { ... } while(0) only for statements, not for expressions.
But I could have used the instead of ?: operator. I don't think
the do { ... } while(0) looks nicer or has any other advantage.
It's more linux convention.
It allows the macro to be used in if-else constructs.
void can_debug_cframe(const char *msg, struct can_frame *cf, ...)
This prototype looks backwards to me.
You mean the order or `msg' and `cf'?
Yes, I believe the can_frame* should be the first argument.
Ah, I didn't know print_hex_dump(). That looks nicer. But as Thomas
mentioned, we shouldn't convert these functions into macros.
The first print_hex_dump should actually be print_hex_dump_bytes.
I was typing what I hoped was a readable example.
I hope you do convert to functions, but not to inline functions.
cheers, Joe
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Patrick McHardy [EMAIL PROTECTED] writes: You drop the module reference again when leaving this function. So sock-ops might contain a stale pointer if the module is unloaded after this. You need to either keep the module reference while the socket is alive or remove stale references when unregistering the protocol. I don't think that can happen. Before we drop the module reference we call sk_alloc() which gets another module reference via its cp-prot argument. If sk_alloc() fails we return with error from can_create() I assume sock-ops won't be used after that. urs - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
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: Patrick McHardy [EMAIL PROTECTED] writes: When the module is unloaded it calls can_proto_unregister() which clears the pointer. Do you see a race condition here? Yes, you do request_module, load the module, get the cp pointer from proto_tab, the module is unloaded again. cp points to stable memory. Using module references would fix this. 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 loading would normally be successful. How can I prevent that? 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_lock_bh(). The _bh variant, because can_rcv() runs in interrupt and we need to block that. I thought this is pretty standard. I'll check this again tomorrow, but I have put much time in these locking issues already, changed it quite a few times and hoped to have got it right finally. 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 list changes. 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Urs Thuermann wrote: Patrick McHardy [EMAIL PROTECTED] writes: When the module is unloaded it calls can_proto_unregister() which clears the pointer. Do you see a race condition here? Yes, you do request_module, load the module, get the cp pointer from proto_tab, the module is unloaded again. cp points to stable memory. Using module references would fix this. 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 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 release lock 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 loading would normally be successful. How can I prevent that? Why do you want to prevent it? The admin unloaded the module, so he apparently doesn't want the operation to succeed. 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_lock_bh(). The _bh variant, because can_rcv() runs in interrupt and we need to block that. I thought this is pretty standard. I'll check this again tomorrow, but I have put much time in these locking issues already, changed it quite a few times and hoped to have got it right finally. 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 list changes. 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 saying you need _rcu for the *read side*. All operations changing the list already use the _rcu variants. I'm sorry if I miss something obvious here, but could you try to explain it to me? 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Patrick McHardy [EMAIL PROTECTED] writes: 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 release lock Ah, ok. Thanks for that hint. I will add it that way. 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 loading would normally be successful. How can I prevent that? Why do you want to prevent it? The admin unloaded the module, so he apparently doesn't want the operation to succeed. 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 described above would also solve this. I'm saying you need _rcu for the *read side*. All operations changing the list already use the _rcu variants. I'm sorry if I miss something obvious here, but could you try to explain it to me? 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, 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Thu, 2007-09-20 at 20:43 +0200, Urs Thuermann wrote:
+#define DBG(...) (debug 1 ? \
+ (printk(KERN_DEBUG can-%s %s: , \
+ IDENT, __func__), printk(args)) : 0)
+#define DBG_FRAME(args...) (debug 2 ? can_debug_cframe(args) : 0)
+#define DBG_SKB(skb) (debug 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(args...)
+#define DBG_FRAME(args...)
+#define DBG_SKB(skb)
+#endif
Shouldn't these be like the more common
#define DBG(fmt, args...) \
do { \
if (debug 1) \
printk(KERN_DEBUG can-%s %s: fmt, IDENT, __func__, ##args); \
} while (0)
#define DBG_FRAME(cf, fmt, args...) \
do { \
if (debug 2) \
can_debug_cframe(cf, fmt, ##args); \
} while (0)
#define DBG_SKB(skb) \
do { \
if (debug 4) \
can_debug_skb(skb); \
while (0)
void can_debug_cframe(const char *msg, struct can_frame *cf, ...)
This prototype looks backwards to me.
msg is a printf format string.
I'd prefer something like this, which removes the unnecessary
kmalloc/kfree pairs or the equivalent conversions to functions.
#define can_debug_cframe(cf, fmt, arg...) \
do { \
char buf[20]; \
int dlc = cf-can_dlc; \
if (dlc 8) \
dlc = 8; \
if (cf-can_id CAN_EFF_FLAG) \
sprintf(buf, %08X [%X] , cf-can_id CAN_EFF_MASK, dlc); \
else \
sprintf(buf, %03X [%X] , cf-can_id CAN_SFF_MASK, dlc); \
printk(KERN_DEBUG fmt, ##arg); \
print_hex_dump(buf, DUMP_PREFIX_NONE, cf-data, dlc); \
} while (0)
and
#define can_debug_skb(skb) \
do { \
pr_debug(skbuff at %p, dev: %d, proto: %04x\n, \
skb, skb-dev ? skb-dev-ifindex : -1, \
ntohs(skb-protocol)); \
pr_debug(users: %d, dataref: %d, nr_frags: %d, \
h,d,t,e,l: %p %+d %+d %+d, %d\n, \
atomic_read(skb-users), \
atomic_read((skb_shinfo(skb)-dataref)), \
skb_shinfo(skb)-nr_frags, \
skb-head, skb-data - skb-head, \
skb-tail - skb-head, skb-end - skb-head, skb-len); \
print_hex_dump(KERN_DEBUG, skb_head: , DUMP_PREFIX_NONE, \
16, 1, skb-head, skb-end - skb-head); \
} while (0)
cheers, Joe
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
On Thu, 2007-09-20 at 13:06 -0700, Joe Perches wrote:
On Thu, 2007-09-20 at 20:43 +0200, Urs Thuermann wrote:
+#define DBG(...) (debug 1 ? \
+ (printk(KERN_DEBUG can-%s %s: , \
+ IDENT, __func__), printk(args)) : 0)
+#define DBG_FRAME(args...) (debug 2 ? can_debug_cframe(args) : 0)
+#define DBG_SKB(skb) (debug 4 ? can_debug_skb(skb) : 0)
+#else
+#define DBG(args...)
+#define DBG_FRAME(args...)
+#define DBG_SKB(skb)
+#endif
Shouldn't these be like the more common
#define DBG(fmt, args...) \
I'd prefer something like this, which removes the unnecessary
kmalloc/kfree pairs or the equivalent conversions to functions.
#define can_debug_cframe(cf, fmt, arg...) \
That's even worse. If we want to have something better than the DBG
macros, then please convert the whole stuff into inline functions.
#ifdef DEBUG
static inline void debug_skb(struct sk_buff *skb)
{
if (debug 4)
...
}
#else
static inline debug_skb(struct sk_buff *skb) {}
#endif
Thanks,
tglx
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Urs Thuermann wrote:
Patrick McHardy [EMAIL PROTECTED] writes:
+HLIST_HEAD(rx_dev_list);
Same here (static).
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
ELF section? Where is that finally located?
Its a seperate section to prevent false sharing.
+if (ret == -ENOSYS)
+printk(KERN_INFO can: request_module(%s) not
+implemented.\n, module_name);
+else if (ret)
+printk(KERN_ERR can: request_module(%s) failed\n,
+ module_name);
Both of these printks seem to be user-triggerable, so they should
be rate-limited (or maybe get removed completely/changed to DBG).
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
per proto number. I will think about this.
IMO this is a perfectly normal condition (not finding a module).
Especially the !KMOD case is hardly an error.
+/* check for success and correct type */
+cp = proto_tab[protocol];
What prevents the module from getting unloaded again (and using
a stale pointer)?
When the module is unloaded it calls can_proto_unregister() which
clears the pointer. Do you see a race condition here?
Yes, you do request_module, load the module, get the cp pointer
from proto_tab, the module is unloaded again. cp points to
stable memory. Using module references would fix this.
+if (!(skb-dev-flags IFF_LOOPBACK)) {
+/*
+ * If the interface is not capable to do loopback
+ * itself, we do it here.
+ */
+struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
+
+if (!newskb) {
+kfree_skb(skb);
+return -ENOMEM;
+}
+
+newskb-sk = skb-sk;
+newskb-ip_summed = CHECKSUM_UNNECESSARY;
+newskb-pkt_type = PACKET_BROADCAST;
+netif_rx(newskb);
So the intention here is to send the packet to the non-loopback device
and manually loop it, which means sending it twice?
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
core module. It is nothing a normal user will ever use.
Thanks for the explanation.
+static struct dev_rcv_lists *find_dev_rcv_lists(struct net_device *dev)
+{
+struct dev_rcv_lists *d;
+struct hlist_node *n;
+
+
+
+hlist_for_each_entry(d, n, rx_dev_list, list) {
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
something.
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_lock_bh(). The
_bh variant, because can_rcv() runs in interrupt and we need to block
that. I thought this is pretty standard.
I'll check this again tomorrow, but I have put much time in these
locking issues already, changed it quite a few times and hoped to have
got it right finally.
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
list changes.
+case NETDEV_REGISTER:
+
+/*
+ * create new dev_rcv_lists for this device
+ *
+ * N.B. zeroing the struct is the correct initialization
+ * for the embedded hlist_head structs.
+ * Another list type, e.g. list_head, would require
+ * explicit initialization.
+ */
+
+DBG(creating new dev_rcv_lists for %s\n, dev-name);
+
+d = kzalloc(sizeof(*d),
+in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
netdevice registration should never happen from interrupt handlers.
Hm, I seem to remember we had such
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Urs Thuermann wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Urs Thuermann [EMAIL PROTECTED]
Looks pretty good, please see below for a few comments (mostly minor
nitpicking, a few things that look like real bugs). Nothing that
couldn't be fixed after merging though.
+++ net-2.6.24/include/linux/can.h2007-09-17 10:27:09.0 +0200
@@ -0,0 +1,113 @@
+/*
+ * linux/can.h
+ *
+ * Definitions for CAN networklayer (socket addr / CAN frame / CAN filter)
+ *
+ * Authors: Oliver Hartkopp [EMAIL PROTECTED]
+ * Urs Thuermann [EMAIL PROTECTED]
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Send feedback to [EMAIL PROTECTED]
+ *
+ */
+
+#ifndef CAN_H
+#define CAN_H
+
+#include linux/types.h
+#include linux/socket.h
+
+/* controller area network (CAN) kernel definitions */
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
ones.
+++ net-2.6.24/include/linux/can/core.h 2007-09-17 11:08:39.0
+0200
@@ -0,0 +1,78 @@
+
+extern int can_proto_register(struct can_proto *cp);
+extern int can_proto_unregister(struct can_proto *cp);
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
(and maybe a WARN_ON for the not-found case).
+extern int can_rx_register(struct net_device *dev, canid_t can_id,
+canid_t mask,
+void (*func)(struct sk_buff *, void *),
+void *data, char *ident);
+
+extern int can_rx_unregister(struct net_device *dev, canid_t can_id,
+ canid_t mask,
+ void (*func)(struct sk_buff *, void *),
+ void *data);
Same here, none of the callers check the return value and since
they're all declared as void they can't propagate any errors back.
+++ net-2.6.24/net/can/af_can.c 2007-09-17 11:06:52.0 +0200
@@ -0,0 +1,1002 @@
+#include linux/module.h
+#include linux/init.h
+#include linux/kmod.h
+#include linux/slab.h
+#include linux/list.h
+#include linux/spinlock.h
+#include linux/rcupdate.h
+#include linux/uaccess.h
+#include linux/net.h
+#include linux/netdevice.h
+#include linux/socket.h
+#include linux/if_ether.h
+#include linux/if_arp.h
+#include linux/skbuff.h
+#include linux/can.h
+#include linux/can/core.h
+#include net/net_namespace.h
+#include net/sock.h
+
+#include af_can.h
It seems most of the things declared in that file are only used within
af_can.c. Might be easier to read the code if you'd just move it over.
+
+#define IDENT core
+static __initdata const char banner[] = KERN_INFO
+ can: controller area network core ( CAN_VERSION_STRING )\n;
+
+MODULE_DESCRIPTION(Controller Area Network PF_CAN core);
+MODULE_LICENSE(Dual BSD/GPL);
+MODULE_AUTHOR(Urs Thuermann [EMAIL PROTECTED],
+ Oliver Hartkopp [EMAIL PROTECTED]);
+
+MODULE_ALIAS_NETPROTO(PF_CAN);
+
+int stats_timer = 1; /* default: on */
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
I'll skip these since you probably know better than me.
+module_param(stats_timer, int, S_IRUGO);
+MODULE_PARM_DESC(stats_timer, enable timer for statistics (default:on));
+
+#ifdef CONFIG_CAN_DEBUG_CORE
+static int debug;
+module_param(debug, int, S_IRUGO);
+MODULE_PARM_DESC(debug, debug print mask: 1:debug, 2:frames, 4:skbs);
+#endif
+
+HLIST_HEAD(rx_dev_list);
Same here (static).
+static struct dev_rcv_lists rx_alldev_list;
+static DEFINE_SPINLOCK(rcv_lists_lock);
+
+static struct kmem_cache *rcv_cache __read_mostly;
+
+/* table of registered CAN protocols */
+static struct can_proto *proto_tab[CAN_NPROTO];
+
+struct timer_list stattimer; /* timer for statistics update */
+struct s_stats stats; /* packet statistics */
+struct s_pstats pstats; /* receive list statistics */
+
+/*
+ * af_can socket functions
+ */
+
+static int can_ioctl(struct socket *sock, unsigned int cmd, unsigned long
arg)
+{
+ struct sock *sk = sock-sk;
+
+ switch (cmd) {
+
+ case SIOCGSTAMP:
+ return sock_get_timestamp(sk, (struct timeval __user *)arg);
+
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
+
+static void can_sock_destruct(struct sock *sk)
+{
+ DBG(called for sock %p\n, sk);
+
+ skb_queue_purge(sk-sk_receive_queue);
+ if (sk-sk_protinfo)
+
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Urs Thuermann wrote: Patrick McHardy [EMAIL PROTECTED] writes: Looks pretty good, please see below for a few comments (mostly minor nitpicking, a few things that look like real bugs). Nothing that couldn't be fixed after merging though. 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. Agreed. No objections to putting this in 2.6.24 from my side .. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Patrick McHardy [EMAIL PROTECTED] writes: Looks pretty good, please see below for a few comments (mostly minor nitpicking, a few things that look like real bugs). Nothing that couldn't be fixed after merging though. 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] CAN: Add PF_CAN core module
Patrick McHardy [EMAIL PROTECTED] writes:
+++ net-2.6.24/include/linux/can.h 2007-09-17 10:27:09.0 +0200
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
ones.
No, this file contains the interface to user space.
+++ net-2.6.24/include/linux/can/core.h 2007-09-17 11:08:39.0
+0200
@@ -0,0 +1,78 @@
+
+extern int can_proto_register(struct can_proto *cp);
+extern int can_proto_unregister(struct can_proto *cp);
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
(and maybe a WARN_ON for the not-found case).
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
there.
Same here, none of the callers check the return value and since
they're all declared as void they can't propagate any errors back.
Same as above.
+#include af_can.h
It seems most of the things declared in that file are only used within
af_can.c. Might be easier to read the code if you'd just move it over.
af_can.h declares the interface between af_can.c and proc.c. There
should be nothing in af_can.h which is only used in af_can.c
+int stats_timer = 1; /* default: on */
This seems to be only used in af_can.c, so it could be static.
__read_mostly also seems to be approriate.
Right. Will be changed.
There are a few more that look like they could be __read_mostly
below, but I'll skip these since you probably know better than me.
Yes, I will check these also.
+HLIST_HEAD(rx_dev_list);
Same here (static).
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
ELF section? Where is that finally located?
+ if (ret == -ENOSYS)
+ printk(KERN_INFO can: request_module(%s) not
+ implemented.\n, module_name);
+ else if (ret)
+ printk(KERN_ERR can: request_module(%s) failed\n,
+ module_name);
Both of these printks seem to be user-triggerable, so they should
be rate-limited (or maybe get removed completely/changed to DBG).
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
per proto number. I will think about this.
+ /* check for success and correct type */
+ cp = proto_tab[protocol];
What prevents the module from getting unloaded again (and using
a stale pointer)?
When the module is unloaded it calls can_proto_unregister() which
clears the pointer. Do you see a race condition here?
+ if (!cp || cp-type != sock-type)
+ return -EPROTONOSUPPORT;
+
+ if (net != init_net)
+ return -EAFNOSUPPORT;
Shouldn't this be done before attempting the module load?
Yes, you're right.
+int can_send(struct sk_buff *skb, int loop)
+{
+ int err;
+
+ if (skb-dev-type != ARPHRD_CAN) {
+ kfree_skb(skb);
+ return -EPERM;
EPERM doesn't seem like the best fit, but I don't have a better
suggestion myself at the moment.
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
grant access to non-root users.
+ if (!(skb-dev-flags IFF_LOOPBACK)) {
+ /*
+* If the interface is not capable to do loopback
+* itself, we do it here.
+*/
+ struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
+
+ if (!newskb) {
+ kfree_skb(skb);
+ return -ENOMEM;
+ }
+
+ newskb-sk = skb-sk;
+ newskb-ip_summed = CHECKSUM_UNNECESSARY;
+ newskb-pkt_type = PACKET_BROADCAST;
+ netif_rx(newskb);
So the intention here is to send the packet to the non-loopback device
and manually loop it, which means sending it twice?
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
Re: [patch 2/7] CAN: Add PF_CAN core module
Hi Urs, Hello Paul, i assume Paul refers to the can_rx_delete_all() function that adds each receive list entry for rcu removal using the can_rx_delete RCU callback, right? So the idea would be to create a second RCU callback - e.g. can_rx_delete_list() - that removes the complete list inside the RCU callback?!? The list removal would therefore be processed inside this new can_rx_delete_list() in RCU context and not inside can_rx_delete_all(). @Paul: Was this your intention? Best regards, Oliver Paul E. McKenney wrote: On Wed, May 16, 2007 at 04:51:02PM +0200, Urs Thuermann wrote: 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. Interesting! One question called out below -- why do call_rcu() on each piece of the struct dev_rcv_lists, instead of doing call_rcu() on the whole thing and having the RCU callback free up the pieces? Given that all the pieces are call_rcu()ed separately, there had better not be persistent pointers to the pieces, right? Doing it in one chunk would make the code a bit simpler and also reduce the RCU overhead a bit. Or am I missing something subtle here? Thanx, Paul - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/7] CAN: Add PF_CAN core module
On Fri, May 18, 2007 at 11:19:01AM +0200, Oliver Hartkopp wrote:
Hi Urs, Hello Paul,
i assume Paul refers to the can_rx_delete_all() function that adds each
receive list entry for rcu removal using the can_rx_delete RCU callback,
right?
So the idea would be to create a second RCU callback - e.g.
can_rx_delete_list() - that removes the complete list inside the RCU
callback?!?
The list removal would therefore be processed inside this new
can_rx_delete_list() in RCU context and not inside can_rx_delete_all().
@Paul: Was this your intention?
My intention was that the list-removing be placed into can_rcv_lists_delete(),
perhaps as follows:
static void can_rx_delete_all(struct hlist_head *rl)
{
struct receiver *r;
struct hlist_node *n;
hlist_for_each_entry(r, n, rl, list) {
hlist_del(r-list);
kmem_cache_free(rcv_cache, r);
}
}
static void can_rcv_lists_delete(struct rcu_head *rp)
{
struct dev_rcv_lists *d = container_of(rp, struct dev_rcv_lists, rcu);
/* remove all receivers hooked at this netdevice */
can_rx_delete_all(d-rx_err);
can_rx_delete_all(d-rx_all);
can_rx_delete_all(d-rx_fil);
can_rx_delete_all(d-rx_inv);
can_rx_delete_all(d-rx_eff);
for (i = 0; i 2048; i++)
can_rx_delete_all(d-rx_sff[i]);
kfree(d);
}
Then the code in can_notifier() can reduce to the following:
if (d) {
hlist_del_rcu(d-list);
/* used to be a string of can_rx_delete_all(). */
} else
printk(KERN_ERR can: notifier: receive list not
found for dev %s\n, dev-name);
spin_lock_bh(rcv_lists_lock);
if (d) {
call_rcu(d-rcu, can_rcv_lists_delete);
}
This moves the traversal work into the callback function. This is not
a problem for CONFIG_PREEMPT_RT and non-CONFIG_PREEMPT, but not sure
about CONFIG_PREEMPT.
But it sure has the potential to cut down on a bunch of call_rcu()
work...
Thanx, Paul
Best regards,
Oliver
Paul E. McKenney wrote:
On Wed, May 16, 2007 at 04:51:02PM +0200, Urs Thuermann wrote:
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.
Interesting! One question called out below -- why do call_rcu() on each
piece of the struct dev_rcv_lists, instead of doing call_rcu() on the
whole thing and having the RCU callback free up the pieces? Given that
all the pieces are call_rcu()ed separately, there had better not be
persistent pointers to the pieces, right?
Doing it in one chunk would make the code a bit simpler and also reduce
the RCU overhead a bit.
Or am I missing something subtle here?
Thanx, Paul
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/7] CAN: Add PF_CAN core module
Hm, this is indeed one step further, than i thought :-)
Thanks for this nifty solution!
I will doublecheck your suggestion with Urs and then we'll change it in
our next patch update (after some more feedback on this mailing list).
Additional feedback is welcome.
Tnx best regards,
Oliver
Paul E. McKenney wrote:
On Fri, May 18, 2007 at 11:19:01AM +0200, Oliver Hartkopp wrote:
Hi Urs, Hello Paul,
i assume Paul refers to the can_rx_delete_all() function that adds each
receive list entry for rcu removal using the can_rx_delete RCU callback,
right?
So the idea would be to create a second RCU callback - e.g.
can_rx_delete_list() - that removes the complete list inside the RCU
callback?!?
The list removal would therefore be processed inside this new
can_rx_delete_list() in RCU context and not inside can_rx_delete_all().
@Paul: Was this your intention?
My intention was that the list-removing be placed into can_rcv_lists_delete(),
perhaps as follows:
static void can_rx_delete_all(struct hlist_head *rl)
{
struct receiver *r;
struct hlist_node *n;
hlist_for_each_entry(r, n, rl, list) {
hlist_del(r-list);
kmem_cache_free(rcv_cache, r);
}
}
static void can_rcv_lists_delete(struct rcu_head *rp)
{
struct dev_rcv_lists *d = container_of(rp, struct dev_rcv_lists, rcu);
/* remove all receivers hooked at this netdevice */
can_rx_delete_all(d-rx_err);
can_rx_delete_all(d-rx_all);
can_rx_delete_all(d-rx_fil);
can_rx_delete_all(d-rx_inv);
can_rx_delete_all(d-rx_eff);
for (i = 0; i 2048; i++)
can_rx_delete_all(d-rx_sff[i]);
kfree(d);
}
Then the code in can_notifier() can reduce to the following:
if (d) {
hlist_del_rcu(d-list);
/* used to be a string of can_rx_delete_all(). */
} else
printk(KERN_ERR can: notifier: receive list not
found for dev %s\n, dev-name);
spin_lock_bh(rcv_lists_lock);
if (d) {
call_rcu(d-rcu, can_rcv_lists_delete);
}
This moves the traversal work into the callback function. This is not
a problem for CONFIG_PREEMPT_RT and non-CONFIG_PREEMPT, but not sure
about CONFIG_PREEMPT.
But it sure has the potential to cut down on a bunch of call_rcu()
work...
Thanx, Paul
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/7] CAN: Add PF_CAN core module
Hello Paul,
as you may see in the attached SVN-log i changed some code according
your suggestions.
I additionally made some clarifications of function names.
If you would like to see the af_can.c completely please visit:
http://svn.berlios.de/svnroot/repos/socketcan/trunk/kernel/2.6/net/can/af_can.c
But this code in the projekt SVN is working for several kernel versions
(which is removed by some scripts when creating a LKML netdev patch) -
just for your information.
Best regards,
Oliver
Original-Nachricht
Betreff:r311 - trunk/kernel/2.6/net/can
Datum: Fri, 18 May 2007 21:47:15 +0200
Von:[EMAIL PROTECTED]
An: [EMAIL PROTECTED]
Author: hartkopp
Date: 2007-05-18 21:47:14 +0200 (Fri, 18 May 2007)
New Revision: 311
Modified:
trunk/kernel/2.6/net/can/af_can.c
Log:
Updated RCU removal of dev_rcv_lists structures in the case of CAN-interfaces
going down (in can_notifier). Thanks to Paul E. McKenny (IBM) who gave this
hint on netdev kernel mailinglist.
Modified: trunk/kernel/2.6/net/can/af_can.c
===
--- trunk/kernel/2.6/net/can/af_can.c 2007-05-16 09:03:21 UTC (rev 310)
+++ trunk/kernel/2.6/net/can/af_can.c 2007-05-18 19:47:14 UTC (rev 311)
@@ -454,31 +454,48 @@
}
EXPORT_SYMBOL(can_rx_register);
-static void can_rcv_lists_delete(struct rcu_head *rp)
+static void can_rx_delete_list(struct hlist_head *rl)
{
+ struct receiver *r;
+ struct hlist_node *n;
+
+ hlist_for_each_entry_rcu(r, n, rl, list) {
+ hlist_del_rcu(r-list);
+ kmem_cache_free(rcv_cache, r);
+ }
+}
+
+/*
+ * can_rx_delete_device - rcu callback for dev_rcv_lists structure removal
+ */
+static void can_rx_delete_device(struct rcu_head *rp)
+{
struct dev_rcv_lists *d = container_of(rp, struct dev_rcv_lists, rcu);
+ int i;
+ /* remove all receivers hooked at this netdevice */
+ can_rx_delete_list(d-rx_err);
+ can_rx_delete_list(d-rx_all);
+ can_rx_delete_list(d-rx_fil);
+ can_rx_delete_list(d-rx_inv);
+ can_rx_delete_list(d-rx_eff);
+
+ for (i = 0; i 2048; i++)
+ can_rx_delete_list(d-rx_sff[i]);
+
kfree(d);
}
-static void can_rx_delete(struct rcu_head *rp)
+/*
+ * can_rx_delete_receiver - rcu callback for single receiver entry removal
+ */
+static void can_rx_delete_receiver(struct rcu_head *rp)
{
struct receiver *r = container_of(rp, struct receiver, rcu);
kmem_cache_free(rcv_cache, r);
}
-static void can_rx_delete_all(struct hlist_head *rl)
-{
- struct receiver *r;
- struct hlist_node *n;
-
- hlist_for_each_entry_rcu(r, n, rl, list) {
- hlist_del_rcu(r-list);
- call_rcu(r-rcu, can_rx_delete);
- }
-}
-
/**
* can_rx_unregister - unsubscribe CAN frames from a specific interface
* @dev: pointer to netdevice (NULL = unsubcribe from 'all' CAN devices list)
@@ -556,7 +573,7 @@
/* schedule the receiver item for deletion */
if (r)
- call_rcu(r-rcu, can_rx_delete);
+ call_rcu(r-rcu, can_rx_delete_receiver);
return ret;
}
@@ -945,7 +962,6 @@
struct net_device *dev = (struct net_device *)data;
struct notifier *n;
struct dev_rcv_lists *d;
- int i;
DBG(called for %s, msg = %lu\n, dev-name, msg);
@@ -986,25 +1002,16 @@
spin_lock_bh(rcv_lists_lock);
d = find_dev_rcv_lists(dev);
- if (d) {
+ if (d)
hlist_del_rcu(d-list);
-
- /* remove all receivers hooked at this netdevice */
- can_rx_delete_all(d-rx_err);
- can_rx_delete_all(d-rx_all);
- can_rx_delete_all(d-rx_fil);
- can_rx_delete_all(d-rx_inv);
- can_rx_delete_all(d-rx_eff);
- for (i = 0; i 2048; i++)
- can_rx_delete_all(d-rx_sff[i]);
- } else
+ else
printk(KERN_ERR can: notifier: receive list not
found for dev %s\n, dev-name);
spin_unlock_bh(rcv_lists_lock);
if (d)
- call_rcu(d-rcu, can_rcv_lists_delete);
+ call_rcu(d-rcu, can_rx_delete_device);
break;
}
___
Socketcan-commit mailing list
[EMAIL PROTECTED]
https://lists.berlios.de/mailman/listinfo/socketcan-commit
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/7] CAN: Add PF_CAN core module
On 5/16/07, Urs Thuermann [EMAIL PROTECTED] wrote:
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 [EMAIL PROTECTED]
Signed-Off-By: Urs Thuermann [EMAIL PROTECTED]
SNIP
+
+/**
+ * struct sockaddr_can - the sockaddr structure for CAN sockets
+ * @can_family: address family number AF_CAN.
+ * @can_ifindex: CAN network interface index.
+ * @can_addr:transport protocol specific address, mostly CAN IDs.
+ */
+struct sockaddr_can {
+ sa_family_t can_family;
+ int can_ifindex;
+ union {
+ struct { canid_t rx_id, tx_id; } tp16;
+ struct { canid_t rx_id, tx_id; } tp20;
+ struct { canid_t rx_id, tx_id; } mcnet;
+ struct { canid_t rx_id, tx_id; } isotp;
+ struct { int lcu, type; } bap;
+ } can_addr;
+};
Can can_ifindex be turned into a unsigned short? That way we would
have it nicely packed, avoiding this hole:
[EMAIL PROTECTED] examples]$ pahole can
/* 1c2 /home/acme/git/pahole/examples/can.c:5 */
struct sockaddr_can {
sa_family_tcan_family;/* 0 2 */
/* XXX 2 bytes hole, try to pack */
intcan_ifindex; /* 4 4 */
union {
struct {
canid_trx_id; /* 8 4 */
canid_ttx_id; /* 12 4 */
} tp16; /* 8 */
struct {
canid_trx_id; /* 8 4 */
canid_ttx_id; /* 12 4 */
} tp20; /* 8 */
struct {
canid_trx_id; /* 8 4 */
canid_ttx_id; /* 12 4 */
} mcnet; /* 8 */
struct {
canid_trx_id; /* 8 4 */
canid_ttx_id; /* 12 4 */
} isotp; /* 8 */
struct {
intlcu; /* 8 4 */
inttype; /* 12 4 */
} bap;/* 8 */
} can_addr; /* 8 8 */
}; /* size: 16, cachelines: 1 */
/* sum members: 14, holes: 1, sum holes: 2 */
/* last cacheline: 16 bytes */
/* definitions: 1 */
- Arnaldo
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [patch 2/7] CAN: Add PF_CAN core module
Arnaldo Carvalho de Melo wrote:
SNIP
+
+/**
+ * struct sockaddr_can - the sockaddr structure for CAN sockets
+ * @can_family: address family number AF_CAN.
+ * @can_ifindex: CAN network interface index.
+ * @can_addr:transport protocol specific address, mostly CAN IDs.
+ */
+struct sockaddr_can {
+ sa_family_t can_family;
+ int can_ifindex;
+ union {
+ struct { canid_t rx_id, tx_id; } tp16;
+ struct { canid_t rx_id, tx_id; } tp20;
+ struct { canid_t rx_id, tx_id; } mcnet;
+ struct { canid_t rx_id, tx_id; } isotp;
+ struct { int lcu, type; } bap;
+ } can_addr;
+};
Can can_ifindex be turned into a unsigned short?
Hm - did you ever search for ifindex in the kernel?
E.g. in struct net_device in include/linux/netdevice.h , or functions
like dev_get_by_index() ?
The interface index (ifindex) is always(!) defined as an integer.
I think, we would get rightly knocked defining ifindex as a short value ;-)
Regards,
Oliver
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/7] CAN: Add PF_CAN core module
On 16 May 2007 21:14:20 +0200, Urs Thuermann [EMAIL PROTECTED] wrote: Arnaldo Carvalho de Melo [EMAIL PROTECTED] writes: Can can_ifindex be turned into a unsigned short? That way we would have it nicely packed, avoiding this hole: Since dev-ifindex is int and we have many assignments between can_ifindex and dev-ifindex it would not make sense to define can_ifindex as a short int. Also in user space, ifindex is int, e.g. in struct ifreq. This would cause implicit truncating type casts from int to short int (some compilers warn about this, haven't tried GCC for a long time) or require many explicit type casts. Quick answer, thank you. - Arnaldo - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
