Urs Thuermann wrote:
> This patch adds the virtual CAN bus (vcan) network driver.
> The vcan device is just a loopback device for CAN frames, no
> real CAN hardware is involved.
> 
> Signed-off-by: Oliver Hartkopp <[EMAIL PROTECTED]>
> Signed-off-by: Urs Thuermann <[EMAIL PROTECTED]>
> 

Also looks mostly fine, a few comments below.

> +++ net-2.6.24/drivers/net/can/vcan.c 2007-09-17 11:17:45.000000000 +0200
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/netdevice.h>
> +#include <linux/if_arp.h>
> +#include <linux/if_ether.h>
> +#include <linux/can.h>
> +#include <net/rtnetlink.h>
> +
> +static __initdata const char banner[] =
> +     KERN_INFO "vcan: Virtual CAN interface driver\n";
> +
> +MODULE_DESCRIPTION("virtual CAN interface");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_AUTHOR("Urs Thuermann <[EMAIL PROTECTED]>");
> +
> +#ifdef CONFIG_CAN_DEBUG_DEVICES
> +static int debug;
> +module_param(debug, int, S_IRUGO);
> +#endif
> +
> +/* To be moved to linux/can/dev.h */
> +#ifdef CONFIG_CAN_DEBUG_DEVICES
> +#define DBG(args...)       (debug & 1 ? \
> +                            (printk(KERN_DEBUG "vcan %s: ", __func__), \
> +                             printk(args)) : 0)
> +#else
> +#define DBG(args...)
> +#endif
> +
> +
> +/*
> + * CAN test feature:
> + * Enable the loopback on driver level for testing the CAN core loopback 
> modes.
> + * See Documentation/networking/can.txt for details.
> + */
> +
> +static int loopback; /* loopback testing. Default: 0 (Off) */
> +module_param(loopback, int, S_IRUGO);
> +MODULE_PARM_DESC(loopback, "Loop back frames (for testing). Default: 0 
> (Off)");


I would still prefer to have this on a per-device level configured
through netlink, but since we currently don't support specifying
flags for new devices anyways, I won't argue about it anymore
(OTOH, if you'd agree I could send a patch to add this feature
to the rtnl_link API).

> +
> +struct vcan_priv {
> +     struct net_device *dev;
> +     struct list_head list;
> +};


This is not needed anymore. The rtnl_link_unregister function calls
the ->dellink function for each device of this type. Check out the
current dummy.c driver.

> +static LIST_HEAD(vcan_devs);
> +
> +static int vcan_open(struct net_device *dev)
> +{
> +     DBG("%s: interface up\n", dev->name);
> +
> +     netif_start_queue(dev);
> +     return 0;
> +}
> +
> +static int vcan_stop(struct net_device *dev)
> +{
> +     DBG("%s: interface down\n", dev->name);
> +
> +     netif_stop_queue(dev);
> +     return 0;
> +}
> +
> +static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
> +{
> +     struct net_device_stats *stats = &dev->stats;
> +
> +     stats->rx_packets++;
> +     stats->rx_bytes += skb->len;
> +
> +     skb->protocol  = htons(ETH_P_CAN);
> +     skb->pkt_type  = PACKET_BROADCAST;
> +     skb->dev       = dev;
> +     skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> +     DBG("received skbuff on interface %d\n", dev->ifindex);
> +
> +     netif_rx(skb);
> +}
> +
> +static int vcan_tx(struct sk_buff *skb, struct net_device *dev)
> +{
> +     struct net_device_stats *stats = &dev->stats;
> +     int loop;
> +
> +     DBG("sending skbuff on interface %s\n", dev->name);
> +
> +     stats->tx_packets++;
> +     stats->tx_bytes += skb->len;
> +
> +     /* set flag whether this packet has to be looped back */
> +     loop = skb->pkt_type == PACKET_LOOPBACK;
> +
> +     if (!loopback) {
> +             /* no loopback handling available inside this driver */
> +
> +             if (loop) {
> +                     /*
> +                      * only count the packets here, because the
> +                      * CAN core already did the loopback for us
> +                      */
> +                     stats->rx_packets++;
> +                     stats->rx_bytes += skb->len;
> +             }
> +             kfree_skb(skb);
> +             return 0;
> +     }
> +
> +     /* perform standard loopback handling for CAN network interfaces */
> +
> +     if (loop) {
> +             struct sock *srcsk = skb->sk;
> +
> +             if (atomic_read(&skb->users) != 1) {
> +                     struct sk_buff *old_skb = skb;
> +
> +                     skb = skb_clone(old_skb, GFP_ATOMIC);
> +                     DBG(KERN_INFO "%s: %s: freeing old skbuff %p, "
> +                         "using new skbuff %p\n",
> +                         dev->name, __FUNCTION__, old_skb, skb);
> +                     kfree_skb(old_skb);

skb_share_check()?

> +                     if (!skb)
> +                             return 0;
> +             } else
> +                     skb_orphan(skb);
> +
> +             /* receive with packet counting */
> +             skb->sk = srcsk;


Where is the socket used and what makes sure it still exists?

> +             vcan_rx(skb, dev);
> +     } else {
> +             /* no looped packets => no counting */
> +             kfree_skb(skb);
> +     }
> +     return 0;
> +}
> +
> +static void vcan_setup(struct net_device *dev)
> +{
> +     DBG("dev %s\n", dev->name);
> +
> +     dev->type              = ARPHRD_CAN;
> +     dev->mtu               = sizeof(struct can_frame);
> +     dev->hard_header_len   = 0;
> +     dev->addr_len          = 0;
> +     dev->tx_queue_len      = 0;
> +     dev->flags             = IFF_NOARP;
> +
> +     /* set flags according to driver capabilities */
> +     if (loopback)
> +             dev->flags |= IFF_LOOPBACK;
> +
> +     dev->open              = vcan_open;
> +     dev->stop              = vcan_stop;
> +     dev->hard_start_xmit   = vcan_tx;
> +     dev->destructor        = free_netdev;
> +
> +}
> +
> +static int vcan_newlink(struct net_device *dev,
> +                     struct nlattr *tb[], struct nlattr *data[])
> +{
> +     struct vcan_priv *priv = netdev_priv(dev);
> +     int err;
> +
> +     err = register_netdevice(dev);
> +     if (err < 0)
> +             return err;
> +
> +     priv->dev = dev;
> +     list_add_tail(&priv->list, &vcan_devs);
> +     return 0;
> +}
> +
> +static void vcan_dellink(struct net_device *dev)
> +{
> +     struct vcan_priv *priv = netdev_priv(dev);
> +
> +     list_del(&priv->list);
> +     unregister_netdevice(dev);
> +}


Both the addlink and dellink function can be removed, the default
for addlink is register_netdevice, the default for unregister
is unregister_netdevice (and the list is not needed anymore as
mentioned above).

> +
> +static struct rtnl_link_ops vcan_link_ops __read_mostly = {
> +       .kind           = "vcan",
> +       .priv_size      = sizeof(struct vcan_priv),
> +       .setup          = vcan_setup,
> +       .newlink        = vcan_newlink,
> +       .dellink        = vcan_dellink,
> +};
> +
> +static __init int vcan_init_module(void)
> +{
> +     int err;
> +
> +     printk(banner);
> +
> +     if (loopback)
> +             printk(KERN_INFO "vcan: enabled loopback on driver level.\n");
> +
> +     rtnl_lock();
> +     err = __rtnl_link_register(&vcan_link_ops);
> +     rtnl_unlock();


Just using rtnl_link_register here is fine.

> +     return err;
> +}
> +
> +static __exit void vcan_cleanup_module(void)
> +{
> +     struct vcan_priv *priv, *n;
> +
> +     rtnl_lock();
> +     list_for_each_entry_safe(priv, n, &vcan_devs, list)
> +             vcan_dellink(priv->dev);
> +     __rtnl_link_unregister(&vcan_link_ops);
> +     rtnl_unlock();


and rtnl_link_unregister (without manual cleanup) 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

Reply via email to