[PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Liming Sun]

The virtio framework uses wmb() when updating avail->idx. It
guarantees the write order, but not necessarily loading order
for the code accessing the memory. This commit adds a load barrier
after reading the avail->idx to make sure all the data in the
descriptor is visible. It also adds a barrier when returning the
packet to virtio framework to make sure read/writes are visible to
the virtio code.

Signed-off-by: Liming Sun <limings@nvidia.com>
---
 drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
index bbc4e71..38800e8 100644
--- a/drivers/platform/mellanox/mlxbf-tmfifo.c
+++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
@@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq, void *arg)
 	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
 		return NULL;
 
+	/* Make sure 'avail->idx' is visible already. */
+	virtio_rmb(false);
+
 	idx = vring->next_avail % vr->num;
 	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
 	if (WARN_ON(head >= vr->num))
@@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct mlxbf_tmfifo_vring *vring,
 	 * done or not. Add a memory barrier here to make sure the update above
 	 * completes before updating the idx.
 	 */
-	mb();
+	virtio_mb(false);
 	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);
 }
 
@@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct mlxbf_tmfifo_vring *vring,
 		desc = NULL;
 		fifo->vring[is_rx] = NULL;
 
+		/*
+		 * Make sure the load/store are in order before
+		 * returning back to virtio.
+		 */
+		virtio_mb(false);
+
 		/* Notify upper layer that packet is done. */
 		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
 		vring_interrupt(0, vring->vq);
-- 
1.8.3.1

RE: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Vadim Pasternak]

> -----Original Message-----
> From: Liming Sun <limings@nvidia.com>
> Sent: Friday, May 7, 2021 6:19 PM
> To: Andy Shevchenko <andy@infradead.org>; Darren Hart
> <dvhart@infradead.org>; Vadim Pasternak <vadimp@nvidia.com>
> Cc: Liming Sun <limings@nvidia.com>; linux-kernel@vger.kernel.org;
> platform-driver-x86@vger.kernel.org
> Subject: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory
> barrier issue
> 
> The virtio framework uses wmb() when updating avail->idx. It guarantees
> the write order, but not necessarily loading order for the code accessing the
> memory. This commit adds a load barrier after reading the avail->idx to make
> sure all the data in the descriptor is visible. It also adds a barrier when
> returning the packet to virtio framework to make sure read/writes are visible
> to the virtio code.

I suppose it should be sent as Bugfix?

> 
> Signed-off-by: Liming Sun <limings@nvidia.com>
> ---
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
> b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index bbc4e71..38800e8 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq,
> void *arg)
>  	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
>  		return NULL;
> 
> +	/* Make sure 'avail->idx' is visible already. */
> +	virtio_rmb(false);
> +
>  	idx = vring->next_avail % vr->num;
>  	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
>  	if (WARN_ON(head >= vr->num))
> @@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct
> mlxbf_tmfifo_vring *vring,
>  	 * done or not. Add a memory barrier here to make sure the update
> above
>  	 * completes before updating the idx.
>  	 */
> -	mb();
> +	virtio_mb(false);
>  	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);  }
> 
> @@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct
> mlxbf_tmfifo_vring *vring,
>  		desc = NULL;
>  		fifo->vring[is_rx] = NULL;
> 
> +		/*
> +		 * Make sure the load/store are in order before
> +		 * returning back to virtio.
> +		 */
> +		virtio_mb(false);
> +
>  		/* Notify upper layer that packet is done. */
>  		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
>  		vring_interrupt(0, vring->vq);
> --
> 1.8.3.1

RE: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Liming Sun]

Thanks! 
Sure, I'll resent it as bugfix in v2.

> -----Original Message-----
> From: Vadim Pasternak <vadimp@nvidia.com>
> Sent: Friday, May 7, 2021 11:39 AM
> To: Liming Sun <limings@nvidia.com>; Andy Shevchenko
> <andy@infradead.org>; Darren Hart <dvhart@infradead.org>
> Cc: linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org
> Subject: RE: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory
> barrier issue
> 
> 
> 
> > -----Original Message-----
> > From: Liming Sun <limings@nvidia.com>
> > Sent: Friday, May 7, 2021 6:19 PM
> > To: Andy Shevchenko <andy@infradead.org>; Darren Hart
> > <dvhart@infradead.org>; Vadim Pasternak <vadimp@nvidia.com>
> > Cc: Liming Sun <limings@nvidia.com>; linux-kernel@vger.kernel.org;
> > platform-driver-x86@vger.kernel.org
> > Subject: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a memory
> > barrier issue
> >
> > The virtio framework uses wmb() when updating avail->idx. It guarantees
> > the write order, but not necessarily loading order for the code accessing
> the
> > memory. This commit adds a load barrier after reading the avail->idx to
> make
> > sure all the data in the descriptor is visible. It also adds a barrier when
> > returning the packet to virtio framework to make sure read/writes are
> visible
> > to the virtio code.
> 
> I suppose it should be sent as Bugfix?
> 
> >
> > Signed-off-by: Liming Sun <limings@nvidia.com>
> > ---
> >  drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
> > b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > index bbc4e71..38800e8 100644
> > --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> > +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> > @@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq,
> > void *arg)
> >  	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
> >  		return NULL;
> >
> > +	/* Make sure 'avail->idx' is visible already. */
> > +	virtio_rmb(false);
> > +
> >  	idx = vring->next_avail % vr->num;
> >  	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
> >  	if (WARN_ON(head >= vr->num))
> > @@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct
> > mlxbf_tmfifo_vring *vring,
> >  	 * done or not. Add a memory barrier here to make sure the update
> > above
> >  	 * completes before updating the idx.
> >  	 */
> > -	mb();
> > +	virtio_mb(false);
> >  	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);  }
> >
> > @@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct
> > mlxbf_tmfifo_vring *vring,
> >  		desc = NULL;
> >  		fifo->vring[is_rx] = NULL;
> >
> > +		/*
> > +		 * Make sure the load/store are in order before
> > +		 * returning back to virtio.
> > +		 */
> > +		virtio_mb(false);
> > +
> >  		/* Notify upper layer that packet is done. */
> >  		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
> >  		vring_interrupt(0, vring->vq);
> > --
> > 1.8.3.1

