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

[Dave Jiang <dave.jiang@intel.com>]

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