netdev - Re: [RFC PATCH v3] 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: <CAMZ6RqLu9hjTApnU3C-OWiZp_1yjSTWe9jBhgajmfEezM9_s+w@mail.gmail.com>
Date: Mon, 15 May 2023 10:57:38 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Harald Mommer <harald.mommer@...nsynergy.com>
Cc: Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com>, 
	virtio-dev@...ts.oasis-open.org, linux-can@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	virtualization@...ts.linux-foundation.org, 
	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>, 
	"Michael S . Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>, 
	Damir Shaikhutdinov <Damir.Shaikhutdinov@...nsynergy.com>
Subject: Re: [RFC PATCH v3] can: virtio: Initial virtio CAN driver.

On Sat. 13 May 2023 at 23:18, Harald Mommer
<harald.mommer@...nsynergy.com> wrote:
> Hello Vincent,
>
> sometimes it's close to weekend and already late. I've to correct myself.

Yeah... I understand that. There is no hurry to answer quickly (and
this is why you see my answer just today). Hope you had a restful
weekend.

> On 12.05.23 11:53, Vincent MAILHOL wrote:
> >
> >> +static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
> >> +                                        struct net_device *dev)
> >> +{
> >> +       struct virtio_can_priv *priv = netdev_priv(dev);
> >> +       struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> >> +       struct virtio_can_tx *can_tx_msg;
> >> +       struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
> >> +       struct scatterlist sg_out[1];
> >> +       struct scatterlist sg_in[1];
> >> +       struct scatterlist *sgs[2];
> This 2 here.
> >
> >> +       /* Normal queue stop when no transmission slots are left */
> >> +       if (atomic_read(&priv->tx_inflight) >= priv->can.echo_skb_max ||
> >> +           vq->num_free == 0 || (vq->num_free < 2 &&
> > Replace the Magic number 2 with a #define.
>
> Is this 2 here.
>
> Obviously with my previous answer I switched into panic mode thinking
> already about explaining indirect descriptors and all kind of virtio
> details and the expression in depth not realizing any more that
> something different was requested.
>
> Appropriate answer:
>
> /* CAN TX needs 2 descriptors: 1 device readable and 1 device writable */
> #define CAN_TX_DESC (1 + 1)
>
> Or something with ARRAY_SIZE(sgs) to get the number of elements in sgs
> keeping the first 2 above.

ARRAY_SIZE(sgs) looks good! It is better than a #define.

> And then I'll have to think again whether I really want to keep
> sgs_in[1] and sgs_out[1] as arrays. Not now, now is weekend.
>
> >> +           !virtio_has_feature(vq->vdev, VIRTIO_RING_F_INDIRECT_DESC))) {
> >> +               netif_stop_queue(dev);
> >> +               netdev_dbg(dev, "TX: Normal stop queue\n");
> >> +       }
> >> +
> >> +       spin_unlock_irqrestore(&priv->tx_lock, flags);
> >> +
> >> +kick:
> >> +       if (netif_queue_stopped(dev) || !netdev_xmit_more()) {
> >> +               if (!virtqueue_kick(vq))
> >> +                       netdev_err(dev, "%s(): Kick failed\n", __func__);
> >> +       }
> >> +
> >> +       return xmit_ret;
> >> +}
> >>