[PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Wei Huang]

Use rte_dev_remove() to replace rte_rawdev_pmd_release()
in ifpga_rawdev_cleanup(), resources occupied by ifpga
raw devices such as threads can be released correctly.

Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")

Signed-off-by: Wei Huang <wei.huang@intel.com>
---
 drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index fdf3c23..88c38aa 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct rte_rawdev *dev, int port,
 void ifpga_rawdev_cleanup(void)
 {
 	struct ifpga_rawdev *dev;
+	struct rte_rawdev *rdev;
 	unsigned int i;
 
 	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
 		dev = &ifpga_rawdevices[i];
 		if (dev->rawdev) {
-			rte_rawdev_pmd_release(dev->rawdev);
+			rdev = dev->rawdev;
+			rte_dev_remove(rdev->device);
 			dev->rawdev = NULL;
 		}
 	}
-- 
1.8.3.1

RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Zhang, Tianfei]

> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Wednesday, January 26, 2022 11:30 AM
> To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Huang, Wei <wei.huang@intel.com>
> Subject: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such as
> threads can be released correctly.
> 
> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
>  drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index fdf3c23..88c38aa 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> rte_rawdev *dev, int port,  void ifpga_rawdev_cleanup(void)  {
>  	struct ifpga_rawdev *dev;
> +	struct rte_rawdev *rdev;
>  	unsigned int i;
> 
>  	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>  		dev = &ifpga_rawdevices[i];
>  		if (dev->rawdev) {
> -			rte_rawdev_pmd_release(dev->rawdev);
> +			rdev = dev->rawdev;
> +			rte_dev_remove(rdev->device);
>  			dev->rawdev = NULL;
>  		}
>  	}
> --

It looks good for me.

Acked-by: Tianfei Zhang <tianfei.zhang@intel.com>

RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Wednesday, January 26, 2022 11:30
> To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Huang, Wei <wei.huang@intel.com>
> Subject: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such as
> threads can be released correctly.
> 
> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
>  drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index fdf3c23..88c38aa 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> rte_rawdev *dev, int port,  void ifpga_rawdev_cleanup(void)  {
>  	struct ifpga_rawdev *dev;
> +	struct rte_rawdev *rdev;
>  	unsigned int i;
> 
>  	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>  		dev = &ifpga_rawdevices[i];
>  		if (dev->rawdev) {
> -			rte_rawdev_pmd_release(dev->rawdev);
> +			rdev = dev->rawdev;
> +			rte_dev_remove(rdev->device);
>  			dev->rawdev = NULL;
>  		}
>  	}
> --
> 1.8.3.1

Acked-by: Rosen Xu <rosen.xu@intel.com>

RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Zhang, Qi Z]

> -----Original Message-----
> From: Xu, Rosen <rosen.xu@intel.com>
> Sent: Wednesday, January 26, 2022 3:08 PM
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> Hi,
> 
> > -----Original Message-----
> > From: Huang, Wei <wei.huang@intel.com>
> > Sent: Wednesday, January 26, 2022 11:30
> > To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit,
> > Ferruh <ferruh.yigit@intel.com>; Huang, Wei <wei.huang@intel.com>
> > Subject: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> >
> > Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> > ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
> > as threads can be released correctly.
> >
> > Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > ---
> >  drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index fdf3c23..88c38aa 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> > rte_rawdev *dev, int port,  void ifpga_rawdev_cleanup(void)  {
> >  	struct ifpga_rawdev *dev;
> > +	struct rte_rawdev *rdev;
> >  	unsigned int i;
> >
> >  	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> >  		dev = &ifpga_rawdevices[i];
> >  		if (dev->rawdev) {
> > -			rte_rawdev_pmd_release(dev->rawdev);
> > +			rdev = dev->rawdev;
> > +			rte_dev_remove(rdev->device);
> >  			dev->rawdev = NULL;
> >  		}
> >  	}
> > --
> > 1.8.3.1
> 
> Acked-by: Rosen Xu <rosen.xu@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Ferruh Yigit]

On 1/26/2022 3:29 AM, Wei Huang wrote:
> Use rte_dev_remove() to replace rte_rawdev_pmd_release()
> in ifpga_rawdev_cleanup(), resources occupied by ifpga
> raw devices such as threads can be released correctly.
> 

As far as I understand you are fixing an issue that not all resources
are released, is this correct?
What are these not released resources?

And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the cleanup,
is it expected that some resources are not freed after this call, or
should we fix that API?
If the device remove API needs to be used, what is the point of
'rte_rawdev_pmd_release()' API?

cc'ed rawdev maintainers for comment.

> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
>   drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
> index fdf3c23..88c38aa 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct rte_rawdev *dev, int port,
>   void ifpga_rawdev_cleanup(void)
>   {
>   	struct ifpga_rawdev *dev;
> +	struct rte_rawdev *rdev;
>   	unsigned int i;
>   
>   	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>   		dev = &ifpga_rawdevices[i];
>   		if (dev->rawdev) {
> -			rte_rawdev_pmd_release(dev->rawdev);
> +			rdev = dev->rawdev;
> +			rte_dev_remove(rdev->device);
>   			dev->rawdev = NULL;
>   		}
>   	}

RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Huang, Wei]

Hi,

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Wednesday, January 26, 2022 21:25
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> On 1/26/2022 3:29 AM, Wei Huang wrote:
> > Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> > ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
> > as threads can be released correctly.
> >
> 
> As far as I understand you are fixing an issue that not all resources are
> released, is this correct?
> What are these not released resources?
> 
> And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the
> cleanup, is it expected that some resources are not freed after this call, or
> should we fix that API?
> If the device remove API needs to be used, what is the point of
> 'rte_rawdev_pmd_release()' API?
> 
> cc'ed rawdev maintainers for comment.

