dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 repost] dmaengine: idxd: fix wq cleanup of WQCFG registers
@ 2021-04-08  3:51 Dave Jiang
  2021-04-12  7:38 ` Vinod Koul
  0 siblings, 1 reply; 3+ messages in thread
From: Dave Jiang @ 2021-04-08  3:51 UTC (permalink / raw)
  To: vkoul; +Cc: Shreenivaas Devarajan, dmaengine

A pre-release silicon erratum workaround where wq reset does not clear
WQCFG registers was leaked into upstream code. Use wq reset command
instead of blasting the MMIO region. This also address an issue where
we clobber registers in future devices.

Fixes: da32b28c95a7 ("dmaengine: idxd: cleanup workqueue config after disabling")
Reported-by: Shreenivaas Devarajan <shreenivaas.devarajan@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
Missed the list. This is a repost.

v3:
- Remove unused vars
v2:
- Set IDXD_WQ_DISABLED for internal state after reset.


 drivers/dma/idxd/device.c |   36 +++++++++++++++++++++++++-----------
 drivers/dma/idxd/idxd.h   |    1 +
 drivers/dma/idxd/sysfs.c  |    9 ++-------
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index b8939e7eccfb..eb313e0e0e9b 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -276,6 +276,23 @@ void idxd_wq_drain(struct idxd_wq *wq)
 	idxd_cmd_exec(idxd, IDXD_CMD_DRAIN_WQ, operand, NULL);
 }
 
+void idxd_wq_reset(struct idxd_wq *wq)
+{
+	struct idxd_device *idxd = wq->idxd;
+	struct device *dev = &idxd->pdev->dev;
+	u32 operand;
+
+	if (wq->state != IDXD_WQ_ENABLED) {
+		dev_dbg(dev, "WQ %d in wrong state: %d\n", wq->id, wq->state);
+		return;
+	}
+
+	dev_dbg(dev, "Resetting WQ %d\n", wq->id);
+	operand = BIT(wq->id % 16) | ((wq->id / 16) << 16);
+	idxd_cmd_exec(idxd, IDXD_CMD_RESET_WQ, operand, NULL);
+	wq->state = IDXD_WQ_DISABLED;
+}
+
 int idxd_wq_map_portal(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
@@ -357,8 +374,6 @@ int idxd_wq_disable_pasid(struct idxd_wq *wq)
 void idxd_wq_disable_cleanup(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
-	struct device *dev = &idxd->pdev->dev;
-	int i, wq_offset;
 
 	lockdep_assert_held(&idxd->dev_lock);
 	memset(wq->wqcfg, 0, idxd->wqcfg_size);
@@ -370,14 +385,6 @@ void idxd_wq_disable_cleanup(struct idxd_wq *wq)
 	wq->ats_dis = 0;
 	clear_bit(WQ_FLAG_DEDICATED, &wq->flags);
 	memset(wq->name, 0, WQ_NAME_SIZE);
-
-	for (i = 0; i < WQCFG_STRIDES(idxd); i++) {
-		wq_offset = WQCFG_OFFSET(idxd, wq->id, i);
-		iowrite32(0, idxd->reg_base + wq_offset);
-		dev_dbg(dev, "WQ[%d][%d][%#x]: %#x\n",
-			wq->id, i, wq_offset,
-			ioread32(idxd->reg_base + wq_offset));
-	}
 }
 
 /* Device control bits */
@@ -636,7 +643,14 @@ static int idxd_wq_config_write(struct idxd_wq *wq)
 	if (!wq->group)
 		return 0;
 
-	memset(wq->wqcfg, 0, idxd->wqcfg_size);
+	/*
+	 * Instead of memset the entire shadow copy of WQCFG, copy from the hardware after
+	 * wq reset. This will copy back the sticky values that are present on some devices.
+	 */
+	for (i = 0; i < WQCFG_STRIDES(idxd); i++) {
+		wq_offset = WQCFG_OFFSET(idxd, wq->id, i);
+		wq->wqcfg->bits[i] = ioread32(idxd->reg_base + wq_offset);
+	}
 
 	/* byte 0-3 */
 	wq->wqcfg->wq_size = wq->size;
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index eee94121991e..21aa6e2017c8 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -387,6 +387,7 @@ void idxd_wq_free_resources(struct idxd_wq *wq);
 int idxd_wq_enable(struct idxd_wq *wq);
 int idxd_wq_disable(struct idxd_wq *wq);
 void idxd_wq_drain(struct idxd_wq *wq);
+void idxd_wq_reset(struct idxd_wq *wq);
 int idxd_wq_map_portal(struct idxd_wq *wq);
 void idxd_wq_unmap_portal(struct idxd_wq *wq);
 void idxd_wq_disable_cleanup(struct idxd_wq *wq);
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index 6d38bf9034e6..0155c1b4f2ef 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -212,7 +212,6 @@ static void disable_wq(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
 	struct device *dev = &idxd->pdev->dev;
-	int rc;
 
 	mutex_lock(&wq->wq_lock);
 	dev_dbg(dev, "%s removing WQ %s\n", __func__, dev_name(&wq->conf_dev));
@@ -233,17 +232,13 @@ static void disable_wq(struct idxd_wq *wq)
 	idxd_wq_unmap_portal(wq);
 
 	idxd_wq_drain(wq);
-	rc = idxd_wq_disable(wq);
+	idxd_wq_reset(wq);
 
 	idxd_wq_free_resources(wq);
 	wq->client_count = 0;
 	mutex_unlock(&wq->wq_lock);
 
-	if (rc < 0)
-		dev_warn(dev, "Failed to disable %s: %d\n",
-			 dev_name(&wq->conf_dev), rc);
-	else
-		dev_info(dev, "wq %s disabled\n", dev_name(&wq->conf_dev));
+	dev_info(dev, "wq %s disabled\n", dev_name(&wq->conf_dev));
 }
 
 static int idxd_config_bus_remove(struct device *dev)



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v3 repost] dmaengine: idxd: fix wq cleanup of WQCFG registers
  2021-04-08  3:51 [PATCH v3 repost] dmaengine: idxd: fix wq cleanup of WQCFG registers Dave Jiang
@ 2021-04-12  7:38 ` Vinod Koul
  2021-04-12 16:00   ` Dave Jiang
  0 siblings, 1 reply; 3+ messages in thread
From: Vinod Koul @ 2021-04-12  7:38 UTC (permalink / raw)
  To: Dave Jiang; +Cc: Shreenivaas Devarajan, dmaengine

On 07-04-21, 20:51, Dave Jiang wrote:

> +void idxd_wq_reset(struct idxd_wq *wq)
> +{
> +	struct idxd_device *idxd = wq->idxd;
> +	struct device *dev = &idxd->pdev->dev;
> +	u32 operand;
> +
> +	if (wq->state != IDXD_WQ_ENABLED) {
> +		dev_dbg(dev, "WQ %d in wrong state: %d\n", wq->id, wq->state);

should this not be dev_err?

> +		return;
> +	}
> +
> +	dev_dbg(dev, "Resetting WQ %d\n", wq->id);

Noisy..?

> +	operand = BIT(wq->id % 16) | ((wq->id / 16) << 16);
> +	idxd_cmd_exec(idxd, IDXD_CMD_RESET_WQ, operand, NULL);
> +	wq->state = IDXD_WQ_DISABLED;
> +}
> +
>  int idxd_wq_map_portal(struct idxd_wq *wq)
>  {
>  	struct idxd_device *idxd = wq->idxd;
> @@ -357,8 +374,6 @@ int idxd_wq_disable_pasid(struct idxd_wq *wq)
>  void idxd_wq_disable_cleanup(struct idxd_wq *wq)
>  {
>  	struct idxd_device *idxd = wq->idxd;
> -	struct device *dev = &idxd->pdev->dev;
> -	int i, wq_offset;
>  
>  	lockdep_assert_held(&idxd->dev_lock);
>  	memset(wq->wqcfg, 0, idxd->wqcfg_size);
> @@ -370,14 +385,6 @@ void idxd_wq_disable_cleanup(struct idxd_wq *wq)
>  	wq->ats_dis = 0;
>  	clear_bit(WQ_FLAG_DEDICATED, &wq->flags);
>  	memset(wq->name, 0, WQ_NAME_SIZE);
> -
> -	for (i = 0; i < WQCFG_STRIDES(idxd); i++) {
> -		wq_offset = WQCFG_OFFSET(idxd, wq->id, i);
> -		iowrite32(0, idxd->reg_base + wq_offset);
> -		dev_dbg(dev, "WQ[%d][%d][%#x]: %#x\n",
> -			wq->id, i, wq_offset,
> -			ioread32(idxd->reg_base + wq_offset));
> -	}
>  }
>  
>  /* Device control bits */
> @@ -636,7 +643,14 @@ static int idxd_wq_config_write(struct idxd_wq *wq)
>  	if (!wq->group)
>  		return 0;
>  
> -	memset(wq->wqcfg, 0, idxd->wqcfg_size);
> +	/*
> +	 * Instead of memset the entire shadow copy of WQCFG, copy from the hardware after
> +	 * wq reset. This will copy back the sticky values that are present on some devices.
> +	 */
> +	for (i = 0; i < WQCFG_STRIDES(idxd); i++) {
> +		wq_offset = WQCFG_OFFSET(idxd, wq->id, i);
> +		wq->wqcfg->bits[i] = ioread32(idxd->reg_base + wq_offset);
> +	}
>  
>  	/* byte 0-3 */
>  	wq->wqcfg->wq_size = wq->size;
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index eee94121991e..21aa6e2017c8 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h
> @@ -387,6 +387,7 @@ void idxd_wq_free_resources(struct idxd_wq *wq);
>  int idxd_wq_enable(struct idxd_wq *wq);
>  int idxd_wq_disable(struct idxd_wq *wq);
>  void idxd_wq_drain(struct idxd_wq *wq);
> +void idxd_wq_reset(struct idxd_wq *wq);
>  int idxd_wq_map_portal(struct idxd_wq *wq);
>  void idxd_wq_unmap_portal(struct idxd_wq *wq);
>  void idxd_wq_disable_cleanup(struct idxd_wq *wq);
> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> index 6d38bf9034e6..0155c1b4f2ef 100644
> --- a/drivers/dma/idxd/sysfs.c
> +++ b/drivers/dma/idxd/sysfs.c
> @@ -212,7 +212,6 @@ static void disable_wq(struct idxd_wq *wq)
>  {
>  	struct idxd_device *idxd = wq->idxd;
>  	struct device *dev = &idxd->pdev->dev;
> -	int rc;
>  
>  	mutex_lock(&wq->wq_lock);
>  	dev_dbg(dev, "%s removing WQ %s\n", __func__, dev_name(&wq->conf_dev));
> @@ -233,17 +232,13 @@ static void disable_wq(struct idxd_wq *wq)
>  	idxd_wq_unmap_portal(wq);
>  
>  	idxd_wq_drain(wq);
> -	rc = idxd_wq_disable(wq);
> +	idxd_wq_reset(wq);
>  
>  	idxd_wq_free_resources(wq);
>  	wq->client_count = 0;
>  	mutex_unlock(&wq->wq_lock);
>  
> -	if (rc < 0)
> -		dev_warn(dev, "Failed to disable %s: %d\n",
> -			 dev_name(&wq->conf_dev), rc);
> -	else
> -		dev_info(dev, "wq %s disabled\n", dev_name(&wq->conf_dev));
> +	dev_info(dev, "wq %s disabled\n", dev_name(&wq->conf_dev));

noisy again

>  }
>  
>  static int idxd_config_bus_remove(struct device *dev)
> 

-- 
~Vinod

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v3 repost] dmaengine: idxd: fix wq cleanup of WQCFG registers
  2021-04-12  7:38 ` Vinod Koul
