Re: [PATCH v2] dmaengine: idxd: Remove shadow Event Log head stored in idxd

[Dave Jiang]

On 2/14/24 7:49 PM, Fenghua Yu wrote:
> head is defined in idxd->evl as a shadow of head in the EVLSTATUS register.
> There are two issues related to the shadow head:
> 
> 1. Mismatch between the shadow head and the state of the EVLSTATUS
>    register:
>    If Event Log is supported, upon completion of the Enable Device command,
>    the Event Log head in the variable idxd->evl->head should be cleared to
>    match the state of the EVLSTATUS register. But the variable is not reset
>    currently, leading mismatch between the variable and the register state.
>    The mismatch causes incorrect processing of Event Log entries.
> 
> 2. Unnecessary shadow head definition:
>    The shadow head is unnecessary as head can be read directly from the
>    EVLSTATUS register. Reading head from the register incurs no additional
>    cost because event log head and tail are always read together and
>    tail is already read directly from the register as required by hardware.
> 
> Remove the shadow Event Log head stored in idxd->evl to address the
> mentioned issues.
> 
> Fixes: 244da66cda35 ("dmaengine: idxd: setup event log configuration")
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>

Much better fix.

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> ---
> Change Log:
> - A previous patch tries to fix this issue in a different way:
> https://lore.kernel.org/lkml/20240209191851.1050501-1-fenghua.yu@xxxxxxxxx/
>   After discussion with Dave Jiang, removing shadow head might be
>   a right fix.
> 
>  drivers/dma/idxd/cdev.c    | 2 +-
>  drivers/dma/idxd/debugfs.c | 2 +-
>  drivers/dma/idxd/idxd.h    | 1 -
>  drivers/dma/idxd/irq.c     | 3 +--
>  4 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> index 77f8885cf407..e5a94a93a3cc 100644
> --- a/drivers/dma/idxd/cdev.c
> +++ b/drivers/dma/idxd/cdev.c
> @@ -345,7 +345,7 @@ static void idxd_cdev_evl_drain_pasid(struct idxd_wq *wq, u32 pasid)
>  	spin_lock(&evl->lock);
>  	status.bits = ioread64(idxd->reg_base + IDXD_EVLSTATUS_OFFSET);
>  	t = status.tail;
> -	h = evl->head;
> +	h = status.head;
>  	size = evl->size;
>  
>  	while (h != t) {
> diff --git a/drivers/dma/idxd/debugfs.c b/drivers/dma/idxd/debugfs.c
> index 9cfbd9b14c4c..f3f25ee676f3 100644
> --- a/drivers/dma/idxd/debugfs.c
> +++ b/drivers/dma/idxd/debugfs.c
> @@ -68,9 +68,9 @@ static int debugfs_evl_show(struct seq_file *s, void *d)
>  
>  	spin_lock(&evl->lock);
>  
> -	h = evl->head;
>  	evl_status.bits = ioread64(idxd->reg_base + IDXD_EVLSTATUS_OFFSET);
>  	t = evl_status.tail;
> +	h = evl_status.head;
>  	evl_size = evl->size;
>  
>  	seq_printf(s, "Event Log head %u tail %u interrupt pending %u\n\n",
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index 47de3f93ff1e..d0f5db6cf1ed 100644
> --- a/drivers/dma/idxd/idxd.h
> +++ b/drivers/dma/idxd/idxd.h
> @@ -300,7 +300,6 @@ struct idxd_evl {
>  	unsigned int log_size;
>  	/* The number of entries in the event log. */
>  	u16 size;
> -	u16 head;
>  	unsigned long *bmap;
>  	bool batch_fail[IDXD_MAX_BATCH_IDENT];
>  };
> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
> index c8a0aa874b11..348aa21389a9 100644
> --- a/drivers/dma/idxd/irq.c
> +++ b/drivers/dma/idxd/irq.c
> @@ -367,9 +367,9 @@ static void process_evl_entries(struct idxd_device *idxd)
>  	/* Clear interrupt pending bit */
>  	iowrite32(evl_status.bits_upper32,
>  		  idxd->reg_base + IDXD_EVLSTATUS_OFFSET + sizeof(u32));
> -	h = evl->head;
>  	evl_status.bits = ioread64(idxd->reg_base + IDXD_EVLSTATUS_OFFSET);
>  	t = evl_status.tail;
> +	h = evl_status.head;
>  	size = idxd->evl->size;
>  
>  	while (h != t) {
> @@ -378,7 +378,6 @@ static void process_evl_entries(struct idxd_device *idxd)
>  		h = (h + 1) % size;
>  	}
>  
> -	evl->head = h;
>  	evl_status.head = h;
>  	iowrite32(evl_status.bits_lower32, idxd->reg_base + IDXD_EVLSTATUS_OFFSET);
>  	spin_unlock(&evl->lock);