[dpdk-dev] net/ixgbe: clean up rte_eth_dev_info_get
Checks
Commit Message
It'not appropriate to call rte_eth_dev_info_get in PMD,
as rte_eth_dev_info_get need to get info from PMD.
Remove rte_eth_dev_info_get from PMD code and get the
info directly.
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
1 file changed, 68 insertions(+), 76 deletions(-)
Comments
On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> It'not appropriate to call rte_eth_dev_info_get in PMD,
> as rte_eth_dev_info_get need to get info from PMD.
> Remove rte_eth_dev_info_get from PMD code and get the
> info directly.
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
> 1 file changed, 68 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 64ce55a..f14a68b 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> int rar_entry;
> uint8_t *new_mac = (uint8_t *)(mac_addr);
> struct rte_eth_dev *dev;
> - struct rte_eth_dev_info dev_info;
> + struct rte_pci_device *pci_dev;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>
> dev = &rte_eth_devices[port];
> - rte_eth_dev_info_get(port, &dev_info);
> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>
> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> + if (is_ixgbe_pmd(dev->data->drv_name))
> return -ENOTSUP;
>
The return value of is_ixgbe_pmd() is not boolean (actually I think it
should be based on its name). If we omit the comparison with 0, the code
looks weird. It looks like it'll return -ENOTSUP if the port's driver
is ixgbe PMD.
Best regards,
Tiwei Bie
Hi Tiwei,
> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, January 25, 2017 11:17 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> > It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > rte_eth_dev_info_get need to get info from PMD.
> > Remove rte_eth_dev_info_get from PMD code and get the info directly.
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > ++++++++++++++++++---------------------
> > 1 file changed, 68 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 64ce55a..f14a68b 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -4401,17 +4401,17 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > int rar_entry;
> > uint8_t *new_mac = (uint8_t *)(mac_addr);
> > struct rte_eth_dev *dev;
> > - struct rte_eth_dev_info dev_info;
> > + struct rte_pci_device *pci_dev;
> >
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >
> > dev = &rte_eth_devices[port];
> > - rte_eth_dev_info_get(port, &dev_info);
> > + pci_dev = IXGBE_DEV_TO_PCI(dev);
> >
> > - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > + if (is_ixgbe_pmd(dev->data->drv_name))
> > return -ENOTSUP;
> >
>
> The return value of is_ixgbe_pmd() is not boolean (actually I think it should be
> based on its name). If we omit the comparison with 0, the code looks weird. It
> looks like it'll return -ENOTSUP if the port's driver is ixgbe PMD.
Yes, it’s weird. But what makes it weird is not the missing comparison but the function name.
Better changing it to ixgbe_pmd_check. How about it?
>
> Best regards,
> Tiwei Bie
On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
> Hi Tiwei,
>
> > -----Original Message-----
> > From: Bie, Tiwei
> > Sent: Wednesday, January 25, 2017 11:17 AM
> > To: Lu, Wenzhuo
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> >
> > On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> > > It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > > rte_eth_dev_info_get need to get info from PMD.
> > > Remove rte_eth_dev_info_get from PMD code and get the info directly.
> > >
> > > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > ---
> > > drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > > ++++++++++++++++++---------------------
> > > 1 file changed, 68 insertions(+), 76 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 64ce55a..f14a68b 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -4401,17 +4401,17 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > int rar_entry;
> > > uint8_t *new_mac = (uint8_t *)(mac_addr);
> > > struct rte_eth_dev *dev;
> > > - struct rte_eth_dev_info dev_info;
> > > + struct rte_pci_device *pci_dev;
> > >
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > >
> > > dev = &rte_eth_devices[port];
> > > - rte_eth_dev_info_get(port, &dev_info);
> > > + pci_dev = IXGBE_DEV_TO_PCI(dev);
> > >
> > > - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > > + if (is_ixgbe_pmd(dev->data->drv_name))
> > > return -ENOTSUP;
> > >
> >
> > The return value of is_ixgbe_pmd() is not boolean (actually I think it should be
> > based on its name). If we omit the comparison with 0, the code looks weird. It
> > looks like it'll return -ENOTSUP if the port's driver is ixgbe PMD.
>
> Yes, it’s weird. But what makes it weird is not the missing comparison but the function name.
> Better changing it to ixgbe_pmd_check. How about it?
>
Yeah, I also prefer to change the helper function itself. But I'm not
good at the naming. I'd like to hear others' opinion. :-)
Best regards,
Tiwei Bie
On 1/25/2017 5:24 AM, Tiwei Bie wrote:
> On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
>> Hi Tiwei,
>>
>>> -----Original Message-----
>>> From: Bie, Tiwei
>>> Sent: Wednesday, January 25, 2017 11:17 AM
>>> To: Lu, Wenzhuo
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>>
>>> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>> rte_eth_dev_info_get need to get info from PMD.
>>>> Remove rte_eth_dev_info_get from PMD code and get the info directly.
>>>>
>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>> ---
>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>> ++++++++++++++++++---------------------
>>>> 1 file changed, 68 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> index 64ce55a..f14a68b 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> @@ -4401,17 +4401,17 @@ static int
>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>> int rar_entry;
>>>> uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>> struct rte_eth_dev *dev;
>>>> - struct rte_eth_dev_info dev_info;
>>>> + struct rte_pci_device *pci_dev;
>>>>
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>
>>>> dev = &rte_eth_devices[port];
>>>> - rte_eth_dev_info_get(port, &dev_info);
>>>> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>
>>>> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>> + if (is_ixgbe_pmd(dev->data->drv_name))
>>>> return -ENOTSUP;
>>>>
>>>
>>> The return value of is_ixgbe_pmd() is not boolean (actually I think it should be
>>> based on its name). If we omit the comparison with 0, the code looks weird. It
>>> looks like it'll return -ENOTSUP if the port's driver is ixgbe PMD.
>>
>> Yes, it’s weird. But what makes it weird is not the missing comparison but the function name.
>> Better changing it to ixgbe_pmd_check. How about it?
>>
>
> Yeah, I also prefer to change the helper function itself. But I'm not
> good at the naming. I'd like to hear others' opinion. :-)
Agree that it looks wrong without 0 comparison.
Helper function is checking if the given port is an ixgbe port or not,
unfortunately you need to this for PMD specific APIs.
So What about is_device_supported(),
I agree it is better if it returns bool,
and I also think it is better if it gets the rte_eth_dev as input
parameter, validating port based on name is API internal knowledge.
Also instead of name comparison against fixed string, it can be
eth_dev->driver->pci_drv.name against driver->name. This makes function
more generic, and perhaps this helper function can be moved into ethdev
layer, later. For this function needs to get both eth_dev and rte_driver
as argument.
>
> Best regards,
> Tiwei Bie
>
Hi Wenzhuo,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> Sent: Wednesday, January 25, 2017 2:39 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
> It'not appropriate to call rte_eth_dev_info_get in PMD,
> as rte_eth_dev_info_get need to get info from PMD.
> Remove rte_eth_dev_info_get from PMD code and get the
> info directly.
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
> 1 file changed, 68 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 64ce55a..f14a68b 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> int rar_entry;
> uint8_t *new_mac = (uint8_t *)(mac_addr);
> struct rte_eth_dev *dev;
> - struct rte_eth_dev_info dev_info;
> + struct rte_pci_device *pci_dev;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>
> dev = &rte_eth_devices[port];
> - rte_eth_dev_info_get(port, &dev_info);
> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>
> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> + if (is_ixgbe_pmd(dev->data->drv_name))
> return -ENOTSUP;
I wonder why do we need now that it is really an ixgbe device all over the place?
Konstantin
On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> Hi Wenzhuo,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
>> Sent: Wednesday, January 25, 2017 2:39 AM
>> To: dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> It'not appropriate to call rte_eth_dev_info_get in PMD,
>> as rte_eth_dev_info_get need to get info from PMD.
>> Remove rte_eth_dev_info_get from PMD code and get the
>> info directly.
>>
>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> ---
>> drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
>> 1 file changed, 68 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 64ce55a..f14a68b 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>> int rar_entry;
>> uint8_t *new_mac = (uint8_t *)(mac_addr);
>> struct rte_eth_dev *dev;
>> - struct rte_eth_dev_info dev_info;
>> + struct rte_pci_device *pci_dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>
>> dev = &rte_eth_devices[port];
>> - rte_eth_dev_info_get(port, &dev_info);
>> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>>
>> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>> + if (is_ixgbe_pmd(dev->data->drv_name))
>> return -ENOTSUP;
>
> I wonder why do we need now that it is really an ixgbe device all over the place?
This device specific API, so it is missing merits of abstraction layer,
application can these APIs with any port_id, API should be protected for it.
> Konstantin
>
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, February 1, 2017 5:40 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > Hi Wenzhuo,
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> >> Sent: Wednesday, January 25, 2017 2:39 AM
> >> To: dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> >>
> >> It'not appropriate to call rte_eth_dev_info_get in PMD,
> >> as rte_eth_dev_info_get need to get info from PMD.
> >> Remove rte_eth_dev_info_get from PMD code and get the
> >> info directly.
> >>
> >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >> ---
> >> drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
> >> 1 file changed, 68 insertions(+), 76 deletions(-)
> >>
> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> index 64ce55a..f14a68b 100644
> >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >> int rar_entry;
> >> uint8_t *new_mac = (uint8_t *)(mac_addr);
> >> struct rte_eth_dev *dev;
> >> - struct rte_eth_dev_info dev_info;
> >> + struct rte_pci_device *pci_dev;
> >>
> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >>
> >> dev = &rte_eth_devices[port];
> >> - rte_eth_dev_info_get(port, &dev_info);
> >> + pci_dev = IXGBE_DEV_TO_PCI(dev);
> >>
> >> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> >> + if (is_ixgbe_pmd(dev->data->drv_name))
> >> return -ENOTSUP;
> >
> > I wonder why do we need now that it is really an ixgbe device all over the place?
>
> This device specific API, so it is missing merits of abstraction layer,
> application can these APIs with any port_id, API should be protected for it.
Ah ok, my bad - didn't realize from the patch that it affects only device specific API :)
Would It be too much hassle to move these functions into a separate file (rte_ixgbe_pmd.c or so)?
Konstantin
>
> > Konstantin
> >
On 2/1/2017 6:10 PM, Ananyev, Konstantin wrote:
>
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, February 1, 2017 5:40 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
>>> Hi Wenzhuo,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
>>>> Sent: Wednesday, January 25, 2017 2:39 AM
>>>> To: dev@dpdk.org
>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>>>
>>>> It'not appropriate to call rte_eth_dev_info_get in PMD,
>>>> as rte_eth_dev_info_get need to get info from PMD.
>>>> Remove rte_eth_dev_info_get from PMD code and get the
>>>> info directly.
>>>>
>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>> ---
>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 144 ++++++++++++++++++---------------------
>>>> 1 file changed, 68 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> index 64ce55a..f14a68b 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> @@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>> int rar_entry;
>>>> uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>> struct rte_eth_dev *dev;
>>>> - struct rte_eth_dev_info dev_info;
>>>> + struct rte_pci_device *pci_dev;
>>>>
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>
>>>> dev = &rte_eth_devices[port];
>>>> - rte_eth_dev_info_get(port, &dev_info);
>>>> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>
>>>> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>> + if (is_ixgbe_pmd(dev->data->drv_name))
>>>> return -ENOTSUP;
>>>
>>> I wonder why do we need now that it is really an ixgbe device all over the place?
>>
>> This device specific API, so it is missing merits of abstraction layer,
>> application can these APIs with any port_id, API should be protected for it.
>
> Ah ok, my bad - didn't realize from the patch that it affects only device specific API :)
> Would It be too much hassle to move these functions into a separate file (rte_ixgbe_pmd.c or so)?
Not sure about the effort it requires, but I second that.
> Konstantin
>
>>
>>> Konstantin
>>>
>
Hi Ferruh,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, January 30, 2017 8:16 PM
> To: Bie, Tiwei; Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
> On 1/25/2017 5:24 AM, Tiwei Bie wrote:
> > On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
> >> Hi Tiwei,
> >>
> >>> -----Original Message-----
> >>> From: Bie, Tiwei
> >>> Sent: Wednesday, January 25, 2017 11:17 AM
> >>> To: Lu, Wenzhuo
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> >>> rte_eth_dev_info_get
> >>>
> >>> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
> >>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> >>>> rte_eth_dev_info_get need to get info from PMD.
> >>>> Remove rte_eth_dev_info_get from PMD code and get the info directly.
> >>>>
> >>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>>> ---
> >>>> drivers/net/ixgbe/ixgbe_ethdev.c | 144
> >>>> ++++++++++++++++++---------------------
> >>>> 1 file changed, 68 insertions(+), 76 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> index 64ce55a..f14a68b 100644
> >>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>>> @@ -4401,17 +4401,17 @@ static int
> >>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> >>>> int rar_entry;
> >>>> uint8_t *new_mac = (uint8_t *)(mac_addr);
> >>>> struct rte_eth_dev *dev;
> >>>> - struct rte_eth_dev_info dev_info;
> >>>> + struct rte_pci_device *pci_dev;
> >>>>
> >>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >>>>
> >>>> dev = &rte_eth_devices[port];
> >>>> - rte_eth_dev_info_get(port, &dev_info);
> >>>> + pci_dev = IXGBE_DEV_TO_PCI(dev);
> >>>>
> >>>> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> >>>> + if (is_ixgbe_pmd(dev->data->drv_name))
> >>>> return -ENOTSUP;
> >>>>
> >>>
> >>> The return value of is_ixgbe_pmd() is not boolean (actually I think
> >>> it should be based on its name). If we omit the comparison with 0,
> >>> the code looks weird. It looks like it'll return -ENOTSUP if the port's driver
> is ixgbe PMD.
> >>
> >> Yes, it’s weird. But what makes it weird is not the missing comparison but
> the function name.
> >> Better changing it to ixgbe_pmd_check. How about it?
> >>
> >
> > Yeah, I also prefer to change the helper function itself. But I'm not
> > good at the naming. I'd like to hear others' opinion. :-)
>
> Agree that it looks wrong without 0 comparison.
>
> Helper function is checking if the given port is an ixgbe port or not,
> unfortunately you need to this for PMD specific APIs.
> So What about is_device_supported(),
>
> I agree it is better if it returns bool, and I also think it is better if it gets the
> rte_eth_dev as input parameter, validating port based on name is API internal
> knowledge.
>
> Also instead of name comparison against fixed string, it can be eth_dev-
> >driver->pci_drv.name against driver->name. This makes function more
Thanks for your suggestion. But I don’t get your point here.
For a specific device, should not the eth_dev->driver->pci_drv.name and the driver->name be the same?
> generic, and perhaps this helper function can be moved into ethdev layer,
> later. For this function needs to get both eth_dev and rte_driver as argument.
>
>
> >
> > Best regards,
> > Tiwei Bie
> >
Hi Konstantin,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Wednesday, February 1, 2017 6:10 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
>
>
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Wednesday, February 1, 2017 5:40 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > rte_eth_dev_info_get
> >
> > On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > > Hi Wenzhuo,
> > >
> > >> -----Original Message-----
> > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> > >> Sent: Wednesday, January 25, 2017 2:39 AM
> > >> To: dev@dpdk.org
> > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > >> rte_eth_dev_info_get
> > >>
> > >> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > >> rte_eth_dev_info_get need to get info from PMD.
> > >> Remove rte_eth_dev_info_get from PMD code and get the info
> > >> directly.
> > >>
> > >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > >> ---
> > >> drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > >> ++++++++++++++++++---------------------
> > >> 1 file changed, 68 insertions(+), 76 deletions(-)
> > >>
> > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> index 64ce55a..f14a68b 100644
> > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > >> @@ -4401,17 +4401,17 @@ static int
> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > >> int rar_entry;
> > >> uint8_t *new_mac = (uint8_t *)(mac_addr);
> > >> struct rte_eth_dev *dev;
> > >> - struct rte_eth_dev_info dev_info;
> > >> + struct rte_pci_device *pci_dev;
> > >>
> > >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > >>
> > >> dev = &rte_eth_devices[port];
> > >> - rte_eth_dev_info_get(port, &dev_info);
> > >> + pci_dev = IXGBE_DEV_TO_PCI(dev);
> > >>
> > >> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > >> + if (is_ixgbe_pmd(dev->data->drv_name))
> > >> return -ENOTSUP;
> > >
> > > I wonder why do we need now that it is really an ixgbe device all over the
> place?
> >
> > This device specific API, so it is missing merits of abstraction
> > layer, application can these APIs with any port_id, API should be protected
> for it.
>
> Ah ok, my bad - didn't realize from the patch that it affects only device
> specific API :) Would It be too much hassle to move these functions into a
> separate file (rte_ixgbe_pmd.c or so)?
> Konstantin
>
> >
> > > Konstantin
> > >
All the device specific API functions are prefixed with rte_pmd_ixgbe so I don't think a separate file is necessary.
Regards,
Bernard.
Hi Bernard,
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Friday, February 3, 2017 9:21 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
> Hi Konstantin,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > Konstantin
> > Sent: Wednesday, February 1, 2017 6:10 PM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
> >
> >
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh
> > > Sent: Wednesday, February 1, 2017 5:40 PM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > > On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > > > Hi Wenzhuo,
> > > >
> > > >> -----Original Message-----
> > > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
> > > >> Sent: Wednesday, January 25, 2017 2:39 AM
> > > >> To: dev@dpdk.org
> > > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > >> rte_eth_dev_info_get
> > > >>
> > > >> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > > >> rte_eth_dev_info_get need to get info from PMD.
> > > >> Remove rte_eth_dev_info_get from PMD code and get the info
> > > >> directly.
> > > >>
> > > >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > >> ---
> > > >> drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > > >> ++++++++++++++++++---------------------
> > > >> 1 file changed, 68 insertions(+), 76 deletions(-)
> > > >>
> > > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> index 64ce55a..f14a68b 100644
> > > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > >> @@ -4401,17 +4401,17 @@ static int
> > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > >> int rar_entry;
> > > >> uint8_t *new_mac = (uint8_t *)(mac_addr);
> > > >> struct rte_eth_dev *dev;
> > > >> - struct rte_eth_dev_info dev_info;
> > > >> + struct rte_pci_device *pci_dev;
> > > >>
> > > >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > > >>
> > > >> dev = &rte_eth_devices[port];
> > > >> - rte_eth_dev_info_get(port, &dev_info);
> > > >> + pci_dev = IXGBE_DEV_TO_PCI(dev);
> > > >>
> > > >> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > > >> + if (is_ixgbe_pmd(dev->data->drv_name))
> > > >> return -ENOTSUP;
> > > >
> > > > I wonder why do we need now that it is really an ixgbe device all over the
> > place?
> > >
> > > This device specific API, so it is missing merits of abstraction
> > > layer, application can these APIs with any port_id, API should be protected
> > for it.
> >
> > Ah ok, my bad - didn't realize from the patch that it affects only device
> > specific API :) Would It be too much hassle to move these functions into a
> > separate file (rte_ixgbe_pmd.c or so)?
> > Konstantin
> >
> > >
> > > > Konstantin
> > > >
> All the device specific API functions are prefixed with rte_pmd_ixgbe
That's ok.
> so I don't think a separate file is necessary.
So far I didn't say it is necessary.
Though I think it would be good to group these functions in a separate file
to help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller and cleaner.
Again would be easier to maintain things in future, when more folks will come up with some extensions for it.
That's why I am asking: would it be a lot of work to do?
It is probably worth doing it now, while we have this API relatively small.
Konstantin
Hi Konstantin,
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, February 3, 2017 9:50 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>
> Hi Bernard,
>
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Friday, February 3, 2017 9:21 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > rte_eth_dev_info_get
> >
> > Hi Konstantin,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> > > Konstantin
> > > Sent: Wednesday, February 1, 2017 6:10 PM
> > > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > rte_eth_dev_info_get
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yigit, Ferruh
> > > > Sent: Wednesday, February 1, 2017 5:40 PM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu,
> > > > Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > > rte_eth_dev_info_get
> > > >
> > > > On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
> > > > > Hi Wenzhuo,
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo
> Lu
> > > > >> Sent: Wednesday, January 25, 2017 2:39 AM
> > > > >> To: dev@dpdk.org
> > > > >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > > > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
> > > > >> rte_eth_dev_info_get
> > > > >>
> > > > >> It'not appropriate to call rte_eth_dev_info_get in PMD, as
> > > > >> rte_eth_dev_info_get need to get info from PMD.
> > > > >> Remove rte_eth_dev_info_get from PMD code and get the info
> > > > >> directly.
> > > > >>
> > > > >> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > > > >> ---
> > > > >> drivers/net/ixgbe/ixgbe_ethdev.c | 144
> > > > >> ++++++++++++++++++---------------------
> > > > >> 1 file changed, 68 insertions(+), 76 deletions(-)
> > > > >>
> > > > >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> index 64ce55a..f14a68b 100644
> > > > >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > >> @@ -4401,17 +4401,17 @@ static int
> > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
> > > > >> int rar_entry;
> > > > >> uint8_t *new_mac = (uint8_t *)(mac_addr);
> > > > >> struct rte_eth_dev *dev;
> > > > >> - struct rte_eth_dev_info dev_info;
> > > > >> + struct rte_pci_device *pci_dev;
> > > > >>
> > > > >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> > > > >>
> > > > >> dev = &rte_eth_devices[port];
> > > > >> - rte_eth_dev_info_get(port, &dev_info);
> > > > >> + pci_dev = IXGBE_DEV_TO_PCI(dev);
> > > > >>
> > > > >> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
> > > > >> + if (is_ixgbe_pmd(dev->data->drv_name))
> > > > >> return -ENOTSUP;
> > > > >
> > > > > I wonder why do we need now that it is really an ixgbe device
> > > > > all over the
> > > place?
> > > >
> > > > This device specific API, so it is missing merits of abstraction
> > > > layer, application can these APIs with any port_id, API should be
> > > > protected
> > > for it.
> > >
> > > Ah ok, my bad - didn't realize from the patch that it affects only
> > > device specific API :) Would It be too much hassle to move these
> > > functions into a separate file (rte_ixgbe_pmd.c or so)?
> > > Konstantin
> > >
> > > >
> > > > > Konstantin
> > > > >
> > All the device specific API functions are prefixed with rte_pmd_ixgbe
>
> That's ok.
>
>
> > so I don't think a separate file is necessary.
>
> So far I didn't say it is necessary.
> Though I think it would be good to group these functions in a separate file to
> help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller
> and cleaner.
> Again would be easier to maintain things in future, when more folks will
> come up with some extensions for it.
> That's why I am asking: would it be a lot of work to do?
> It is probably worth doing it now, while we have this API relatively small.
> Konstantin
>
I would need to investigate what is involved in moving the rte_pmd_ixgbe_* functions to a separate file.
They may be using some of the static functions and data in the ixgbe_ethdev.c file which could be a problem.
The rte_pmd_ixgbe_* functions are considered an "interim solution" until a "generic solution" is agreed.
It might be best to postpone this work until the "generic solution" is agreed.
Regards,
Bernard.
On 2/3/2017 6:50 AM, Lu, Wenzhuo wrote:
> Hi Ferruh,
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, January 30, 2017 8:16 PM
>> To: Bie, Tiwei; Lu, Wenzhuo
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> On 1/25/2017 5:24 AM, Tiwei Bie wrote:
>>> On Wed, Jan 25, 2017 at 01:13:32PM +0800, Lu, Wenzhuo wrote:
>>>> Hi Tiwei,
>>>>
>>>>> -----Original Message-----
>>>>> From: Bie, Tiwei
>>>>> Sent: Wednesday, January 25, 2017 11:17 AM
>>>>> To: Lu, Wenzhuo
>>>>> Cc: dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>> rte_eth_dev_info_get
>>>>>
>>>>> On Wed, Jan 25, 2017 at 10:39:22AM +0800, Wenzhuo Lu wrote:
>>>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>>>> rte_eth_dev_info_get need to get info from PMD.
>>>>>> Remove rte_eth_dev_info_get from PMD code and get the info directly.
>>>>>>
>>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>>>> ---
>>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>>>> ++++++++++++++++++---------------------
>>>>>> 1 file changed, 68 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> index 64ce55a..f14a68b 100644
>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>> @@ -4401,17 +4401,17 @@ static int
>>>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>>> int rar_entry;
>>>>>> uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>>> struct rte_eth_dev *dev;
>>>>>> - struct rte_eth_dev_info dev_info;
>>>>>> + struct rte_pci_device *pci_dev;
>>>>>>
>>>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>>>
>>>>>> dev = &rte_eth_devices[port];
>>>>>> - rte_eth_dev_info_get(port, &dev_info);
>>>>>> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>>>
>>>>>> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>>>> + if (is_ixgbe_pmd(dev->data->drv_name))
>>>>>> return -ENOTSUP;
>>>>>>
>>>>>
>>>>> The return value of is_ixgbe_pmd() is not boolean (actually I think
>>>>> it should be based on its name). If we omit the comparison with 0,
>>>>> the code looks weird. It looks like it'll return -ENOTSUP if the port's driver
>> is ixgbe PMD.
>>>>
>>>> Yes, it’s weird. But what makes it weird is not the missing comparison but
>> the function name.
>>>> Better changing it to ixgbe_pmd_check. How about it?
>>>>
>>>
>>> Yeah, I also prefer to change the helper function itself. But I'm not
>>> good at the naming. I'd like to hear others' opinion. :-)
>>
>> Agree that it looks wrong without 0 comparison.
>>
>> Helper function is checking if the given port is an ixgbe port or not,
>> unfortunately you need to this for PMD specific APIs.
>> So What about is_device_supported(),
>>
>> I agree it is better if it returns bool, and I also think it is better if it gets the
>> rte_eth_dev as input parameter, validating port based on name is API internal
>> knowledge.
>>
>> Also instead of name comparison against fixed string, it can be eth_dev-
>>> driver->pci_drv.name against driver->name. This makes function more
> Thanks for your suggestion. But I don’t get your point here.
> For a specific device, should not the eth_dev->driver->pci_drv.name and the driver->name be the same?
Yes they are same.
But there is a intention to unlink "eth_drv->pci_drv", to better support
non pci devices,
so instead of a PMD directly accessing name through this link, I believe
it is better to use rte_drier->name, which is more generic.
>
>
>> generic, and perhaps this helper function can be moved into ethdev layer,
>> later. For this function needs to get both eth_dev and rte_driver as argument.
>>
>>
>>>
>>> Best regards,
>>> Tiwei Bie
>>>
>
On 2/3/2017 10:02 AM, Iremonger, Bernard wrote:
> Hi Konstantin,
>
>> -----Original Message-----
>> From: Ananyev, Konstantin
>> Sent: Friday, February 3, 2017 9:50 AM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>> dev@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> Hi Bernard,
>>
>>> -----Original Message-----
>>> From: Iremonger, Bernard
>>> Sent: Friday, February 3, 2017 9:21 AM
>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
>>> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>> dev@dpdk.org
>>> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>> rte_eth_dev_info_get
>>>
>>> Hi Konstantin,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
>>>> Konstantin
>>>> Sent: Wednesday, February 1, 2017 6:10 PM
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>> rte_eth_dev_info_get
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Yigit, Ferruh
>>>>> Sent: Wednesday, February 1, 2017 5:40 PM
>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu,
>>>>> Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>> rte_eth_dev_info_get
>>>>>
>>>>> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
>>>>>> Hi Wenzhuo,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo
>> Lu
>>>>>>> Sent: Wednesday, January 25, 2017 2:39 AM
>>>>>>> To: dev@dpdk.org
>>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>>>>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>>>> rte_eth_dev_info_get
>>>>>>>
>>>>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>>>>> rte_eth_dev_info_get need to get info from PMD.
>>>>>>> Remove rte_eth_dev_info_get from PMD code and get the info
>>>>>>> directly.
>>>>>>>
>>>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>>>>> ---
>>>>>>> drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>>>>> ++++++++++++++++++---------------------
>>>>>>> 1 file changed, 68 insertions(+), 76 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> index 64ce55a..f14a68b 100644
>>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> @@ -4401,17 +4401,17 @@ static int
>>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>>>> int rar_entry;
>>>>>>> uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>>>> struct rte_eth_dev *dev;
>>>>>>> - struct rte_eth_dev_info dev_info;
>>>>>>> + struct rte_pci_device *pci_dev;
>>>>>>>
>>>>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>>>>
>>>>>>> dev = &rte_eth_devices[port];
>>>>>>> - rte_eth_dev_info_get(port, &dev_info);
>>>>>>> + pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>>>>
>>>>>>> - if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>>>>> + if (is_ixgbe_pmd(dev->data->drv_name))
>>>>>>> return -ENOTSUP;
>>>>>>
>>>>>> I wonder why do we need now that it is really an ixgbe device
>>>>>> all over the
>>>> place?
>>>>>
>>>>> This device specific API, so it is missing merits of abstraction
>>>>> layer, application can these APIs with any port_id, API should be
>>>>> protected
>>>> for it.
>>>>
>>>> Ah ok, my bad - didn't realize from the patch that it affects only
>>>> device specific API :) Would It be too much hassle to move these
>>>> functions into a separate file (rte_ixgbe_pmd.c or so)?
>>>> Konstantin
>>>>
>>>>>
>>>>>> Konstantin
>>>>>>
>>> All the device specific API functions are prefixed with rte_pmd_ixgbe
>>
>> That's ok.
>>
>>
>>> so I don't think a separate file is necessary.
>>
>> So far I didn't say it is necessary.
>> Though I think it would be good to group these functions in a separate file to
>> help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller
>> and cleaner.
>> Again would be easier to maintain things in future, when more folks will
>> come up with some extensions for it.
>> That's why I am asking: would it be a lot of work to do?
>> It is probably worth doing it now, while we have this API relatively small.
>> Konstantin
>>
> I would need to investigate what is involved in moving the rte_pmd_ixgbe_* functions to a separate file.
> They may be using some of the static functions and data in the ixgbe_ethdev.c file which could be a problem.
> The rte_pmd_ixgbe_* functions are considered an "interim solution" until a "generic solution" is agreed.
> It might be best to postpone this work until the "generic solution" is agreed.
Still I believe it is good idea to move these functions into a separate
file, but not for this release J.
For PMD specific APIs, even we keep using direct API call method or
benefit from abstraction layer, means of using ioctl perhaps, these APIs
will be implemented in the PMD. And logically grouping them is good idea.
Briefly, I don't think we need to wait until "generic solution" agreed
for this.
But, of course an investigation needs to be done on required effort, and
decide based on that.
Thanks,
ferruh
>
> Regards,
>
> Bernard.
>
@@ -4401,17 +4401,17 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
int rar_entry;
uint8_t *new_mac = (uint8_t *)(mac_addr);
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -4902,17 +4902,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
struct ixgbe_hw *hw;
struct ixgbe_mac_info *mac;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (on > 1)
@@ -4932,17 +4932,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
struct ixgbe_hw *hw;
struct ixgbe_mac_info *mac;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (on > 1)
@@ -4961,17 +4961,17 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
struct ixgbe_hw *hw;
uint32_t ctrl;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (vlan_id > ETHER_MAX_VLAN_ID)
@@ -4997,14 +4997,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
struct ixgbe_hw *hw;
uint32_t ctrl;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
if (on > 1)
@@ -5031,14 +5029,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
int i;
int num_queues = (int)(IXGBE_QDE_IDX_MASK >> IXGBE_QDE_IDX_SHIFT);
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
if (on > 1)
@@ -5061,18 +5057,18 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
struct ixgbe_hw *hw;
uint32_t reg_value;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
/* only support VF's 0 to 63 */
- if ((vf >= dev_info.max_vfs) || (vf > 63))
+ if ((vf >= pci_dev->max_vfs) || (vf > 63))
return -EINVAL;
if (on > 1)
@@ -5094,19 +5090,21 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
rte_pmd_ixgbe_set_vf_vlan_stripq(uint8_t port, uint16_t vf, uint8_t on)
{
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
+ struct ixgbe_hw *hw;
uint16_t queues_per_pool;
uint32_t q;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
+ hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (on > 1)
@@ -5122,8 +5120,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
* first 124 queues 0-123 will be allocated to VF's and only
* the last 4 queues 123-127 will be assigned to the PF.
*/
-
- queues_per_pool = dev_info.vmdq_queue_num / dev_info.max_vmdq_pools;
+ if (hw->mac.type == ixgbe_mac_82598EB)
+ queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+ ETH_16_POOLS;
+ else
+ queues_per_pool = (uint16_t)hw->mac.max_rx_queues /
+ ETH_64_POOLS;
for (q = 0; q < queues_per_pool; q++)
(*dev->dev_ops->vlan_strip_queue_set)(dev,
@@ -5136,19 +5138,19 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
{
int val = 0;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
struct ixgbe_hw *hw;
uint32_t vmolr;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (on > 1)
@@ -5181,7 +5183,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
rte_pmd_ixgbe_set_vf_rx(uint8_t port, uint16_t vf, uint8_t on)
{
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
uint32_t reg, addr;
uint32_t val;
const uint8_t bit1 = 0x1;
@@ -5190,12 +5192,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (on > 1)
@@ -5231,7 +5233,7 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
rte_pmd_ixgbe_set_vf_tx(uint8_t port, uint16_t vf, uint8_t on)
{
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
+ struct rte_pci_device *pci_dev;
uint32_t reg, addr;
uint32_t val;
const uint8_t bit1 = 0x1;
@@ -5241,12 +5243,12 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (on > 1)
@@ -5282,7 +5284,6 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
uint64_t vf_mask, uint8_t vlan_on)
{
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
int ret = 0;
uint16_t vf_idx;
struct ixgbe_hw *hw;
@@ -5290,9 +5291,8 @@ static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
if ((vlan > ETHER_MAX_VLAN_ID) || (vf_mask == 0))
@@ -5318,7 +5318,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
uint16_t tx_rate, uint64_t q_msk)
{
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
struct ixgbe_hw *hw;
struct ixgbe_vf_info *vfinfo;
struct rte_eth_link link;
@@ -5332,13 +5331,13 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
dev = &rte_eth_devices[port];
- rte_eth_dev_info_get(port, &dev_info);
+ pci_dev = IXGBE_DEV_TO_PCI(dev);
rte_eth_link_get_nowait(port, &link);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- if (vf >= dev_info.max_vfs)
+ if (vf >= pci_dev->max_vfs)
return -EINVAL;
if (tx_rate > link.link_speed)
@@ -5347,7 +5346,6 @@ int rte_pmd_ixgbe_set_vf_rate_limit(uint8_t port, uint16_t vf,
if (q_msk == 0)
return 0;
- pci_dev = IXGBE_DEV_TO_PCI(dev);
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
vfinfo = *(IXGBE_DEV_PRIVATE_TO_P_VFDATA(dev->data->dev_private));
nb_q_per_pool = RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool;
@@ -8227,16 +8225,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
{
struct ixgbe_hw *hw;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
uint32_t ctrl;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ dev = &rte_eth_devices[port];
+
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- dev = &rte_eth_devices[port];
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
/* Stop the data paths */
@@ -8311,16 +8308,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
{
struct ixgbe_hw *hw;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
uint32_t ctrl;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ dev = &rte_eth_devices[port];
+
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- dev = &rte_eth_devices[port];
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
/* Stop the data paths */
@@ -8376,16 +8372,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
{
struct ixgbe_hw *hw;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
uint32_t ctrl;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ dev = &rte_eth_devices[port];
+
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- dev = &rte_eth_devices[port];
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8402,16 +8397,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
{
struct ixgbe_hw *hw;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
uint32_t ctrl;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ dev = &rte_eth_devices[port];
+
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- dev = &rte_eth_devices[port];
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
ctrl = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
@@ -8430,16 +8424,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
{
struct ixgbe_hw *hw;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
uint32_t ctrl, i;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ dev = &rte_eth_devices[port];
+
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- dev = &rte_eth_devices[port];
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
if (idx != 0 && idx != 1)
@@ -8487,16 +8480,15 @@ int ixgbe_enable_sec_tx_path_generic(struct ixgbe_hw *hw)
{
struct ixgbe_hw *hw;
struct rte_eth_dev *dev;
- struct rte_eth_dev_info dev_info;
uint32_t ctrl, i;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
- rte_eth_dev_info_get(port, &dev_info);
- if (is_ixgbe_pmd(dev_info.driver_name) != 0)
+ dev = &rte_eth_devices[port];
+
+ if (is_ixgbe_pmd(dev->data->drv_name))
return -ENOTSUP;
- dev = &rte_eth_devices[port];
hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
if (idx != 0 && idx != 1)