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