Hi Mattias,
>-----Original Message-----
>From: Mattias Rönnblom <hof...@lysator.liu.se>
>Sent: Saturday, October 30, 2021 1:38 AM
>To: Vijay Kumar Srivastava <vsriv...@xilinx.com>; dev@dpdk.org
>Cc: maxime.coque...@redhat.com; chenbo....@intel.com;
>andrew.rybche...@oktetlabs.ru; Vijay Kumar Srivastava
><vsriv...@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 <vsriv...@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]
