Received-SPF: pass (google.com: domain of linux-kernel+bounces-48738-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <0878aedf-35c2-4901-8662-2688574dd06f@opensynergy.com>
Date: Thu, 1 Feb 2024 19:57:45 +0100
Subject: Re: [PATCH v5] can: virtio: Initial virtio CAN driver.
Content-Language: en-US
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
 Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@opensynergy.com>,
 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>,
 Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Cc: Damir Shaikhutdinov <Damir.Shaikhutdinov@opensynergy.com>,
 linux-kernel@vger.kernel.org, linux-can@vger.kernel.org,
 netdev@vger.kernel.org, virtualization@lists.linux.dev
References: <20240108131039.2234044-1-Mikhail.Golubev-Ciuchea@opensynergy.com>
 <a366f529-c901-4cd1-a1a6-c3958562cace@wanadoo.fr>
From: Harald Mommer <harald.mommer@opensynergy.com>
In-Reply-To: <a366f529-c901-4cd1-a1a6-c3958562cace@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bFVRRExZcWVBUDZMT3VjTDBRSFE4WklNak1pQU41YzA1NnJ4QStFVStKZzVG?=
 =?utf-8?B?bjBMZGNaZExvQkZUTE5hOEJyNWlReWtkSHNqdUt4RC9UTmJ5UjB0cS9WWTgz?=
 =?utf-8?B?WGx1MWVqOUhOS1dmNFBYYWo5dGY5dlFSUWZLckdKN2svMXowYTNZeGViN1hN?=
 =?utf-8?B?bFJMYTRYYkFlcnp6MWVWRStQK0ptTGVPZ2RteWtWZHVCSkRJT3pGN1hRYThZ?=
 =?utf-8?B?L0grNm0vNHVBL2lOQmp1WFVYQThKVmtiT2U3MkNHMGVtVTRodGtlRHRxa3Jr?=
 =?utf-8?B?T1Y5ZDRKTUFhUWlSSmlmWkxnSUZobzU4SG0xY25pc09MYlNXbGkyOTFQZzl5?=
 =?utf-8?B?cTY1VXU3NDVqUitqTjdNL0R4eU5RSkZ3Tld0a1EwbmJwZURwZHVzcXpDcE9O?=
 =?utf-8?B?QThDNWxmNlZtTzR4Mlk3QnRydDFWWGdzUHhzMnVneXBrVXlWUFl2UEhuTXVR?=
 =?utf-8?B?NHVTbG8vRU4vN1ZzNmU1NFhSNjNXWFcrcExScGZLR0JIVHhWS1JkWDlOM3lC?=
 =?utf-8?B?TmgzRlR0cmJ3NGFHM0hKeFVITVVvbW12ZnUzMkZCYkYxcHl2YS92bDVoNFBR?=
 =?utf-8?B?dlZNYnhBb3ZNQ3VYMVZ6SWRDS1VGTlRDUUczYVpXN1ZoVW44SnZOSzR3M1RM?=
 =?utf-8?B?TTZjeVFjM1ZkTEJ6amY5aDNPdERESWJEWXhUOHZQNG04TU53bHJrNHlja3lE?=
 =?utf-8?B?YzA1UGhxU2xWWTF6cTJBUlR0YkF1WGYvdjlhd2pURFh4V3pXeUNXbUdPM1Z3?=
 =?utf-8?B?SC81dkY3TnlMK1Yxb3BNWUMzNnlyWk9ZSmVacms0ZWxMRTlSU0M5MzFqbzZT?=
 =?utf-8?B?Sis0anBCSm1pR2U2VlpZWFllZlBMbEF2THc1aTRKNUdydUwwZGRIMmhkaGZm?=
 =?utf-8?B?RjRGRUtpb3FVVHpKQ0xMUDdvN2VOeXRvNDVSOVJHV2l4STZLdmRWTjgyNlpG?=
 =?utf-8?B?cUVtUlZNWWk2bDZtWGxQN2o1OVNNN1V3TVdhL2xCalM1U1FibmlseDNYRzhK?=
 =?utf-8?B?RFptUG5XQlFzdTcrY09BOCtuaTRZN3NKQ05mVWNKR3c0TytEQmw3SmtiK1R3?=
 =?utf-8?B?Q0llZzNNalZTQjdpRFF0V05LeGtKZHFSSVZUeXhETHBGM1A5STZYV0FQdC82?=
 =?utf-8?B?cENhMW1GaWM1M0J2a2FhcFBmRE14YlJYZ2JkRWFDNDZrMDRSWG9nVUZjRi9y?=
 =?utf-8?B?cVRGWGc1N1FhSU1yaFVxNHBUL3RXU0pTOVRMUmVVMjBRMTFPZEY0MDM2ajU5?=
 =?utf-8?B?aTFzZ0FHUEpDMTM2WmVYRkNNTVJ2dUE0Mzd0RENpWFlsZ0hOVkFFc3lwUmlS?=
 =?utf-8?B?Q1lhR3h2VlFyZVVxZU1SSEpYWDBGWDVBaDF6THlSdUs0UXV0UE9LdFE5WFJZ?=
 =?utf-8?B?SzZuMnRGSXJqREY1Lzc4bDdQM3B1YWdBZFp6UWZCaE5zVXpyZ2ZiVWFmS0da?=
 =?utf-8?B?K2paSXd3NDY4OEkwTzJPUVpKZmxxMG5GQ05hQis2R3RJUFRXM0pmSnhpTmgv?=
 =?utf-8?B?YW5kMnMxQjdvMTRIdWZlRlVoRDRSS04wK2FQc2VUc3U2MGFUSDF2M1RrMVU5?=
 =?utf-8?B?TVdtNG9ySjRkRVkzTDV5UGlvYmZ6eWJOQWlkVHBKSzVWc29hanl1RUcxNTdL?=
 =?utf-8?B?ckk5RlBOQi9PUGhHbjhqWWhFVnVxR3BrMkJ1RjRxY21hcWhUSTFrYzhVbDU0?=
 =?utf-8?B?VlNKTTY4dlp4dGhtQm4wd0w2cXdtbEN5WXJrZVUwbGV0ODVGL3FZQU5xUVlK?=
 =?utf-8?B?b0g3OTJGNkhIbHNwSUNzOUJRU25IYlJ5WEtpTG5RUnppczhGUm1xUHh4dGhE?=
 =?utf-8?B?STRpV05jN2VOZkFYVVF4YXZzRWdnd2s1LzVxcXRlVndybWdzTTBPNzJvVjBO?=
 =?utf-8?B?bWNKSlZBbFR0djFYOVFnamZ2cnd1OUdmSi9zMWUxbm81RzZEMjlwa240SGl3?=
 =?utf-8?B?Z1hLUHBiWDlnT3YrcnVXRW5BRHFtdlVqZGpTckFYbG9yem9oU0tVWDZvQnRm?=
 =?utf-8?B?OHYrc0I1TWp1UHhUbnErLzA5UFZaOHhITmhzalRCOXQ1cnJvcWRpZ2IzM3Z2?=
 =?utf-8?B?MWh5NmZxbmtsSjVaaExKWnhGQXFPUGgxdkp2Zm1CM2FpcmJ3c1htVGRDVUVT?=
 =?utf-8?Q?ihFXTX7gWGp2hPbbteawBCfDI?=
X-OriginatorOrg: opensynergy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4896b714-0591-443f-9137-08dc2357ae0e
X-MS-Exchange-CrossTenant-AuthSource: BEZP281MB2279.DEUP281.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2024 18:57:47.0024
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 800fae25-9b1b-4edc-993d-c939c4e84a64
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2p1N4jtRwYjRZKdoihYc83eyXqeQJn91xZMF5Vz/1IfthCwaFUqdU+pSoNehWj3fRF2BrXzqnZ3cUImHVM7Vpw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BEZP281MB2343
X-TM-AS-ERS: 104.47.7.169-0.0.0.0
X-TMASE-Version: StarCloud-1.3-9.1.1015-28158.001
X-TMASE-Result: 10--20.387600-4.000000
X-TMASE-MatchedRID: UuaOI1zLN1j7aDO4CTTXAUu+LBodhvWx3B1hSgpgA3NLbD6MAjV/HXkS
	2NLbu9vrHe/Upp/4ecR55GhBiSRnqdLnVLHHb0BJJ96EZQbLI1wnfL6KqKSjb7AFnlFVORaFHzZ
	m1Hzj3+G2Jx4NJM8XQj0PPrfB1Bh4PNrwgyWv0FiFhGp9GNPDremlj3mhNSlqjuOgrRhTc4Aovn
	Wk0W+Kc5wF9/WUecaz/goanha5wsKVUfMM1YjB30kXVQaP16N5AJ4bT6ig74ci1tnHAtK8I+TWQ
	MB+YaBlLqQeOQ4Yktfi8HzKNwucr8imVA71DJInuva7QpapAo3j08NQZZ6WgbDKT8rFLjFMW/1Y
	AH/fjRTfXDU2ciEUMsKjGRQnQ81hjWDfMO3Wy/k0UkwbRDtAu1xQCEUiIhCicYO7LzLWqS4D/dH
	yT/Xh7Q==
X-TMASE-XGENCLOUD: 6a90687c-9de7-4693-b320-4061278f29e6-0-0-200-0
X-TM-Deliver-Signature: EB7D5A088DDC6AE0F39EE35575067D4B
X-TM-Addin-Auth: PhzsgAzHGC0aLkhDNMKp7S9baJwBQqI2AXgWIqhHN5vMhjIJ89CJSi7axkl
	hMfZkidWRo+Kch2P/UjXzC7CARNhRqZbKVz64DPCNUPqASvL63Hj5D5IJmTo39GwiHVHS/maEL9
	RZNJ26NKFIGGrGrNg0ANrq9T8p3lFJMhDhwvMVdC2zwfx8DyP6sKyJZkK5+3wqeWCLI2ks5hlh8
	Ww9a+3kw41muMUMpL3diRF7MBKmqIUzv0MSSZgj4hEUyycdxZ93lXkdCYZMWnmMioL280w0dTkQ
	IzvhKOwEk9Fqrk0=.Roo4zbJplWNWZ09rQRHFs0UmDpwD4iIqY+1C+ZXrw2ozIT2XcpkTM8rDu9
	DFDMmc6BX6kWfnz7i6mlpZCzKCwXBJyqFRLWGsW703NxzC6IU20iy3TNAuFejoXiI0UXu/+ctyV
	9650UdwfahYsP0gPmGjy/YuEIGbqdKhEdF1OXqNf2+kYtdifHK3Y6f8HSoGGWvImJKNpZYGeEj6
	k7WVHmtSllxefODMbLeY3JqnfpJR/MkbTqJB6/u7aSK7r6DFGP+4LFhDUKBENmX3bVmdLWQjsh3
	AQPMJS5SpiEhMs3aF50TVE+d3r4RguvZ/vptTDzTgCZcB82EhuUhpe29VVQ==
X-TM-Addin-ProductCode: EMS
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=opensynergy.com;
	s=TM-DKIM-20210503141657; t=1706813868;
	bh=NvZCVdlBjooETymEjP5dwWcbxl1lFmbHfAodrQ+uXtM=; l=3745;
	h=Date:To:From;
	b=FpTt98YJa9vWjxpqzvqfILqW4TwH958qOn+/1hkusgvua7U8mFuAt4FsWZ6zL3tQ/
	 9LyLvlFT4pt80c+dk6UJJ0sk8hkMLT8XLyXQWSHhO60+noS4oBVjUYx/lOnz7Opr3C
	 boX1fyYC7EhYFXdH2FFsN1sPA3KMiyDQEYSQx7OF7LC02yRgo98F6FdmQUuEeMa/gV
	 zl8/Xy4DuqimHns41tbNm/K5KBhb9RDRJ77X11X8fHuA6tdlGDeoPxa6bqRPvmf3dQ
	 RTm3yoUHB7xIRzaEUL3d4jyYeM5RLDYYPctJUyPVM7boxke/Ya+pXHcX6zHbs6dpeF
	 B7BmKmQkH4Qjw==

Hello,

I thought there would be some more comments coming and I could address 
everything in one chunk. Not the case, besides your comments silence.

On 08.01.24 20:34, Christophe JAILLET wrote:
>
> Hi,
> a few nits below, should there be a v6.
>

I'm sure there will be but not so soon. Probably after acceptance of the 
virtio CAN specification or after change requests to the specification 
are received and the driver has to be adapted to an updated draft.


>
>> +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() ?


Yes, will be done, already in my local code base.


>
>> +
>> +    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() ?


See above.


>
>> +}
>
> ...
>
>> +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?


     err = register_virtio_can_dev(dev);
     if (err) {
         virtio_can_del_vq(vdev);
         dev_err(&vdev->dev, "Couldn't register candev (err=%d)\n", err);
         goto on_failure;
     }


>
> CJ
>
>> +
>> +    napi_enable(&priv->napi);
>> +    napi_enable(&priv->napi_tx);
>> +
>> +    /* Request device going live */
>> +    virtio_device_ready(vdev); /* Optionally done by 
>> virtio_dev_probe() */


The virtio_device_ready() will also be removed. The caller will make the 
device live anyway after return. There are no operations between the 
virtio_device_ready() and the return 0 which make it necessary to have 
the device live early before the return.


>> +
>> +    return 0;
>> +
>> +on_failure:
>> +    virtio_can_free_candev(dev);
>> +    return err;
>> +}
>
> ...
>