[dpdk-dev] [PATCH v3 01/10] vdpa/sfc: introduce Xilinx vDPA driver

[Vijay Kumar Srivastava]

Hi Chenbo, 

>-----Original Message-----
>From: Xia, Chenbo <chenbo.xia at intel.com>
>Sent: Monday, November 1, 2021 2:01 PM
>To: Vijay Kumar Srivastava <vsrivast at xilinx.com>; dev at dpdk.org
>Cc: maxime.coquelin at redhat.com; andrew.rybchenko at oktetlabs.ru; Vijay
>Kumar Srivastava <vsrivast at xilinx.com>
>Subject: RE: [PATCH v3 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>
>Hi Vijay,
>
>> -----Original Message-----
>> From: Vijay Srivastava <vijay.srivastava at xilinx.com>
>> Sent: Friday, October 29, 2021 10:47 PM
>> To: dev at dpdk.org
>> Cc: maxime.coquelin at redhat.com; Xia, Chenbo <chenbo.xia at intel.com>;
>> andrew.rybchenko at oktetlabs.ru; Vijay Kumar Srivastava
>> <vsrivast at xilinx.com>
>> Subject: [PATCH v3 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>>
>> From: Vijay Kumar Srivastava <vsrivast at xilinx.com>
>>
>> Add new vDPA PMD to support vDPA operations of Xilinx devices.
>> This patch implements probe and remove functions.
>>
>> Signed-off-by: Vijay Kumar Srivastava <vsrivast at xilinx.com>
>> Acked-by: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>
>Note that your patch is not rebased on the latest commit. Reviewers may have
>trouble to review your patch.
Ack.

When I had prepared the patch series I needed patches from main (or next-net) due to changes 
in the  intr_handle (internal) as at that time next-virtio was not updated. 

>> ---
>> v2:
[SNIP]
>> --- a/doc/guides/vdpadevs/index.rst
>> +++ b/doc/guides/vdpadevs/index.rst
>> @@ -14,3 +14,5 @@ which can be used from an application through vhost
>API.
>>      features_overview
>>      ifc
>>      mlx5
>> +    sfc
>> +
>
>The above blank line not needed.
Ack.

>> diff --git a/doc/guides/vdpadevs/sfc.rst b/doc/guides/vdpadevs/sfc.rst
>> +sfc_vdpa_vfio_setup(struct sfc_vdpa_adapter *sva) {
>> +	struct rte_pci_device *dev = sva->pdev;
>> +	char dev_name[RTE_DEV_NAME_MAX_LEN] = {0};
>> +	int rc;
>> +
>> +	if (dev == NULL)
>
>I think it will never happen
Yes, This check can be removed. 

>> +		goto fail_inval;
>> +
>> +	rte_pci_device_name(&dev->addr, dev_name,
>RTE_DEV_NAME_MAX_LEN);

[SNIP]

>> +#include <rte_bus_pci.h>
>> +
>> +#include "sfc_vdpa_log.h"
>> +
>> +/* Adapter private data */
>> +struct sfc_vdpa_adapter {
>> +	TAILQ_ENTRY(sfc_vdpa_adapter)	next;
>> +	struct rte_pci_device		*pdev;
>> +	struct rte_pci_addr		pci_addr;
>
>I don't think pci_addr is needed, you can get it through pdev.
Ack. 

>> +
>> +	char