[dpdk-dev,v4,21/23] ethdev: Move filling of rte_eth_dev_info->pci_dev to dev_infos_get()
Checks
Commit Message
Only the device itself can decide its PCI or not.
Signed-off-by: Jan Blunck <jblunck@infradead.org>
Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
drivers/net/bnx2x/bnx2x_ethdev.c | 1 +
drivers/net/bnxt/bnxt_ethdev.c | 2 ++
drivers/net/cxgbe/cxgbe_ethdev.c | 2 ++
drivers/net/e1000/em_ethdev.c | 1 +
drivers/net/e1000/igb_ethdev.c | 2 ++
drivers/net/ena/ena_ethdev.c | 2 ++
drivers/net/enic/enic_ethdev.c | 1 +
drivers/net/fm10k/fm10k_ethdev.c | 1 +
drivers/net/i40e/i40e_ethdev.c | 1 +
drivers/net/i40e/i40e_ethdev_vf.c | 1 +
drivers/net/ixgbe/ixgbe_ethdev.c | 2 ++
drivers/net/mlx4/mlx4.c | 2 ++
drivers/net/mlx5/mlx5_ethdev.c | 2 ++
drivers/net/nfp/nfp_net.c | 1 +
drivers/net/qede/qede_ethdev.c | 1 +
drivers/net/szedata2/rte_eth_szedata2.c | 1 +
drivers/net/thunderx/nicvf_ethdev.c | 2 ++
drivers/net/virtio/virtio_ethdev.c | 1 +
drivers/net/vmxnet3/vmxnet3_ethdev.c | 4 +++-
lib/librte_ether/rte_ethdev.c | 1 -
20 files changed, 29 insertions(+), 2 deletions(-)
Comments
On Wed, 21 Dec 2016 16:09:44 +0100
Jan Blunck <jblunck@infradead.org> wrote:
> Only the device itself can decide its PCI or not.
>
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
I would still like to kill dev_pci from the dev_info API.
On Thursday 22 December 2016 01:39 AM, Stephen Hemminger wrote:
> On Wed, 21 Dec 2016 16:09:44 +0100
> Jan Blunck <jblunck@infradead.org> wrote:
>
>> Only the device itself can decide its PCI or not.
>>
>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>
> I would still like to kill dev_pci from the dev_info API.
>
+1. It should be rte_dev reference instead.
(But, I am also OK about doing that immediately over this set.)
-
Shreyansh
On Thu, Dec 22, 2016 at 9:11 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> On Thursday 22 December 2016 01:39 AM, Stephen Hemminger wrote:
>>
>> On Wed, 21 Dec 2016 16:09:44 +0100
>> Jan Blunck <jblunck@infradead.org> wrote:
>>
>>> Only the device itself can decide its PCI or not.
>>>
>>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>>> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>> ---
>>
>>
>> I would still like to kill dev_pci from the dev_info API.
>>
I'm fine with that too.
>
> +1. It should be rte_dev reference instead.
>
Only if you can give use-cases for what users should be able to do
with it. If that is the case we need to clearly define what that
means. Do we want to enable users to control the low-level EAL device
directly and shortcut the ethdev driver? If that is necessary we need
to give control to the driver first to decide if it is safe to do so.
On Friday 23 December 2016 04:20 PM, Jan Blunck wrote:
> On Thu, Dec 22, 2016 at 9:11 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> On Thursday 22 December 2016 01:39 AM, Stephen Hemminger wrote:
>>>
>>> On Wed, 21 Dec 2016 16:09:44 +0100
>>> Jan Blunck <jblunck@infradead.org> wrote:
>>>
>>>> Only the device itself can decide its PCI or not.
>>>>
>>>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>>>> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>>> ---
>>>
>>>
>>> I would still like to kill dev_pci from the dev_info API.
>>>
>
> I'm fine with that too.
>
>>
>> +1. It should be rte_dev reference instead.
>>
>
> Only if you can give use-cases for what users should be able to do
> with it. If that is the case we need to clearly define what that
> means. Do we want to enable users to control the low-level EAL device
> directly and shortcut the ethdev driver? If that is necessary we need
> to give control to the driver first to decide if it is safe to do so.
>
An ethernet device is not necessarily a PCI device. With planned removal
of rte_pci_device from rte_eth_device, this will be realized.
Similarly, the info is also not PCI device specific.
With the '+1', my intention was not to say we should do it in this
patch. We should prepare eth_dev_info in similar manner as done for
pci_dev of rte_eth_dev (ETH_DEV_PCI_DEV() style macro, or inline).
And now for whether we should expose lower level device details or not,
I was of the view that keeping pci_dev linked to this structure exposes
more lower level info than keeping rte_dev. Another view point could be
to completely do away with pci_info within eth_dev_info - but, I am not
sure of dependencies on it.
-
Shreyansh
On Fri, Dec 23, 2016 at 12:11 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> On Friday 23 December 2016 04:20 PM, Jan Blunck wrote:
>>
>> On Thu, Dec 22, 2016 at 9:11 AM, Shreyansh Jain <shreyansh.jain@nxp.com>
>> wrote:
>>>
>>> On Thursday 22 December 2016 01:39 AM, Stephen Hemminger wrote:
>>>>
>>>>
>>>> On Wed, 21 Dec 2016 16:09:44 +0100
>>>> Jan Blunck <jblunck@infradead.org> wrote:
>>>>
>>>>> Only the device itself can decide its PCI or not.
>>>>>
>>>>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>>>>> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>>>> ---
>>>>
>>>>
>>>>
>>>> I would still like to kill dev_pci from the dev_info API.
>>>>
>>
>> I'm fine with that too.
>>
>>>
>>> +1. It should be rte_dev reference instead.
>>>
>>
>> Only if you can give use-cases for what users should be able to do
>> with it. If that is the case we need to clearly define what that
>> means. Do we want to enable users to control the low-level EAL device
>> directly and shortcut the ethdev driver? If that is necessary we need
>> to give control to the driver first to decide if it is safe to do so.
>>
>
> An ethernet device is not necessarily a PCI device. With planned removal of
> rte_pci_device from rte_eth_device, this will be realized.
> Similarly, the info is also not PCI device specific.
>
> With the '+1', my intention was not to say we should do it in this patch. We
> should prepare eth_dev_info in similar manner as done for pci_dev of
> rte_eth_dev (ETH_DEV_PCI_DEV() style macro, or inline).
Which is exactly what this patch is doing. I'm moving the filling of
the PCI information out of the generic code because only the driver
could know if it is actually handling a PCI device. The generic code
can not use the ETH_DEV_PCI_DEV() macro in a safe manner.
> And now for whether we should expose lower level device details or not, I
> was of the view that keeping pci_dev linked to this structure exposes more
> lower level info than keeping rte_dev. Another view point could be to
> completely do away with pci_info within eth_dev_info - but, I am not sure of
> dependencies on it.
If I understand Stephen correctly he questions the benefit of pushing
down the code to the drivers instead of killing that code completely.
I'll see what I can do here.
On Friday 23 December 2016 04:57 PM, Jan Blunck wrote:
> On Fri, Dec 23, 2016 at 12:11 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> On Friday 23 December 2016 04:20 PM, Jan Blunck wrote:
>>>
>>> On Thu, Dec 22, 2016 at 9:11 AM, Shreyansh Jain <shreyansh.jain@nxp.com>
>>> wrote:
>>>>
>>>> On Thursday 22 December 2016 01:39 AM, Stephen Hemminger wrote:
>>>>>
>>>>>
>>>>> On Wed, 21 Dec 2016 16:09:44 +0100
>>>>> Jan Blunck <jblunck@infradead.org> wrote:
>>>>>
>>>>>> Only the device itself can decide its PCI or not.
>>>>>>
>>>>>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>>>>>> Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>
>>>>> I would still like to kill dev_pci from the dev_info API.
>>>>>
>>>
>>> I'm fine with that too.
>>>
>>>>
>>>> +1. It should be rte_dev reference instead.
>>>>
>>>
>>> Only if you can give use-cases for what users should be able to do
>>> with it. If that is the case we need to clearly define what that
>>> means. Do we want to enable users to control the low-level EAL device
>>> directly and shortcut the ethdev driver? If that is necessary we need
>>> to give control to the driver first to decide if it is safe to do so.
>>>
>>
>> An ethernet device is not necessarily a PCI device. With planned removal of
>> rte_pci_device from rte_eth_device, this will be realized.
>> Similarly, the info is also not PCI device specific.
>>
>> With the '+1', my intention was not to say we should do it in this patch. We
>> should prepare eth_dev_info in similar manner as done for pci_dev of
>> rte_eth_dev (ETH_DEV_PCI_DEV() style macro, or inline).
>
> Which is exactly what this patch is doing. I'm moving the filling of
> the PCI information out of the generic code because only the driver
> could know if it is actually handling a PCI device. The generic code
> can not use the ETH_DEV_PCI_DEV() macro in a safe manner.
I fully agree with you and support this series.
Probably I went a step further in my eagerness - I was only hinting that
now info filling part is relatively free of pci-ness, we should remove
eth_dev_info linkage to PCI.
There is no change I am expecting in your patch in this regard. Yours is
the first step to this change.
>
>> And now for whether we should expose lower level device details or not, I
>> was of the view that keeping pci_dev linked to this structure exposes more
>> lower level info than keeping rte_dev. Another view point could be to
>> completely do away with pci_info within eth_dev_info - but, I am not sure of
>> dependencies on it.
>
> If I understand Stephen correctly he questions the benefit of pushing
> down the code to the drivers instead of killing that code completely.
> I'll see what I can do here.
>
Ok. Then I read it wrong. I read it in the sense that we should remove
PCI dependency of eth_info. Except, I considered that removal more as
replacement with rte_device. My mistake!
@@ -431,6 +431,7 @@ static void
bnx2x_dev_infos_get(struct rte_eth_dev *dev, __rte_unused struct rte_eth_dev_info *dev_info)
{
struct bnx2x_softc *sc = dev->data->dev_private;
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->max_rx_queues = sc->max_rx_queues;
dev_info->max_tx_queues = sc->max_tx_queues;
dev_info->min_rx_bufsize = BNX2X_MIN_RX_BUF_SIZE;
@@ -303,6 +303,8 @@ static void bnxt_dev_info_get_op(struct rte_eth_dev *eth_dev,
struct bnxt *bp = (struct bnxt *)eth_dev->data->dev_private;
uint16_t max_vnics, i, j, vpool, vrxq;
+ dev_info->pci_dev = rte_eth_dev_to_pci(eth_dev);
+
/* MAC Specifics */
dev_info->max_mac_addrs = MAX_NUM_MAC_ADDR;
dev_info->max_hash_mac_addrs = 0;
@@ -147,6 +147,8 @@ static void cxgbe_dev_info_get(struct rte_eth_dev *eth_dev,
.nb_align = 1,
};
+ device_info->pci_dev = rte_eth_dev_to_pci(eth_dev);
+
device_info->min_rx_bufsize = CXGBE_MIN_RX_BUFSIZE;
device_info->max_rx_pktlen = CXGBE_MAX_RX_PKTLEN;
device_info->max_rx_queues = max_queues;
@@ -1048,6 +1048,7 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
{
struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->min_rx_bufsize = 256; /* See BSIZE field of RCTL register. */
dev_info->max_rx_pktlen = em_get_max_pktlen(hw);
dev_info->max_mac_addrs = hw->mac.rar_entry_count;
@@ -1986,6 +1986,7 @@ eth_igb_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
{
struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->min_rx_bufsize = 256; /* See BSIZE field of RCTL register. */
dev_info->max_rx_pktlen = 0x3FFF; /* See RLPML register. */
dev_info->max_mac_addrs = hw->mac.rar_entry_count;
@@ -2114,6 +2115,7 @@ eth_igbvf_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
{
struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->min_rx_bufsize = 256; /* See BSIZE field of RCTL register. */
dev_info->max_rx_pktlen = 0x3FFF; /* See RLPML register. */
dev_info->max_mac_addrs = hw->mac.rar_entry_count;
@@ -1436,6 +1436,8 @@ static void ena_infos_get(struct rte_eth_dev *dev,
ena_dev = &adapter->ena_dev;
ena_assert_msg(ena_dev != NULL, "Uninitialized device");
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
+
dev_info->speed_capa =
ETH_LINK_SPEED_1G |
ETH_LINK_SPEED_2_5G |
@@ -459,6 +459,7 @@ static void enicpmd_dev_info_get(struct rte_eth_dev *eth_dev,
struct enic *enic = pmd_priv(eth_dev);
ENICPMD_FUNC_TRACE();
+ device_info->pci_dev = rte_eth_dev_to_pci(eth_dev);
/* Scattered Rx uses two receive queues per rx queue exposed to dpdk */
device_info->max_rx_queues = enic->conf_rq_count / 2;
device_info->max_tx_queues = enic->conf_wq_count;
@@ -1393,6 +1393,7 @@ fm10k_dev_infos_get(struct rte_eth_dev *dev,
PMD_INIT_FUNC_TRACE();
+ dev_info->pci_dev = pdev;
dev_info->min_rx_bufsize = FM10K_MIN_RX_BUF_SIZE;
dev_info->max_rx_pktlen = FM10K_MAX_PKT_SIZE;
dev_info->max_rx_queues = hw->mac.max_queues;
@@ -2596,6 +2596,7 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
struct i40e_vsi *vsi = pf->main_vsi;
struct rte_pci_device *pci_dev = rte_eth_dev_to_pci(dev);
+ dev_info->pci_dev = pci_dev;
dev_info->max_rx_queues = vsi->nb_qps;
dev_info->max_tx_queues = vsi->nb_qps;
dev_info->min_rx_bufsize = I40E_BUF_SIZE_MIN;
@@ -2224,6 +2224,7 @@ i40evf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
memset(dev_info, 0, sizeof(*dev_info));
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->max_rx_queues = vf->vsi_res->num_queue_pairs;
dev_info->max_tx_queues = vf->vsi_res->num_queue_pairs;
dev_info->min_rx_bufsize = I40E_BUF_SIZE_MIN;
@@ -3035,6 +3035,7 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
+ dev_info->pci_dev = pci_dev;
dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
dev_info->max_tx_queues = (uint16_t)hw->mac.max_tx_queues;
if (RTE_ETH_DEV_SRIOV(dev).active == 0) {
@@ -3167,6 +3168,7 @@ ixgbevf_dev_info_get(struct rte_eth_dev *dev,
struct rte_pci_device *pci_dev = rte_eth_dev_to_pci(dev);
struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ dev_info->pci_dev = pci_dev;
dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
dev_info->max_tx_queues = (uint16_t)hw->mac.max_tx_queues;
dev_info->min_rx_bufsize = 1024; /* cf BSIZEPACKET in SRRCTL reg */
@@ -4421,6 +4421,8 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
unsigned int max;
char ifname[IF_NAMESIZE];
+ info->pci_dev = rte_eth_dev_to_pci(dev);
+
if (priv == NULL)
return;
priv_lock(priv);
@@ -562,6 +562,8 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
unsigned int max;
char ifname[IF_NAMESIZE];
+ info->pci_dev = rte_eth_dev_to_pci(dev);
+
priv_lock(priv);
/* FIXME: we should ask the device for these values. */
info->min_rx_bufsize = 32;
@@ -1008,6 +1008,7 @@ nfp_net_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->driver_name = dev->driver->pci_drv.driver.name;
dev_info->max_rx_queues = (uint16_t)hw->max_rx_queues;
dev_info->max_tx_queues = (uint16_t)hw->max_tx_queues;
@@ -651,6 +651,7 @@ qede_dev_info_get(struct rte_eth_dev *eth_dev,
PMD_INIT_FUNC_TRACE(edev);
+ dev_info->pci_dev = rte_eth_dev_to_pci(eth_dev);
dev_info->min_rx_bufsize = (uint32_t)(ETHER_MIN_MTU +
QEDE_ETH_OVERHEAD);
dev_info->max_rx_pktlen = (uint32_t)ETH_TX_MAX_NON_LSO_PKT_LEN;
@@ -1031,6 +1031,7 @@ eth_dev_info(struct rte_eth_dev *dev,
struct rte_eth_dev_info *dev_info)
{
struct pmd_internals *internals = dev->data->dev_private;
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
dev_info->if_index = 0;
dev_info->max_mac_addrs = 1;
dev_info->max_rx_pktlen = (uint32_t)-1;
@@ -1339,6 +1339,8 @@ nicvf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
PMD_INIT_FUNC_TRACE();
+ dev_info->pci_dev = pci_dev;
+
dev_info->min_rx_bufsize = ETHER_MIN_MTU;
dev_info->max_rx_pktlen = NIC_HW_MAX_FRS;
dev_info->max_rx_queues =
@@ -1626,6 +1626,7 @@ virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
uint64_t tso_mask;
struct virtio_hw *hw = dev->data->dev_private;
+ dev_info->pci_dev = hw->dev;
dev_info->max_rx_queues =
RTE_MIN(hw->max_queue_pairs, VIRTIO_MAX_RX_QUEUES);
dev_info->max_tx_queues =
@@ -706,9 +706,11 @@ vmxnet3_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
}
static void
-vmxnet3_dev_info_get(__rte_unused struct rte_eth_dev *dev,
+vmxnet3_dev_info_get(struct rte_eth_dev *dev,
struct rte_eth_dev_info *dev_info)
{
+ dev_info->pci_dev = rte_eth_dev_to_pci(dev);
+
dev_info->max_rx_queues = VMXNET3_MAX_RX_QUEUES;
dev_info->max_tx_queues = VMXNET3_MAX_TX_QUEUES;
dev_info->min_rx_bufsize = 1518 + RTE_PKTMBUF_HEADROOM;
@@ -1568,7 +1568,6 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info)
RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
(*dev->dev_ops->dev_infos_get)(dev, dev_info);
- dev_info->pci_dev = dev->pci_dev;
dev_info->driver_name = dev->data->drv_name;
dev_info->nb_rx_queues = dev->data->nb_rx_queues;
dev_info->nb_tx_queues = dev->data->nb_tx_queues;