[PATCH v2] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Liming Sun]

The virtio framework uses wmb() when updating avail->idx. It
guarantees the write order, but not necessarily loading order
for the code accessing the memory. This commit adds a load barrier
after reading the avail->idx to make sure all the data in the
descriptor is visible. It also adds a barrier when returning the
packet to virtio framework to make sure read/writes are visible to
the virtio code.

Fixes: 1357dfd7261f ("platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc")
Signed-off-by: Liming Sun <limings@nvidia.com>
---
v1->v2:
  Updates for Vadim's comments:
  - Add the 'Fixes' field in the commit message.
v1: Initial version
---
 drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
index bbc4e71..38800e8 100644
--- a/drivers/platform/mellanox/mlxbf-tmfifo.c
+++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
@@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq, void *arg)
 	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
 		return NULL;
 
+	/* Make sure 'avail->idx' is visible already. */
+	virtio_rmb(false);
+
 	idx = vring->next_avail % vr->num;
 	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
 	if (WARN_ON(head >= vr->num))
@@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct mlxbf_tmfifo_vring *vring,
 	 * done or not. Add a memory barrier here to make sure the update above
 	 * completes before updating the idx.
 	 */
-	mb();
+	virtio_mb(false);
 	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);
 }
 
@@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct mlxbf_tmfifo_vring *vring,
 		desc = NULL;
 		fifo->vring[is_rx] = NULL;
 
+		/*
+		 * Make sure the load/store are in order before
+		 * returning back to virtio.
+		 */
+		virtio_mb(false);
+
 		/* Notify upper layer that packet is done. */
 		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
 		vring_interrupt(0, vring->vq);
-- 
1.8.3.1

Re: [PATCH v2] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Hans de Goede]

Hi,

On 5/8/21 2:30 AM, Liming Sun wrote:
> The virtio framework uses wmb() when updating avail->idx. It
> guarantees the write order, but not necessarily loading order
> for the code accessing the memory. This commit adds a load barrier
> after reading the avail->idx to make sure all the data in the
> descriptor is visible. It also adds a barrier when returning the
> packet to virtio framework to make sure read/writes are visible to
> the virtio code.
> 
> Fixes: 1357dfd7261f ("platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc")
> Signed-off-by: Liming Sun <limings@nvidia.com>

I'm not familiar enough with this / the virtio code to be able to
judge if this makes sense (I assume it does).

Can I get an Ack or Reviewed-by from one of the other Mellanox
folks please?

Regards,

Hans



> ---
> v1->v2:
>   Updates for Vadim's comments:
>   - Add the 'Fixes' field in the commit message.
> v1: Initial version
> ---
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index bbc4e71..38800e8 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq, void *arg)
>  	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
>  		return NULL;
>  
> +	/* Make sure 'avail->idx' is visible already. */
> +	virtio_rmb(false);
> +
>  	idx = vring->next_avail % vr->num;
>  	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
>  	if (WARN_ON(head >= vr->num))
> @@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct mlxbf_tmfifo_vring *vring,
>  	 * done or not. Add a memory barrier here to make sure the update above
>  	 * completes before updating the idx.
>  	 */
> -	mb();
> +	virtio_mb(false);
>  	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);
>  }
>  
> @@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct mlxbf_tmfifo_vring *vring,
>  		desc = NULL;
>  		fifo->vring[is_rx] = NULL;
>  
> +		/*
> +		 * Make sure the load/store are in order before
> +		 * returning back to virtio.
> +		 */
> +		virtio_mb(false);
> +
>  		/* Notify upper layer that packet is done. */
>  		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
>  		vring_interrupt(0, vring->vq);
> 

RE: [PATCH v2] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Vadim Pasternak]

