linux-kernel - Re: [PATCH v5] can: virtio: Initial virtio CAN driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a366f529-c901-4cd1-a1a6-c3958562cace@wanadoo.fr>
Date: Mon, 8 Jan 2024 20:34:17 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com>,
 Wolfgang Grandegger <wg@...ndegger.com>,
 Marc Kleine-Budde <mkl@...gutronix.de>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Harald Mommer <harald.mommer@...nsynergy.com>,
 "Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@...nsynergy.com>,
 linux-kernel@...r.kernel.org, linux-can@...r.kernel.org,
 netdev@...r.kernel.org, virtualization@...ts.linux.dev
Subject: Re: [PATCH v5] can: virtio: Initial virtio CAN driver.

Le 08/01/2024 à 14:10, Mikhail Golubev-Ciuchea a écrit :
> From: Harald Mommer <harald.mommer@...nsynergy.com>
> 
> - CAN Control
> 
>    - "ip link set up can0" starts the virtual CAN controller,
>    - "ip link set up can0" stops the virtual CAN controller
> 
> - CAN RX
> 
>    Receive CAN frames. CAN frames can be standard or extended, classic or
>    CAN FD. Classic CAN RTR frames are supported.
> 
> - CAN TX
> 
>    Send CAN frames. CAN frames can be standard or extended, classic or
>    CAN FD. Classic CAN RTR frames are supported.
> 
> - CAN BusOff indication
> 
>    CAN BusOff is handled by a bit in the configuration space.
> 
> Signed-off-by: Harald Mommer <Harald.Mommer@...nsynergy.com>
> Signed-off-by: Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com>
> Co-developed-by: Marc Kleine-Budde <mkl@...gutronix.de>
> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
> Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@...nsynergy.com>
> ---

Hi,
a few nits below, should there be a v6.


> +static int virtio_can_alloc_tx_idx(struct virtio_can_priv *priv)
> +{
> +	int tx_idx;
> +
> +	tx_idx = ida_alloc_range(&priv->tx_putidx_ida, 0,
> +				 priv->can.echo_skb_max - 1, GFP_KERNEL);
> +	if (tx_idx >= 0)
> +		atomic_add(1, &priv->tx_inflight);

atomic_inc() ?

> +
> +	return tx_idx;
> +}
> +
> +static void virtio_can_free_tx_idx(struct virtio_can_priv *priv,
> +				   unsigned int idx)
> +{
> +	ida_free(&priv->tx_putidx_ida, idx);
> +	atomic_sub(1, &priv->tx_inflight);

atomic_dec() ?

> +}

...

> +static int virtio_can_probe(struct virtio_device *vdev)
> +{
> +	struct virtio_can_priv *priv;
> +	struct net_device *dev;
> +	int err;
> +
> +	dev = alloc_candev(sizeof(struct virtio_can_priv),
> +			   VIRTIO_CAN_ECHO_SKB_MAX);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	priv = netdev_priv(dev);
> +
> +	ida_init(&priv->tx_putidx_ida);
> +
> +	netif_napi_add(dev, &priv->napi, virtio_can_rx_poll);
> +	netif_napi_add(dev, &priv->napi_tx, virtio_can_tx_poll);
> +
> +	SET_NETDEV_DEV(dev, &vdev->dev);
> +
> +	priv->dev = dev;
> +	priv->vdev = vdev;
> +	vdev->priv = priv;
> +
> +	priv->can.do_set_mode = virtio_can_set_mode;
> +	/* Set Virtio CAN supported operations */
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING;
> +	if (virtio_has_feature(vdev, VIRTIO_CAN_F_CAN_FD)) {
> +		err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD);
> +		if (err != 0)
> +			goto on_failure;
> +	}
> +
> +	/* Initialize virtqueues */
> +	err = virtio_can_find_vqs(priv);
> +	if (err != 0)
> +		goto on_failure;
> +
> +	INIT_LIST_HEAD(&priv->tx_list);
> +
> +	spin_lock_init(&priv->tx_lock);
> +	mutex_init(&priv->ctrl_lock);
> +
> +	init_completion(&priv->ctrl_done);
> +
> +	virtio_can_populate_vqs(vdev);
> +
> +	register_virtio_can_dev(dev);

Check for error?

CJ

> +
> +	napi_enable(&priv->napi);
> +	napi_enable(&priv->napi_tx);
> +
> +	/* Request device going live */
> +	virtio_device_ready(vdev); /* Optionally done by virtio_dev_probe() */
> +
> +	return 0;
> +
> +on_failure:
> +	virtio_can_free_candev(dev);
> +	return err;
> +}

...