[PATCH v2] dmaengine: idxd: force wq context cleanup on device disable path

[PATCH v2] dmaengine: idxd: force wq context cleanup on device disable path

[Fenghua Yu]

From: Dave Jiang <dave.jiang@intel.com>

Testing shown that when a wq mode is setup to be dedicated and then torn
down and reconfigured to shared, the wq configured end up being dedicated
anyays. The root cause is when idxd_device_wqs_clear_state() gets called
during idxd_driver removal, idxd_wq_disable_cleanup() does not get called
vs when the wq driver is removed first. The check of wq state being
"enabled" causes the cleanup to be bypassed. However, idxd_driver->remove()
releases all wq drivers. So the wqs goes to "disabled" state and will never
be "enabled". By that point, the driver has no idea if the wq was
previously configured or clean. So force call idxd_wq_disable_cleanup() on
all wqs always to make sure everything gets cleaned up.

Reported-by: Tony Zhu <tony.zhu@intel.com>
Tested-by: Tony Zhu <tony.zhu@intel.com>
Fixes: 0dcfe41e9a4c ("dmanegine: idxd: cleanup all device related bits after disabling device")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
Change Log:
v2:
- Re-based to 5.19-rc2 so that it can be applied cleanly. No functionality
  change.

v1:
https://patchwork.kernel.org/project/linux-dmaengine/patch/165090959239.1376825.18183942742142655091.stgit@djiang5-desk3.ch.intel.com/

 drivers/dma/idxd/device.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index ff0ea60051f0..5a8cc52c1abf 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -716,10 +716,7 @@ static void idxd_device_wqs_clear_state(struct idxd_device *idxd)
 		struct idxd_wq *wq = idxd->wqs[i];
 
 		mutex_lock(&wq->wq_lock);
-		if (wq->state == IDXD_WQ_ENABLED) {
-			idxd_wq_disable_cleanup(wq);
-			wq->state = IDXD_WQ_DISABLED;
-		}
+		idxd_wq_disable_cleanup(wq);
 		idxd_wq_device_reset_cleanup(wq);
 		mutex_unlock(&wq->wq_lock);
 	}
-- 
2.32.0

Re: [PATCH v2] dmaengine: idxd: force wq context cleanup on device disable path

[Fenghua Yu]

On Wed, Jun 15, 2022 at 04:42:19PM -0700, Fenghua Yu wrote:
> From: Dave Jiang <dave.jiang@intel.com>
> 
> Testing shown that when a wq mode is setup to be dedicated and then torn
> down and reconfigured to shared, the wq configured end up being dedicated
> anyays. The root cause is when idxd_device_wqs_clear_state() gets called
> during idxd_driver removal, idxd_wq_disable_cleanup() does not get called
> vs when the wq driver is removed first. The check of wq state being
> "enabled" causes the cleanup to be bypassed. However, idxd_driver->remove()
> releases all wq drivers. So the wqs goes to "disabled" state and will never
> be "enabled". By that point, the driver has no idea if the wq was
> previously configured or clean. So force call idxd_wq_disable_cleanup() on
> all wqs always to make sure everything gets cleaned up.
> 
> Reported-by: Tony Zhu <tony.zhu@intel.com>
> Tested-by: Tony Zhu <tony.zhu@intel.com>
> Fixes: 0dcfe41e9a4c ("dmanegine: idxd: cleanup all device related bits after disabling device")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

> ---
> Change Log:
> v2:
> - Re-based to 5.19-rc2 so that it can be applied cleanly. No functionality
>   change.
> 
> v1:
> https://patchwork.kernel.org/project/linux-dmaengine/patch/165090959239.1376825.18183942742142655091.stgit@djiang5-desk3.ch.intel.com/
> 
>  drivers/dma/idxd/device.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index ff0ea60051f0..5a8cc52c1abf 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -716,10 +716,7 @@ static void idxd_device_wqs_clear_state(struct idxd_device *idxd)
>  		struct idxd_wq *wq = idxd->wqs[i];
>  
>  		mutex_lock(&wq->wq_lock);
> -		if (wq->state == IDXD_WQ_ENABLED) {
> -			idxd_wq_disable_cleanup(wq);
> -			wq->state = IDXD_WQ_DISABLED;
> -		}
> +		idxd_wq_disable_cleanup(wq);
>  		idxd_wq_device_reset_cleanup(wq);
>  		mutex_unlock(&wq->wq_lock);
>  	}
> -- 
> 2.32.0
> 

Re: [PATCH v2] dmaengine: idxd: force wq context cleanup on device disable path

[Vinod Koul]

On 15-06-22, 17:53, Fenghua Yu wrote:
> On Wed, Jun 15, 2022 at 04:42:19PM -0700, Fenghua Yu wrote:
> > From: Dave Jiang <dave.jiang@intel.com>
> > 
> > Testing shown that when a wq mode is setup to be dedicated and then torn
> > down and reconfigured to shared, the wq configured end up being dedicated
> > anyays. The root cause is when idxd_device_wqs_clear_state() gets called
> > during idxd_driver removal, idxd_wq_disable_cleanup() does not get called
> > vs when the wq driver is removed first. The check of wq state being
> > "enabled" causes the cleanup to be bypassed. However, idxd_driver->remove()
> > releases all wq drivers. So the wqs goes to "disabled" state and will never
> > be "enabled". By that point, the driver has no idea if the wq was
> > previously configured or clean. So force call idxd_wq_disable_cleanup() on
> > all wqs always to make sure everything gets cleaned up.
> > 
> > Reported-by: Tony Zhu <tony.zhu@intel.com>
> > Tested-by: Tony Zhu <tony.zhu@intel.com>
> > Fixes: 0dcfe41e9a4c ("dmanegine: idxd: cleanup all device related bits after disabling device")
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>

Pls send updated patchset

> 
> > ---
> > Change Log:
> > v2:
> > - Re-based to 5.19-rc2 so that it can be applied cleanly. No functionality
> >   change.
> > 
> > v1:
> > https://patchwork.kernel.org/project/linux-dmaengine/patch/165090959239.1376825.18183942742142655091.stgit@djiang5-desk3.ch.intel.com/
> > 
> >  drivers/dma/idxd/device.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> > index ff0ea60051f0..5a8cc52c1abf 100644
> > --- a/drivers/dma/idxd/device.c
> > +++ b/drivers/dma/idxd/device.c
> > @@ -716,10 +716,7 @@ static void idxd_device_wqs_clear_state(struct idxd_device *idxd)
> >  		struct idxd_wq *wq = idxd->wqs[i];
> >  
> >  		mutex_lock(&wq->wq_lock);
> > -		if (wq->state == IDXD_WQ_ENABLED) {
> > -			idxd_wq_disable_cleanup(wq);
> > -			wq->state = IDXD_WQ_DISABLED;
> > -		}
> > +		idxd_wq_disable_cleanup(wq);
> >  		idxd_wq_device_reset_cleanup(wq);
> >  		mutex_unlock(&wq->wq_lock);
> >  	}
> > -- 
> > 2.32.0
> > 

-- 
~Vinod