Yes, this patch is to release all the resources of ifpga_rawdev after testpmd exit, the not released resources are interrupt and thread.

rte_rawdev_pmd_release implemented in ifpga_rawdev only release memory allocated by ifpga driver, that's the expected behavior.

I think it's a simple and safe way to release resources completely by calling rte_dev_remove.

> 
> > Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > ---
> >   drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index fdf3c23..88c38aa 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> rte_rawdev *dev, int port,
> >   void ifpga_rawdev_cleanup(void)
> >   {
> >   	struct ifpga_rawdev *dev;
> > +	struct rte_rawdev *rdev;
> >   	unsigned int i;
> >
> >   	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> >   		dev = &ifpga_rawdevices[i];
> >   		if (dev->rawdev) {
> > -			rte_rawdev_pmd_release(dev->rawdev);
> > +			rdev = dev->rawdev;
> > +			rte_dev_remove(rdev->device);
> >   			dev->rawdev = NULL;
> >   		}
> >   	}

Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Ferruh Yigit]

On 1/27/2022 8:57 AM, Huang, Wei wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Wednesday, January 26, 2022 21:25
>> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
>> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
>> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
>> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
>> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
>>
>> On 1/26/2022 3:29 AM, Wei Huang wrote:
>>> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
>>> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
>>> as threads can be released correctly.
>>>
>>
>> As far as I understand you are fixing an issue that not all resources are
>> released, is this correct?
>> What are these not released resources?
>>
>> And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the
>> cleanup, is it expected that some resources are not freed after this call, or
>> should we fix that API?
>> If the device remove API needs to be used, what is the point of
>> 'rte_rawdev_pmd_release()' API?
>>
>> cc'ed rawdev maintainers for comment.
> 
> Yes, this patch is to release all the resources of ifpga_rawdev after testpmd exit, the not released resources are interrupt and thread.
> 
> rte_rawdev_pmd_release implemented in ifpga_rawdev only release memory allocated by ifpga driver, that's the expected behavior.
> 
> I think it's a simple and safe way to release resources completely by calling rte_dev_remove.
> 

If device hot remove is better option, why 'rte_rawdev_pmd_release()' API exists?

>>
>>> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
>>>
>>> Signed-off-by: Wei Huang <wei.huang@intel.com>
>>> ---
>>>    drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
>>> b/drivers/raw/ifpga/ifpga_rawdev.c
>>> index fdf3c23..88c38aa 100644
>>> --- a/drivers/raw/ifpga/ifpga_rawdev.c
>>> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
>>> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
>> rte_rawdev *dev, int port,
>>>    void ifpga_rawdev_cleanup(void)
>>>    {
>>>    	struct ifpga_rawdev *dev;
>>> +	struct rte_rawdev *rdev;
>>>    	unsigned int i;
>>>
>>>    	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>>>    		dev = &ifpga_rawdevices[i];
>>>    		if (dev->rawdev) {
>>> -			rte_rawdev_pmd_release(dev->rawdev);
>>> +			rdev = dev->rawdev;
>>> +			rte_dev_remove(rdev->device);
>>>    			dev->rawdev = NULL;
>>>    		}
>>>    	}
> 

RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function

[Huang, Wei]

Hi, 

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, January 27, 2022 20:34
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> On 1/27/2022 8:57 AM, Huang, Wei wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Sent: Wednesday, January 26, 2022 21:25
> >> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
> >> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
> >> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> >> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> >> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> >>
> >> On 1/26/2022 3:29 AM, Wei Huang wrote:
> >>> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> >>> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
> >>> as threads can be released correctly.
> >>>
> >>
> >> As far as I understand you are fixing an issue that not all resources
> >> are released, is this correct?
> >> What are these not released resources?
> >>
> >> And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the
> >> cleanup, is it expected that some resources are not freed after this
> >> call, or should we fix that API?
> >> If the device remove API needs to be used, what is the point of
> >> 'rte_rawdev_pmd_release()' API?
> >>
> >> cc'ed rawdev maintainers for comment.
> >
> > Yes, this patch is to release all the resources of ifpga_rawdev after testpmd
> exit, the not released resources are interrupt and thread.
> >
> > rte_rawdev_pmd_release implemented in ifpga_rawdev only release
> memory allocated by ifpga driver, that's the expected behavior.
> >
> > I think it's a simple and safe way to release resources completely by calling
> rte_dev_remove.
> >
> 
> If device hot remove is better option, why 'rte_rawdev_pmd_release()' API
> exists?

Agree, let me try to free all resources in rte_rawdev_pmd_release().

> 
> >>
> >>> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> >>>
> >>> Signed-off-by: Wei Huang <wei.huang@intel.com>
> >>> ---
> >>>    drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> >>> b/drivers/raw/ifpga/ifpga_rawdev.c
> >>> index fdf3c23..88c38aa 100644
> >>> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> >>> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> >>> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> >> rte_rawdev *dev, int port,
> >>>    void ifpga_rawdev_cleanup(void)
> >>>    {
> >>>    	struct ifpga_rawdev *dev;
> >>> +	struct rte_rawdev *rdev;
> >>>    	unsigned int i;
> >>>
> >>>    	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> >>>    		dev = &ifpga_rawdevices[i];
> >>>    		if (dev->rawdev) {
> >>> -			rte_rawdev_pmd_release(dev->rawdev);
> >>> +			rdev = dev->rawdev;
> >>> +			rte_dev_remove(rdev->device);
> >>>    			dev->rawdev = NULL;
> >>>    		}
> >>>    	}
> >