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

[Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>]

Le 08/01/2024 à 14:10, Mikhail Golubev-Ciuchea a écrit :
> From: Harald Mommer <harald.mommer@xxxxxxxxxxxxxxx>
>
> - 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@xxxxxxxxxxxxxxx>
> Co-developed-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@xxxxxxxxxxxxxxx>
> ---

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;
> +}

...