diff mbox

[dpdk-dev] net/i40e: fix wrong device flag setup

Message ID 1486164969-2566-1-git-send-email-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Qi Zhang Feb. 3, 2017, 11:36 p.m. UTC 
  dev_flags is wrong overwritten by RTE_ETH_DEV_DETACHABLE after
rte_eth_copy_pci_info.

Fixes: 22dda618c00c ("pci: separate detaching ethernet ports from PCI devices")

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/bnxt/bnxt_ethdev.c       | 2 +-
 drivers/net/e1000/igb_ethdev.c       | 4 ++--
 drivers/net/fm10k/fm10k_ethdev.c     | 2 +-
 drivers/net/i40e/i40e_ethdev.c       | 2 +-
 drivers/net/i40e/i40e_ethdev_vf.c    | 2 +-
 drivers/net/ixgbe/ixgbe_ethdev.c     | 4 ++--
 drivers/net/nfp/nfp_net.c            | 2 +-
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

Comments

Ferruh Yigit Feb. 7, 2017, 10:24 a.m. UTC | #1 
On 2/3/2017 11:36 PM, Qi Zhang wrote:
> dev_flags is wrong overwritten by RTE_ETH_DEV_DETACHABLE after
> rte_eth_copy_pci_info.
> 
> Fixes: 22dda618c00c ("pci: separate detaching ethernet ports from PCI devices")
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  drivers/net/bnxt/bnxt_ethdev.c       | 2 +-
>  drivers/net/e1000/igb_ethdev.c       | 4 ++--
>  drivers/net/fm10k/fm10k_ethdev.c     | 2 +-
>  drivers/net/i40e/i40e_ethdev.c       | 2 +-
>  drivers/net/i40e/i40e_ethdev_vf.c    | 2 +-
>  drivers/net/ixgbe/ixgbe_ethdev.c     | 4 ++--
>  drivers/net/nfp/nfp_net.c            | 2 +-
>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 2 +-
>  8 files changed, 10 insertions(+), 10 deletions(-)
> 

Hi Qi,

Thanks for the fix.

Is following missed, or intentionally excluded:

drivers/net/e1000/em_ethdev.c
drivers/net/virtio/virtio_ethdev.c


Thanks,
ferruh
Ferruh Yigit Feb. 7, 2017, 10:32 a.m. UTC | #2 
On 2/7/2017 10:24 AM, Ferruh Yigit wrote:
> On 2/3/2017 11:36 PM, Qi Zhang wrote:
>> dev_flags is wrong overwritten by RTE_ETH_DEV_DETACHABLE after
>> rte_eth_copy_pci_info.
>>
>> Fixes: 22dda618c00c ("pci: separate detaching ethernet ports from PCI devices")
>>
>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>> ---
>>  drivers/net/bnxt/bnxt_ethdev.c       | 2 +-
>>  drivers/net/e1000/igb_ethdev.c       | 4 ++--
>>  drivers/net/fm10k/fm10k_ethdev.c     | 2 +-
>>  drivers/net/i40e/i40e_ethdev.c       | 2 +-
>>  drivers/net/i40e/i40e_ethdev_vf.c    | 2 +-
>>  drivers/net/ixgbe/ixgbe_ethdev.c     | 4 ++--
>>  drivers/net/nfp/nfp_net.c            | 2 +-
>>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 2 +-
>>  8 files changed, 10 insertions(+), 10 deletions(-)
>>
> 
> Hi Qi,
> 
> Thanks for the fix.
> 
> Is following missed, or intentionally excluded:
> 
> drivers/net/e1000/em_ethdev.c

> drivers/net/virtio/virtio_ethdev.c

Hi Yuanhan,

I saw while checking for this, there can be an issue in virtio for this
path, can you please check:

eth_virtio_dev_init()
  set-some-eth_dev-dev_flags
  virtio_init_device()
    rte_eth_copy_pci_info()
      eth_dev->data->dev_flags = 0;   <---- Reset previous flags


