Received-SPF: pass (google.com: domain of linux-kernel+bounces-67284-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <57689db8-3793-484a-9c85-bdbcec1d36ae@intel.com>
Date: Thu, 15 Feb 2024 09:06:27 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] dmaengine: idxd: Remove shadow Event Log head stored
 in idxd
Content-Language: en-US
To: Fenghua Yu <fenghua.yu@intel.com>, Vinod Koul <vkoul@kernel.org>
Cc: dmaengine@vger.kernel.org, linux-kernel <linux-kernel@vger.kernel.org>
References: <20240215024931.1739621-1-fenghua.yu@intel.com>
From: Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20240215024931.1739621-1-fenghua.yu@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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@intel.com>

Much better fix.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> Change Log:
> - A previous patch tries to fix this issue in a different way:
> https://lore.kernel.org/lkml/20240209191851.1050501-1-fenghua.yu@intel.com/
>   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);