On 11/03/2010 07:41 PM, Oliver Hartkopp wrote: > Hi Urs, > > i tested your patch that uses the sockets inode number instead of the pointer > address to the sock structure. It works great and has indeed a new benefit > for the debugging. > > Additionally i added a counter for tx drops to the bcm that was missing in my > tests for ages: > > har...@vwagwolkf320:~/linux-2.6$ cat /proc/net/can-bcm/43572 >>>> socket inode 43572 / dropped 0 / dropped_tx 0 / bound vcan1 <<< > > The problem of the address exposure for the callback function and its > parameter could be fixed by adding a new kernel config option - as done by > different other kernel code: CONFIG_CAN_DEBUG_NETLAYER > > Unfortunately i mixed all this into one patch for this testing. > Anyway, what's your opinion about the suggested changes?
I have some nitpicking...see comments inline:
> Regards,
> Oliver
>
> ---
>
> diff --git a/net/can/Kconfig b/net/can/Kconfig
> index 89395b2..b57f3c4 100644
> --- a/net/can/Kconfig
> +++ b/net/can/Kconfig
> @@ -40,5 +40,14 @@ config CAN_BCM
> CAN messages are used on the bus (e.g. in automotive environments).
> To use the Broadcast Manager, use AF_CAN with protocol CAN_BCM.
>
> +config CAN_DEBUG_NETLAYER
> + bool "Enable CAN network layer debugging"
> + depends on CAN
> + default N
default no is the default, so the default statement can be removed
completely.
> + ---help---
> + Say Y here if you are developing and/or debugging protocols for the
> + CAN network layer. When enabled e.g. kernel internal addresses to
> + structures and function pointers are exposed in the procfs output
> + in /proc/net/can/rcvlist_* .
>
> source "drivers/net/can/Kconfig"
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 08ffe9e..768e79f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -124,8 +124,8 @@ struct bcm_sock {
> struct list_head rx_ops;
> struct list_head tx_ops;
> unsigned long dropped_usr_msgs;
> + unsigned long dropped_tx_msgs;
> struct proc_dir_entry *bcm_proc_read;
> - char procname [9]; /* pointer printed in ASCII with \0 */
> };
>
> static inline struct bcm_sock *bcm_sk(const struct sock *sk)
> @@ -165,10 +165,9 @@ static int bcm_proc_show(struct seq_file *m, void *v)
> struct bcm_sock *bo = bcm_sk(sk);
> struct bcm_op *op;
>
> - seq_printf(m, ">>> socket %p", sk->sk_socket);
> - seq_printf(m, " / sk %p", sk);
> - seq_printf(m, " / bo %p", bo);
> + seq_printf(m, ">>> socket inode %lu", sock_i_ino(sk));
> seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
> + seq_printf(m, " / dropped_tx %lu", bo->dropped_tx_msgs);
> seq_printf(m, " / bound %s", bcm_proc_getifname(ifname, bo->ifindex));
> seq_printf(m, " <<<\n");
>
> @@ -266,7 +265,12 @@ static void bcm_can_tx(struct bcm_op *op)
> /* send with loopback */
> skb->dev = dev;
> skb->sk = op->sk;
> - can_send(skb, 1);
> +
> + if (can_send(skb, 1)) {
That's unusual coding style. In the kernel often an intermediate
variable "ret" or "err" is used.
> + struct bcm_sock *bo = bcm_sk(op->sk);
> +
> + bo->dropped_tx_msgs++;
> + }
>
> /* update statistics */
> op->currframe++;
> @@ -1426,6 +1430,7 @@ static int bcm_release(struct socket *sock)
> struct sock *sk = sock->sk;
> struct bcm_sock *bo = bcm_sk(sk);
> struct bcm_op *op, *next;
> + char proc_fname[32];
>
> /* remove bcm_ops, timer, rx_unregister(), etc. */
>
> @@ -1465,8 +1470,10 @@ static int bcm_release(struct socket *sock)
> }
>
> /* remove procfs entry */
> - if (proc_dir && bo->bcm_proc_read)
> - remove_proc_entry(bo->procname, proc_dir);
> + if (proc_dir && bo->bcm_proc_read) {
> + snprintf(proc_fname, sizeof(proc_fname), "%lu", sock_i_ino(sk));
> + remove_proc_entry(proc_fname, proc_dir);
> + }
>
> /* remove device reference */
> if (bo->bound) {
> @@ -1489,6 +1496,7 @@ static int bcm_connect(struct socket *sock, struct
> sockaddr *uaddr, int len,
> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> struct sock *sk = sock->sk;
> struct bcm_sock *bo = bcm_sk(sk);
> + char proc_fname[32];
>
> if (len < sizeof(*addr))
> return -EINVAL;
> @@ -1521,8 +1529,8 @@ static int bcm_connect(struct socket *sock, struct
> sockaddr *uaddr, int len,
>
> if (proc_dir) {
> /* unique socket address as filename */
> - sprintf(bo->procname, "%p", sock);
^^^^^^^^^^^^^
Just wondering:
Is this a "struct bcm_sock", right? Is the procname member still used,
or can it be removed now?
> - bo->bcm_proc_read = proc_create_data(bo->procname, 0644,
> + snprintf(proc_fname, sizeof(proc_fname), "%lu", sock_i_ino(sk));
> + bo->bcm_proc_read = proc_create_data(proc_fname, 0644,
> proc_dir,
> &bcm_proc_fops, sk);
> }
> diff --git a/net/can/proc.c b/net/can/proc.c
> index f4265cc..6011547 100644
> --- a/net/can/proc.c
> +++ b/net/can/proc.c
> @@ -208,8 +208,12 @@ static void can_print_rcvlist(struct seq_file *m, struct
> hlist_head *rx_list,
> " %-5s %03X %08x %08lx %08lx %8ld %s\n";
>
> seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
> - (unsigned long)r->func, (unsigned long)r->data,
> - r->matches, r->ident);
> +#ifdef CONFIG_CAN_DEBUG_NETLAYER
> + (unsigned long)r->func, (unsigned long)r->data,
> +#else
> + 0, 0,
> +#endif
> + r->matches, r->ident);
> }
> }
cheers, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
