Re: [PATCH v3] dmaengine: idxd: fix wq cleanup of WQCFG registers

[Dave Jiang <dave.jiang@xxxxxxxxx>]

On 4/7/2021 12:52 PM, Dave Jiang wrote:
> 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@xxxxxxxxx>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> ---
> v3:
> - Remove unused vars
> v2:
> - Set IDXD_WQ_DISABLED for internal state after reset.

Forgot to set cc to dmaengine@vger



>   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)