Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230511151444.162882-1-Mikhail.Golubev-Ciuchea@opensynergy.com>
 <CAMZ6RqJbjoApwZbiivbvJRYQyBWfWXG4azmwuXGaicrMq0Lozg@mail.gmail.com> <3950ac4f-3292-e6ca-7484-77f3a5639305@opensynergy.com>
In-Reply-To: <3950ac4f-3292-e6ca-7484-77f3a5639305@opensynergy.com>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Mon, 15 May 2023 10:57:38 +0900
Message-ID: <CAMZ6RqLu9hjTApnU3C-OWiZp_1yjSTWe9jBhgajmfEezM9_s+w@mail.gmail.com>
Subject: Re: [RFC PATCH v3] can: virtio: Initial virtio CAN driver.
To:     Harald Mommer <harald.mommer@opensynergy.com>
Cc:     Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@opensynergy.com>,
        virtio-dev@lists.oasis-open.org, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        Wolfgang Grandegger <wg@grandegger.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        Damir Shaikhutdinov <Damir.Shaikhutdinov@opensynergy.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat. 13 May 2023 at 23:18, Harald Mommer
<harald.mommer@opensynergy.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;
> >> +}
> >>