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

[Vijay Kumar Srivastava <vsrivast@xilinx.com>]

Hi Mattias,

>-----Original Message-----
>From: Mattias Rönnblom <hofors@lysator.liu.se>
>Sent: Saturday, October 30, 2021 1:38 AM
>To: Vijay Kumar Srivastava <vsrivast@xilinx.com>; dev@dpdk.org
>Cc: maxime.coquelin@redhat.com; chenbo.xia@intel.com; 
>andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava 
><vsrivast@xilinx.com>
>Subject: Re: [dpdk-dev] [PATCH v3 01/10] vdpa/sfc: introduce Xilinx 
>vDPA driver
>
>On 2021-10-29 16:46, Vijay Srivastava wrote:
>> From: Vijay Kumar Srivastava <vsrivast@xilinx.com>

[SNIP]
>>
>> +struct sfc_vdpa_adapter *
>> +sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev) {
>> +	bool found = false;
>> +	struct sfc_vdpa_adapter *sva;
>
>Remove found flag and set sva to NULL here instead.
This flag is needed for the scenario when  pdev != sva->pdev.
In this scenario, sva would be non-null and it should not be returned. 
So I think it's OK to keep this flag. 

>> +
>> +	pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
>> +
>> +	TAILQ_FOREACH(sva, &sfc_vdpa_adapter_list, next) {
>> +		if (pdev == sva->pdev) {
>> +			found = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
>> +
>> +	return found ? sva : NULL;
>> +}
>> +
>> +static int
>> +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)
>> +		goto fail_inval;
>> +
>> +	rte_pci_device_name(&dev->addr, dev_name,
>RTE_DEV_NAME_MAX_LEN);
>> +
>> +	sva->vfio_container_fd = rte_vfio_container_create();
>> +	if (sva->vfio_container_fd < 0)	{
>> +		sfc_vdpa_err(sva, "failed to create VFIO container");
>> +		goto fail_container_create;
>> +	}
>> +
>> +	rc = rte_vfio_get_group_num(rte_pci_get_sysfs_path(), dev_name,
>> +				    &sva->iommu_group_num);
>> +	if (rc <= 0) {
>
>Only rc < 0 guarantees that rte_errno is set.
rte_vfio_get_group_num retunrs >0 on success 0 for non-existent group or VFIO <0 for errors so used check '(rc <= 0)' looks OK.

>> +		sfc_vdpa_err(sva, "failed to get IOMMU group for %s : %s",
>> +			     dev_name, rte_strerror(-rc));
>> +		goto fail_get_group_num;
>> +	}
>> +
>> +	sva->vfio_group_fd =
>> +		rte_vfio_container_group_bind(sva->vfio_container_fd,
>> +					      sva->iommu_group_num);
>> +	if (sva->vfio_group_fd < 0) {
>> +		sfc_vdpa_err(sva,
>> +			     "failed to bind IOMMU group %d to container %d",
>> +			     sva->iommu_group_num, sva->vfio_container_fd);
>> +		goto fail_group_bind;
>> +	}
>> +
>> +	if (rte_pci_map_device(dev) != 0) {
>> +		sfc_vdpa_err(sva, "failed to map PCI device %s : %s",
>> +			     dev_name, rte_strerror(rte_errno));
>> +		goto fail_pci_map_device;
>> +	}
>> +
>> +	sva->vfio_dev_fd = rte_intr_dev_fd_get(dev->intr_handle);
>> +
>> +	return 0;
>> +
>> +fail_pci_map_device:
>> +	if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
>> +					sva->iommu_group_num) != 0) {
>> +		sfc_vdpa_err(sva,
>> +			     "failed to unbind IOMMU group %d from container
>%d",
>> +			     sva->iommu_group_num, sva->vfio_container_fd);
>> +	}
>> +
>> +fail_group_bind:
>> +fail_get_group_num:
>> +	if (rte_vfio_container_destroy(sva->vfio_container_fd) != 0) {
>
>Don't use braces for single statements, per DPDK coding style.
As per DPDK coding guidelines : 
"Do not use braces ({ and }) for control statements with zero or just a single statement, unless that statement is more 
than a single line in which case the braces are permitted." 

I believe it can be applied here as well as it has more than one line.

Regards,
Vijay

>> +		sfc_vdpa_err(sva, "failed to destroy container %d",
>> +			     sva->vfio_container_fd);
>> +	}

[SNIP]