Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
On 4/4/2018 6:57 PM, De Lara Guarch, Pablo wrote: > > >> -Original Message- >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ferruh Yigit >> Sent: Tuesday, April 3, 2018 10:50 AM >> To: David Marchand ; santosh >> >> Cc: dev@dpdk.org; Shreyansh Jain ; Legacy, Allain >> (Wind River) ; Tomasz Duszynski >> ; Thomas Monjalon >> Subject: Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with >> generic dev >> >> On 4/3/2018 10:06 AM, David Marchand wrote: >>> On Mon, Apr 2, 2018 at 6:13 PM, santosh >>> wrote: >>>> On Friday 30 March 2018 08:59 PM, David Marchand wrote: >>>>> I can see we enforce the driver name by putting it after the call to >>>>> .dev_infos_get. >>>>> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n2399 >>>>> >>>>> octeontx pmd seems to try to do something about it: >>>>> http://dpdk.org/browse/dpdk/tree/drivers/net/octeontx/octeontx_ethde >>>>> v.c#n622 >>>>> >>>>> Not sure it does something, might be a thing to cleanup. >>>>> >>>>> >>>> In case, if your referring to driver_name update then indeed its a >>>> cleanup [1]. >>>> >>>> Otherwise, I don't see any issue with v4 Or may be /I /misunderstood >>>> your comment. >>> >>> I agree there is no fundamental issue. >>> >>> dev_info->device = dev->device; >>> >>> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); >>> (*dev->dev_ops->dev_infos_get)(dev, dev_info); >>> dev_info->driver_name = dev->device->driver->name; >>> >>> If somebody (I mean some pmd out there) has a usecase with >>> dev_info->device != dev->device, why not. >> >> Intentional let drivers update this variable although I don't also see any >> use case >> of it. >> >> This variable was set by PMDs before this patch, so I don't see any reason >> to be >> so strict here. >> >> If driver does anything ethdev will set dev_info->device for it, if it want >> to >> overwrite, for any reason, it will have the capability. > > Looks good to me. Will do the same for cryptodev and bbdev. > The only thing that I am missing here is an update in documentation, > adding the ABI Change in release notes. Right, I forget about it, will send a new version. Thanks, ferruh > > Apart from it: > > Acked-by: Pablo de Lara > >> >>> >>> Thomas ? >>> >>> >
Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
> -Original Message- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Ferruh Yigit > Sent: Tuesday, April 3, 2018 10:50 AM > To: David Marchand ; santosh > > Cc: dev@dpdk.org; Shreyansh Jain ; Legacy, Allain > (Wind River) ; Tomasz Duszynski > ; Thomas Monjalon > Subject: Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with > generic dev > > On 4/3/2018 10:06 AM, David Marchand wrote: > > On Mon, Apr 2, 2018 at 6:13 PM, santosh > > wrote: > >> On Friday 30 March 2018 08:59 PM, David Marchand wrote: > >>> I can see we enforce the driver name by putting it after the call to > >>> .dev_infos_get. > >>> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n2399 > >>> > >>> octeontx pmd seems to try to do something about it: > >>> http://dpdk.org/browse/dpdk/tree/drivers/net/octeontx/octeontx_ethde > >>> v.c#n622 > >>> > >>> Not sure it does something, might be a thing to cleanup. > >>> > >>> > >> In case, if your referring to driver_name update then indeed its a > >> cleanup [1]. > >> > >> Otherwise, I don't see any issue with v4 Or may be /I /misunderstood > >> your comment. > > > > I agree there is no fundamental issue. > > > > dev_info->device = dev->device; > > > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > > (*dev->dev_ops->dev_infos_get)(dev, dev_info); > > dev_info->driver_name = dev->device->driver->name; > > > > If somebody (I mean some pmd out there) has a usecase with > > dev_info->device != dev->device, why not. > > Intentional let drivers update this variable although I don't also see any > use case > of it. > > This variable was set by PMDs before this patch, so I don't see any reason to > be > so strict here. > > If driver does anything ethdev will set dev_info->device for it, if it want to > overwrite, for any reason, it will have the capability. Looks good to me. Will do the same for cryptodev and bbdev. The only thing that I am missing here is an update in documentation, adding the ABI Change in release notes. Apart from it: Acked-by: Pablo de Lara > > > > > Thomas ? > > > >
Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
On 4/3/2018 10:06 AM, David Marchand wrote: > On Mon, Apr 2, 2018 at 6:13 PM, santosh > wrote: >> On Friday 30 March 2018 08:59 PM, David Marchand wrote: >>> I can see we enforce the driver name by putting it after the call to >>> .dev_infos_get. >>> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n2399 >>> >>> octeontx pmd seems to try to do something about it: >>> http://dpdk.org/browse/dpdk/tree/drivers/net/octeontx/octeontx_ethdev.c#n622 >>> >>> Not sure it does something, might be a thing to cleanup. >>> >>> >> In case, if your referring to driver_name update then >> indeed its a cleanup [1]. >> >> Otherwise, I don't see any issue with v4 Or >> may be /I /misunderstood your comment. > > I agree there is no fundamental issue. > > dev_info->device = dev->device; > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > (*dev->dev_ops->dev_infos_get)(dev, dev_info); > dev_info->driver_name = dev->device->driver->name; > > If somebody (I mean some pmd out there) has a usecase with > dev_info->device != dev->device, why not. Intentional let drivers update this variable although I don't also see any use case of it. This variable was set by PMDs before this patch, so I don't see any reason to be so strict here. If driver does anything ethdev will set dev_info->device for it, if it want to overwrite, for any reason, it will have the capability. > > Thomas ? > >
Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
On Mon, Apr 2, 2018 at 6:13 PM, santosh wrote: > On Friday 30 March 2018 08:59 PM, David Marchand wrote: >> I can see we enforce the driver name by putting it after the call to >> .dev_infos_get. >> http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n2399 >> >> octeontx pmd seems to try to do something about it: >> http://dpdk.org/browse/dpdk/tree/drivers/net/octeontx/octeontx_ethdev.c#n622 >> >> Not sure it does something, might be a thing to cleanup. >> >> > In case, if your referring to driver_name update then > indeed its a cleanup [1]. > > Otherwise, I don't see any issue with v4 Or > may be /I /misunderstood your comment. I agree there is no fundamental issue. dev_info->device = dev->device; RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); (*dev->dev_ops->dev_infos_get)(dev, dev_info); dev_info->driver_name = dev->device->driver->name; If somebody (I mean some pmd out there) has a usecase with dev_info->device != dev->device, why not. Thomas ? -- David Marchand
Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
Hello David, On Friday 30 March 2018 08:59 PM, David Marchand wrote: > On Fri, Mar 30, 2018 at 5:17 PM, Ferruh Yigit wrote: >> Public struct rte_eth_dev_info has a "struct rte_pci_device" field in it >> although it is common for all ethdev in all buses. >> >> Replacing pci specific struct with generic device struct and updating >> places that are using pci device in a way to get this information from >> generic device. >> >> Signed-off-by: Ferruh Yigit > [snip] > >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c >> index 209796d46..68bdc3103 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -2421,6 +2421,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct >> rte_eth_dev_info *dev_info) >> memset(dev_info, 0, sizeof(struct rte_eth_dev_info)); >> dev_info->rx_desc_lim = lim; >> dev_info->tx_desc_lim = lim; >> + dev_info->device = dev->device; >> >> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); >> (*dev->dev_ops->dev_infos_get)(dev, dev_info); > Reviewed-by: David Marchand > > Just a little comment, do we want the pmd to be able to override this ? > > I can see we enforce the driver name by putting it after the call to > .dev_infos_get. > http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n2399 > > octeontx pmd seems to try to do something about it: > http://dpdk.org/browse/dpdk/tree/drivers/net/octeontx/octeontx_ethdev.c#n622 > > Not sure it does something, might be a thing to cleanup. > > In case, if your referring to driver_name update then indeed its a cleanup [1]. Otherwise, I don't see any issue with v4 Or may be /I /misunderstood your comment. Thanks. [1] http://dpdk.org/dev/patchwork/patch/36880/
Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
Hello Ferruh, On Fri, Mar 30, 2018 at 5:29 PM, David Marchand wrote: > On Fri, Mar 30, 2018 at 5:17 PM, Ferruh Yigit wrote: >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c >> index 209796d46..68bdc3103 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -2421,6 +2421,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct >> rte_eth_dev_info *dev_info) >> memset(dev_info, 0, sizeof(struct rte_eth_dev_info)); >> dev_info->rx_desc_lim = lim; >> dev_info->tx_desc_lim = lim; >> + dev_info->device = dev->device; >> >> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); >> (*dev->dev_ops->dev_infos_get)(dev, dev_info); > > Just a little comment, do we want the pmd to be able to override this ? I still don't see when the pmd would need to override the dev_info->device. The pmd is the one that populated dev->device in the first place. Why would it want to report something different in dev_infos ? Some comments from pmd maintainers, maybe ? -- David Marchand
Re: [dpdk-dev] [PATCH v4] ethdev: replace bus specific struct with generic dev
On Fri, Mar 30, 2018 at 5:17 PM, Ferruh Yigit wrote: > Public struct rte_eth_dev_info has a "struct rte_pci_device" field in it > although it is common for all ethdev in all buses. > > Replacing pci specific struct with generic device struct and updating > places that are using pci device in a way to get this information from > generic device. > > Signed-off-by: Ferruh Yigit [snip] > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index 209796d46..68bdc3103 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -2421,6 +2421,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct > rte_eth_dev_info *dev_info) > memset(dev_info, 0, sizeof(struct rte_eth_dev_info)); > dev_info->rx_desc_lim = lim; > dev_info->tx_desc_lim = lim; > + dev_info->device = dev->device; > > RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); > (*dev->dev_ops->dev_infos_get)(dev, dev_info); Reviewed-by: David Marchand Just a little comment, do we want the pmd to be able to override this ? I can see we enforce the driver name by putting it after the call to .dev_infos_get. http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n2399 octeontx pmd seems to try to do something about it: http://dpdk.org/browse/dpdk/tree/drivers/net/octeontx/octeontx_ethdev.c#n622 Not sure it does something, might be a thing to cleanup. -- David Marchand
