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

[Dave Jiang <dave.jiang@xxxxxxxxx>]

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)