@ 2021-04-12 16:00   ` Dave Jiang
  0 siblings, 0 replies; 3+ messages in thread
From: Dave Jiang @ 2021-04-12 16:00 UTC (permalink / raw)
  To: Vinod Koul; +Cc: Shreenivaas Devarajan, dmaengine


On 4/12/2021 12:38 AM, Vinod Koul wrote:
> On 07-04-21, 20:51, Dave Jiang wrote:
>
>> +void idxd_wq_reset(struct idxd_wq *wq)
>> +{
>> +	struct idxd_device *idxd = wq->idxd;
>> +	struct device *dev = &idxd->pdev->dev;
>> +	u32 operand;
>> +
>> +	if (wq->state != IDXD_WQ_ENABLED) {
>> +		dev_dbg(dev, "WQ %d in wrong state: %d\n", wq->id, wq->state);
> should this not be dev_err?

This typically can happen if the WQ is already disabled. If we make it 
dev_err, it becomes noisy. Mostly it's harmless and should just be FYI 
for debug.


>
>> +		return;
>> +	}
>> +
>> +	dev_dbg(dev, "Resetting WQ %d\n", wq->id);
> Noisy..?

Will remove.


>> +	operand = BIT(wq->id % 16) | ((wq->id / 16) << 16);
>> +	idxd_cmd_exec(idxd, IDXD_CMD_RESET_WQ, operand, NULL);
>> +	wq->state = IDXD_WQ_DISABLED;
>> +}
>> +
>>   int idxd_wq_map_portal(struct idxd_wq *wq)
>>   {
>>   	struct idxd_device *idxd = wq->idxd;
>> @@ -357,8 +374,6 @@ int idxd_wq_disable_pasid(struct idxd_wq *wq)
>>   void idxd_wq_disable_cleanup(struct idxd_wq *wq)
>>   {
>>   	struct idxd_device *idxd = wq->idxd;
>> -	struct device *dev = &idxd->pdev->dev;
>> -	int i, wq_offset;
>>   
>>   	lockdep_assert_held(&idxd->dev_lock);
>>   	memset(wq->wqcfg, 0, idxd->wqcfg_size);
>> @@ -370,14 +385,6 @@ void idxd_wq_disable_cleanup(struct idxd_wq *wq)
>>   	wq->ats_dis = 0;
>>   	clear_bit(WQ_FLAG_DEDICATED, &wq->flags);
>>   	memset(wq->name, 0, WQ_NAME_SIZE);
>> -
>> -	for (i = 0; i < WQCFG_STRIDES(idxd); i++) {
>> -		wq_offset = WQCFG_OFFSET(idxd, wq->id, i);
>> -		iowrite32(0, idxd->reg_base + wq_offset);
>> -		dev_dbg(dev, "WQ[%d][%d][%#x]: %#x\n",
>> -			wq->id, i, wq_offset,
>> -			ioread32(idxd->reg_base + wq_offset));
>> -	}
>>   }
>>   
>>   /* Device control bits */
>> @@ -636,7 +643,14 @@ static int idxd_wq_config_write(struct idxd_wq *wq)
>>   	if (!wq->group)
>>   		return 0;
>>   
>> -	memset(wq->wqcfg, 0, idxd->wqcfg_size);
>> +	/*
>> +	 * Instead of memset the entire shadow copy of WQCFG, copy from the hardware after
>> +	 * wq reset. This will copy back the sticky values that are present on some devices.
>> +	 */
>> +	for (i = 0; i < WQCFG_STRIDES(idxd); i++) {
>> +		wq_offset = WQCFG_OFFSET(idxd, wq->id, i);
>> +		wq->wqcfg->bits[i] = ioread32(idxd->reg_base + wq_offset);
>> +	}
>>   
>>   	/* byte 0-3 */
>>   	wq->wqcfg->wq_size = wq->size;
>> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
>> index eee94121991e..21aa6e2017c8 100644
>> --- a/drivers/dma/idxd/idxd.h
>> +++ b/drivers/dma/idxd/idxd.h
>> @@ -387,6 +387,7 @@ void idxd_wq_free_resources(struct idxd_wq *wq);
>>   int idxd_wq_enable(struct idxd_wq *wq);
>>   int idxd_wq_disable(struct idxd_wq *wq);
>>   void idxd_wq_drain(struct idxd_wq *wq);
>> +void idxd_wq_reset(struct idxd_wq *wq);
>>   int idxd_wq_map_portal(struct idxd_wq *wq);
>>   void idxd_wq_unmap_portal(struct idxd_wq *wq);
>>   void idxd_wq_disable_cleanup(struct idxd_wq *wq);
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index 6d38bf9034e6..0155c1b4f2ef 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -212,7 +212,6 @@ static void disable_wq(struct idxd_wq *wq)
>>   {
>>   	struct idxd_device *idxd = wq->idxd;
>>   	struct device *dev = &idxd->pdev->dev;
>> -	int rc;
>>   
>>   	mutex_lock(&wq->wq_lock);
>>   	dev_dbg(dev, "%s removing WQ %s\n", __func__, dev_name(&wq->conf_dev));
>> @@ -233,17 +232,13 @@ static void disable_wq(struct idxd_wq *wq)
>>   	idxd_wq_unmap_portal(wq);
>>   
>>   	idxd_wq_drain(wq);
>> -	rc = idxd_wq_disable(wq);
>> +	idxd_wq_reset(wq);
>>   
>>   	idxd_wq_free_resources(wq);
>>   	wq->client_count = 0;
>>   	mutex_unlock(&wq->wq_lock);
>>   
>> -	if (rc < 0)
>> -		dev_warn(dev, "Failed to disable %s: %d\n",
>> -			 dev_name(&wq->conf_dev), rc);
>> -	else
>> -		dev_info(dev, "wq %s disabled\n", dev_name(&wq->conf_dev));
>> +	dev_info(dev, "wq %s disabled\n", dev_name(&wq->conf_dev));
> noisy again

This is helpful for users enabling/disabling the WQ. If we want to 
remove this, then we should probably do it in a different patch and 
clean up all the enabling and disabling emits.


>
>>   }
>>   
>>   static int idxd_config_bus_remove(struct device *dev)
>>

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-12 16:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08  3:51 [PATCH v3 repost] dmaengine: idxd: fix wq cleanup of WQCFG registers Dave Jiang
2021-04-12  7:38 ` Vinod Koul
2021-04-12 16:00   ` Dave Jiang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).