Re: [PATCH 2/5] can: slcan: Convert to can_dev interface

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 10981 bytes --]

On 04/15/2014 04:51 PM, Alexander Stein wrote:
> No actual feature change despite the interface name. This is just a
> preparation for additional CAN related features.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/net/can/Kconfig |  40 +++++++++----------
>  drivers/net/can/slcan.c | 104 +++++++++++++++++++++---------------------------
>  2 files changed, 65 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 9e7d95d..daea043 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -9,26 +9,6 @@ config CAN_VCAN
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called vcan.
>  
> -config CAN_SLCAN
> -	tristate "Serial / USB serial CAN Adaptors (slcan)"
> -	depends on TTY
> -	---help---
> -	  CAN driver for several 'low cost' CAN interfaces that are attached
> -	  via serial lines or via USB-to-serial adapters using the LAWICEL
> -	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
> -
> -	  As only the sending and receiving of CAN frames is implemented, this
> -	  driver should work with the (serial/USB) CAN hardware from:
> -	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
> -
> -	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
> -	  slcand) can be found in the can-utils at the SocketCAN SVN, see
> -	  http://developer.berlios.de/projects/socketcan for details.
> -
> -	  The slcan driver supports up to 10 CAN netdevices by default which
> -	  can be changed by the 'maxdev=xx' module option. This driver can
> -	  also be built as a module. If so, the module will be called slcan.
> -
>  config CAN_DEV
>  	tristate "Platform CAN drivers with Netlink support"
>  	default y
> @@ -125,6 +105,26 @@ config CAN_GRCAN
>  	  endian syntheses of the cores would need some modifications on
>  	  the hardware level to work.
>  
> +config CAN_SLCAN
> +	tristate "Serial / USB serial CAN Adaptors (slcan)"
> +	depends on TTY
> +	---help---
> +	  CAN driver for several 'low cost' CAN interfaces that are attached
> +	  via serial lines or via USB-to-serial adapters using the LAWICEL
> +	  ASCII protocol. The driver implements the tty linediscipline N_SLCAN.
> +
> +	  As only the sending and receiving of CAN frames is implemented, this
> +	  driver should work with the (serial/USB) CAN hardware from:
> +	  www.canusb.com / www.can232.com / www.mictronics.de / www.canhack.de
> +
> +	  Userspace tools to attach the SLCAN line discipline (slcan_attach,
> +	  slcand) can be found in the can-utils at the SocketCAN SVN, see
> +	  http://developer.berlios.de/projects/socketcan for details.
> +
> +	  The slcan driver supports up to 10 CAN netdevices by default which
> +	  can be changed by the 'maxdev=xx' module option. This driver can
> +	  also be built as a module. If so, the module will be called slcan.
> +
>  source "drivers/net/can/mscan/Kconfig"
>  
>  source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 2577c1e..337e6e3 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -53,6 +53,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/can.h>
> +#include <linux/can/dev.h>
>  #include <linux/can/skb.h>
>  
>  static __initconst const char banner[] =
> @@ -79,6 +80,7 @@ MODULE_PARM_DESC(maxdev, "Maximum number of slcan interfaces");
>  #define SLC_EFF_ID_LEN 8
>  
>  struct slcan {
> +	struct can_priv		can;	/* must be the first member */
>  	int			magic;
>  
>  	/* Various fields. */
> @@ -99,6 +101,7 @@ struct slcan {
>  };
>  
>  static struct net_device **slcan_devs;
> +static DEFINE_MUTEX(slcan_mutex);
>  
>   /************************************************************************
>    *			SLCAN ENCAPSULATION FORMAT			 *
> @@ -143,31 +146,33 @@ static struct net_device **slcan_devs;
>  static void slc_bump(struct slcan *sl)
>  {
>  	struct sk_buff *skb;
> -	struct can_frame cf;
> +	struct can_frame *cf;
> +	canid_t can_id;
> +	u8 can_dlc;
>  	int i, tmp;
>  	u32 tmpid;
>  	char *cmd = sl->rbuff;
>  
> -	cf.can_id = 0;
> +	can_id = 0;
>  
>  	switch (*cmd) {
>  	case 'r':
> -		cf.can_id = CAN_RTR_FLAG;
> +		can_id = CAN_RTR_FLAG;
>  		/* fallthrough */
>  	case 't':
>  		/* store dlc ASCII value and terminate SFF CAN ID string */
> -		cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
> +		can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN];
>  		sl->rbuff[SLC_CMD_LEN + SLC_SFF_ID_LEN] = 0;
>  		/* point to payload data behind the dlc */
>  		cmd += SLC_CMD_LEN + SLC_SFF_ID_LEN + 1;
>  		break;
>  	case 'R':
> -		cf.can_id = CAN_RTR_FLAG;
> +		can_id = CAN_RTR_FLAG;
>  		/* fallthrough */
>  	case 'T':
> -		cf.can_id |= CAN_EFF_FLAG;
> +		can_id |= CAN_EFF_FLAG;
>  		/* store dlc ASCII value and terminate EFF CAN ID string */
> -		cf.can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
> +		can_dlc = sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN];
>  		sl->rbuff[SLC_CMD_LEN + SLC_EFF_ID_LEN] = 0;
>  		/* point to payload data behind the dlc */
>  		cmd += SLC_CMD_LEN + SLC_EFF_ID_LEN + 1;
> @@ -179,49 +184,39 @@ static void slc_bump(struct slcan *sl)
>  	if (kstrtou32(sl->rbuff + SLC_CMD_LEN, 16, &tmpid))
>  		return;
>  
> -	cf.can_id |= tmpid;
> +	can_id |= tmpid;
>  
>  	/* get can_dlc from sanitized ASCII value */
> -	if (cf.can_dlc >= '0' && cf.can_dlc < '9')
> -		cf.can_dlc -= '0';
> +	if (can_dlc >= '0' && can_dlc < '9')
> +		can_dlc -= '0';
>  	else
>  		return;
>  
> -	*(u64 *) (&cf.data) = 0; /* clear payload */
> +	skb = alloc_can_skb(sl->dev, &cf);
> +	if (!skb)
> +		return;
> +	cf->can_id = can_id;
> +	cf->can_dlc = can_dlc;
>  
>  	/* RTR frames may have a dlc > 0 but they never have any data bytes */
> -	if (!(cf.can_id & CAN_RTR_FLAG)) {
> -		for (i = 0; i < cf.can_dlc; i++) {
> +	if (!(cf->can_id & CAN_RTR_FLAG)) {
> +		for (i = 0; i < cf->can_dlc; i++) {
>  			tmp = hex_to_bin(*cmd++);
>  			if (tmp < 0)
>  				return;
> -			cf.data[i] = (tmp << 4);
> +			cf->data[i] = (tmp << 4);
>  			tmp = hex_to_bin(*cmd++);
>  			if (tmp < 0)
>  				return;
> -			cf.data[i] |= tmp;
> +			cf->data[i] |= tmp;
>  		}
>  	}
> -
> -	skb = dev_alloc_skb(sizeof(struct can_frame) +
> -			    sizeof(struct can_skb_priv));
> -	if (!skb)
> -		return;
> -
>  	skb->dev = sl->dev;

I think this is not needed anymore, as alloc_can_skb() sets this already.

> -	skb->protocol = htons(ETH_P_CAN);
> -	skb->pkt_type = PACKET_BROADCAST;
> -	skb->ip_summed = CHECKSUM_UNNECESSARY;
> -
> -	can_skb_reserve(skb);
> -	can_skb_prv(skb)->ifindex = sl->dev->ifindex;
>  
> -	memcpy(skb_put(skb, sizeof(struct can_frame)),
> -	       &cf, sizeof(struct can_frame));
>  	netif_rx_ni(skb);
>  
>  	sl->dev->stats.rx_packets++;
> -	sl->dev->stats.rx_bytes += cf.can_dlc;
> +	sl->dev->stats.rx_bytes += cf->can_dlc;

Please don't touch the skb (and this cf) after netif_rx_ni

>  }
>  
>  /* parse tty input stream */
> @@ -385,6 +380,10 @@ static int slc_close(struct net_device *dev)
>  	netif_stop_queue(dev);
>  	sl->rcount   = 0;
>  	sl->xleft    = 0;
> +	sl->can.state = CAN_STATE_STOPPED;
> +
> +	close_candev(dev);
> +
>  	spin_unlock_bh(&sl->lock);
>  
>  	return 0;
> @@ -394,12 +393,19 @@ static int slc_close(struct net_device *dev)
>  static int slc_open(struct net_device *dev)
>  {
>  	struct slcan *sl = netdev_priv(dev);
> +	int err;
>  
>  	if (sl->tty == NULL)
>  		return -ENODEV;
>  
> +	err = open_candev(dev);
> +	if (err)
> +		return err;
> +
> +	sl->can.state = CAN_STATE_ERROR_ACTIVE;
>  	sl->flags &= (1 << SLF_INUSE);
>  	netif_start_queue(dev);
> +
>  	return 0;
>  }
>  
> @@ -407,7 +413,7 @@ static int slc_open(struct net_device *dev)
>  static void slc_free_netdev(struct net_device *dev)
>  {
>  	int i = dev->base_addr;
> -	free_netdev(dev);
> +	free_candev(dev);
>  	slcan_devs[i] = NULL;
>  }
>  
> @@ -417,23 +423,6 @@ static const struct net_device_ops slc_netdev_ops = {
>  	.ndo_start_xmit         = slc_xmit,
>  };
>  
> -static void slc_setup(struct net_device *dev)
> -{
> -	dev->netdev_ops		= &slc_netdev_ops;
> -	dev->destructor		= slc_free_netdev;
> -
> -	dev->hard_header_len	= 0;
> -	dev->addr_len		= 0;
> -	dev->tx_queue_len	= 10;
> -
> -	dev->mtu		= sizeof(struct can_frame);
> -	dev->type		= ARPHRD_CAN;
> -
> -	/* New-style flags. */
> -	dev->flags		= IFF_NOARP;
> -	dev->features           = NETIF_F_HW_CSUM;
> -}
> -
>  /******************************************
>    Routines looking at TTY side.
>   ******************************************/
> @@ -495,7 +484,6 @@ static void slc_sync(void)
>  static struct slcan *slc_alloc(dev_t line)
>  {
>  	int i;
> -	char name[IFNAMSIZ];
>  	struct net_device *dev = NULL;
>  	struct slcan       *sl;
>  
> @@ -510,12 +498,13 @@ static struct slcan *slc_alloc(dev_t line)
>  	if (i >= maxdev)
>  		return NULL;
>  
> -	sprintf(name, "slcan%d", i);
> -	dev = alloc_netdev(sizeof(*sl), name, slc_setup);
> +	dev = alloc_candev(sizeof(*sl), 1);
>  	if (!dev)
>  		return NULL;
>  
>  	dev->base_addr  = i;
> +	dev->netdev_ops = &slc_netdev_ops;
> +	dev->destructor = slc_free_netdev;
>  	sl = netdev_priv(dev);
>  
>  	/* Initialize channel control data */
> @@ -548,11 +537,8 @@ static int slcan_open(struct tty_struct *tty)
>  	if (tty->ops->write == NULL)
>  		return -EOPNOTSUPP;
>  
> -	/* RTnetlink lock is misused here to serialize concurrent
> -	   opens of slcan channels. There are better ways, but it is
> -	   the simplest one.
> -	 */
> -	rtnl_lock();
> +	/* Serialize concurrent opens of slcan channels. */
> +	mutex_lock(&slcan_mutex);
>  
>  	/* Collect hanged up channels. */
>  	slc_sync();
> @@ -580,13 +566,13 @@ static int slcan_open(struct tty_struct *tty)
>  
>  		set_bit(SLF_INUSE, &sl->flags);
>  
> -		err = register_netdevice(sl->dev);
> +		err = register_candev(sl->dev);
>  		if (err)
>  			goto err_free_chan;
>  	}
>  
>  	/* Done.  We have linked the TTY line to a channel. */
> -	rtnl_unlock();
> +	mutex_unlock(&slcan_mutex);
>  	tty->receive_room = 65536;	/* We don't flow control */
>  
>  	/* TTY layer expects 0 on success */
> @@ -598,7 +584,7 @@ err_free_chan:
>  	clear_bit(SLF_INUSE, &sl->flags);
>  
>  err_exit:
> -	rtnl_unlock();
> +	mutex_unlock(&slcan_mutex);
>  
>  	/* Count references from TTY module */
>  	return err;
> 

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]