diff mbox series

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

Message ID 20240215024931.1739621-1-fenghua.yu@intel.com
State New
Headers
Series [v2] dmaengine: idxd: Remove shadow Event Log head stored in idxd |

Commit Message

Fenghua Yu Feb. 15, 2024, 2:49 a.m. UTC 
  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>
---
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(-)

Comments

Dave Jiang Feb. 15, 2024, 4:06 p.m. UTC | #1 
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);
Vinod Koul Feb. 16, 2024, 12:34 p.m. UTC | #2 
On Wed, 14 Feb 2024 18:49:31 -0800, 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.
> 
> [...]

Applied, thanks!

[1/1] dmaengine: idxd: Remove shadow Event Log head stored in idxd
      commit: ecec7c9f29a7114a3e23a14020b1149ea7dffb4f

Best regards,
diff mbox series

Patch

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