> -----Original Message-----
> From: Liming Sun <limings@nvidia.com>
> Sent: Saturday, May 8, 2021 3:30 AM
> To: Andy Shevchenko <andy@infradead.org>; Darren Hart
> <dvhart@infradead.org>; Vadim Pasternak <vadimp@nvidia.com>
> Cc: Liming Sun <limings@nvidia.com>; linux-kernel@vger.kernel.org;
> platform-driver-x86@vger.kernel.org
> Subject: [PATCH v2] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier
> issue
> 
> The virtio framework uses wmb() when updating avail->idx. It guarantees
> the write order, but not necessarily loading order for the code accessing the
> memory. This commit adds a load barrier after reading the avail->idx to make
> sure all the data in the descriptor is visible. It also adds a barrier when
> returning the packet to virtio framework to make sure read/writes are visible
> to the virtio code.
> 
> Fixes: 1357dfd7261f ("platform/mellanox: Add TmFifo driver for Mellanox
> BlueField Soc")
> Signed-off-by: Liming Sun <limings@nvidia.com>

Reviewed-by: Vadim Pasternak <vadimp@nvidia.com>

Liming,
Please, next time send patch to upstream after getting official approval from
our internal review.
In this case notes v1 -> v2 and version"v2" will be useless.

Thanks,
Vadim.

> ---
> v1->v2:
>   Updates for Vadim's comments:
>   - Add the 'Fixes' field in the commit message.
> v1: Initial version
> ---
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c
> b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index bbc4e71..38800e8 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq,
> void *arg)
>  	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
>  		return NULL;
> 
> +	/* Make sure 'avail->idx' is visible already. */
> +	virtio_rmb(false);
> +
>  	idx = vring->next_avail % vr->num;
>  	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
>  	if (WARN_ON(head >= vr->num))
> @@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct
> mlxbf_tmfifo_vring *vring,
>  	 * done or not. Add a memory barrier here to make sure the update
> above
>  	 * completes before updating the idx.
>  	 */
> -	mb();
> +	virtio_mb(false);
>  	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);  }
> 
> @@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct
> mlxbf_tmfifo_vring *vring,
>  		desc = NULL;
>  		fifo->vring[is_rx] = NULL;
> 
> +		/*
> +		 * Make sure the load/store are in order before
> +		 * returning back to virtio.
> +		 */
> +		virtio_mb(false);
> +
>  		/* Notify upper layer that packet is done. */
>  		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
>  		vring_interrupt(0, vring->vq);
> --
> 1.8.3.1

Re: [PATCH v2] platform/mellanox: mlxbf-tmfifo: Fix a memory barrier issue

[Hans de Goede]

Hi,

On 5/8/21 2:30 AM, Liming Sun wrote:
> The virtio framework uses wmb() when updating avail->idx. It
> guarantees the write order, but not necessarily loading order
> for the code accessing the memory. This commit adds a load barrier
> after reading the avail->idx to make sure all the data in the
> descriptor is visible. It also adds a barrier when returning the
> packet to virtio framework to make sure read/writes are visible to
> the virtio code.
> 
> Fixes: 1357dfd7261f ("platform/mellanox: Add TmFifo driver for Mellanox BlueField Soc")
> Signed-off-by: Liming Sun <limings@nvidia.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

I will also include this in the next pdx86-fixes pull-req for 5.13.

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
> v1->v2:
>   Updates for Vadim's comments:
>   - Add the 'Fixes' field in the commit message.
> v1: Initial version
> ---
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index bbc4e71..38800e8 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -294,6 +294,9 @@ static irqreturn_t mlxbf_tmfifo_irq_handler(int irq, void *arg)
>  	if (vring->next_avail == virtio16_to_cpu(vdev, vr->avail->idx))
>  		return NULL;
>  
> +	/* Make sure 'avail->idx' is visible already. */
> +	virtio_rmb(false);
> +
>  	idx = vring->next_avail % vr->num;
>  	head = virtio16_to_cpu(vdev, vr->avail->ring[idx]);
>  	if (WARN_ON(head >= vr->num))
> @@ -322,7 +325,7 @@ static void mlxbf_tmfifo_release_desc(struct mlxbf_tmfifo_vring *vring,
>  	 * done or not. Add a memory barrier here to make sure the update above
>  	 * completes before updating the idx.
>  	 */
> -	mb();
> +	virtio_mb(false);
>  	vr->used->idx = cpu_to_virtio16(vdev, vr_idx + 1);
>  }
>  
> @@ -733,6 +736,12 @@ static bool mlxbf_tmfifo_rxtx_one_desc(struct mlxbf_tmfifo_vring *vring,
>  		desc = NULL;
>  		fifo->vring[is_rx] = NULL;
>  
> +		/*
> +		 * Make sure the load/store are in order before
> +		 * returning back to virtio.
> +		 */
> +		virtio_mb(false);
> +
>  		/* Notify upper layer that packet is done. */
>  		spin_lock_irqsave(&fifo->spin_lock[is_rx], flags);
>  		vring_interrupt(0, vring->vq);
>