Regards,
ferruh
Yuanhan Liu Feb. 8, 2017, 1:49 a.m. UTC | #3 
On Tue, Feb 07, 2017 at 10:32:32AM +0000, Ferruh Yigit wrote:
> On 2/7/2017 10:24 AM, Ferruh Yigit wrote:
> > On 2/3/2017 11:36 PM, Qi Zhang wrote:
> >> dev_flags is wrong overwritten by RTE_ETH_DEV_DETACHABLE after
> >> rte_eth_copy_pci_info.
> >>
...
> Hi Yuanhan,
> 
> I saw while checking for this, there can be an issue in virtio for this
> path, can you please check:
> 
> eth_virtio_dev_init()
>   set-some-eth_dev-dev_flags
>   virtio_init_device()
>     rte_eth_copy_pci_info()
>       eth_dev->data->dev_flags = 0;   <---- Reset previous flags

Hi Ferruh,

Nice catch! And yes, it's buggy. It may be trivially fixed by setting
the orders correctly. But my question is why "rte_eth_copy_pci_info"
does a reset on "dev_flags"? It's an *un-documented* side-effect of
this function, which could be very error-prone, like this one.

	--yliu
diff mbox

Patch

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 501c97d..aaff3c8 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -1087,7 +1087,7 @@  bnxt_dev_init(struct rte_eth_dev *eth_dev)
 		RTE_LOG(INFO, PMD, "%s", bnxt_version);
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 	bp = eth_dev->data->dev_private;
 
 	if (bnxt_vf_pciid(pci_dev->id.device_id))
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 8843dd1..4fd5786 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -777,7 +777,7 @@  eth_igb_dev_init(struct rte_eth_dev *eth_dev)
 	}
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	hw->hw_addr= (void *)pci_dev->mem_resource[0].addr;
 
@@ -986,7 +986,7 @@  eth_igbvf_dev_init(struct rte_eth_dev *eth_dev)
 
 	pci_dev = E1000_DEV_TO_PCI(eth_dev);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	hw->device_id = pci_dev->id.device_id;
 	hw->vendor_id = pci_dev->id.vendor_id;
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index dd021e4..c4fe746 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -2858,7 +2858,7 @@  eth_fm10k_dev_init(struct rte_eth_dev *dev)
 		return 0;
 
 	rte_eth_copy_pci_info(dev, pdev);
-	dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	macvlan = FM10K_DEV_PRIVATE_TO_MACVLAN(dev->data->dev_private);
 	memset(macvlan, 0, sizeof(*macvlan));
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index a818998..f8ec91c 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -1045,7 +1045,7 @@  eth_i40e_dev_init(struct rte_eth_dev *dev)
 	intr_handle = &pci_dev->intr_handle;
 
 	rte_eth_copy_pci_info(dev, pci_dev);
-	dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	pf->adapter = I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 	pf->adapter->eth_dev = dev;
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 17a035c..d2c25e8 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1478,7 +1478,7 @@  i40evf_dev_init(struct rte_eth_dev *eth_dev)
 	}
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	hw->vendor_id = pci_dev->id.vendor_id;
 	hw->device_id = pci_dev->id.device_id;
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index ad63e5a..c2433c0 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -1170,7 +1170,7 @@  eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
 	}
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	/* Vendor and Device ID need to be set before init of shared code */
 	hw->device_id = pci_dev->id.device_id;
@@ -1621,7 +1621,7 @@  eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
 	}
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	hw->device_id = pci_dev->id.device_id;
 	hw->vendor_id = pci_dev->id.vendor_id;
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index ab2d331..a2c78a6 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -2465,7 +2465,7 @@  nfp_net_init(struct rte_eth_dev *eth_dev)
 
 	pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	hw->device_id = pci_dev->id.device_id;
 	hw->vendor_id = pci_dev->id.vendor_id;
diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 52e3644..2a52f04 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -250,7 +250,7 @@  eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev)
 		return 0;
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
-	eth_dev->data->dev_flags = RTE_ETH_DEV_DETACHABLE;
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;
 
 	/* Vendor and Device ID need to be set before init of shared code */
 	hw->device_id = pci_dev->id.device_id;