LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages
@ 2021-09-16 10:33 Cristian Marussi
  2021-09-16 10:33 ` [PATCH 2/3] firmware: arm_scmi: Simplify spinlocks in virtio transport Cristian Marussi
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Cristian Marussi @ 2021-09-16 10:33 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Tryshnivskyy, Michael S. Tsirkin

Be more verbose avoiding to use _once flavour of dev_info/_err/_notice.
Remove usage of __func_ to identify which vqueue is referred in some error
messages and explicitly name the TX/RX vqueue.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Apply on sudeep/for-next/scmi on top of:

440320fdecdb ("firmware: arm_scmi: Fix virtio transport Kconfig dependency")
---
 drivers/firmware/arm_scmi/virtio.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 224577f86928..9a758d294693 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -94,8 +94,8 @@ static int scmi_vio_feed_vq_rx(struct scmi_vio_channel *vioch,
 
 	rc = virtqueue_add_inbuf(vioch->vqueue, &sg_in, 1, msg, GFP_ATOMIC);
 	if (rc)
-		dev_err_once(vioch->cinfo->dev,
-			     "failed to add to virtqueue (%d)\n", rc);
+		dev_err(vioch->cinfo->dev,
+			"failed to add to RX virtqueue (%d)\n", rc);
 	else
 		virtqueue_kick(vioch->vqueue);
 
@@ -187,8 +187,8 @@ static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
 static int virtio_link_supplier(struct device *dev)
 {
 	if (!scmi_vdev) {
-		dev_notice_once(dev,
-				"Deferring probe after not finding a bound scmi-virtio device\n");
+		dev_notice(dev,
+			   "Deferring probe after not finding a bound scmi-virtio device\n");
 		return -EPROBE_DEFER;
 	}
 
@@ -328,9 +328,8 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
 	rc = virtqueue_add_sgs(vioch->vqueue, sgs, 1, 1, msg, GFP_ATOMIC);
 	if (rc) {
 		list_add(&msg->list, &vioch->free_list);
-		dev_err_once(vioch->cinfo->dev,
-			     "%s() failed to add to virtqueue (%d)\n", __func__,
-			     rc);
+		dev_err(vioch->cinfo->dev,
+			"failed to add to TX virtqueue (%d)\n", rc);
 	} else {
 		virtqueue_kick(vioch->vqueue);
 	}
@@ -418,10 +417,10 @@ static int scmi_vio_probe(struct virtio_device *vdev)
 			sz /= DESCRIPTORS_PER_TX_MSG;
 
 		if (sz > MSG_TOKEN_MAX) {
-			dev_info_once(dev,
-				      "%s virtqueue could hold %d messages. Only %ld allowed to be pending.\n",
-				      channels[i].is_rx ? "rx" : "tx",
-				      sz, MSG_TOKEN_MAX);
+			dev_info(dev,
+				 "%s virtqueue could hold %d messages. Only %ld allowed to be pending.\n",
+				 channels[i].is_rx ? "rx" : "tx",
+				 sz, MSG_TOKEN_MAX);
 			sz = MSG_TOKEN_MAX;
 		}
 		channels[i].max_msg = sz;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/3] firmware: arm_scmi: Simplify spinlocks in virtio transport
  2021-09-16 10:33 [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Cristian Marussi
@ 2021-09-16 10:33 ` Cristian Marussi
  2021-09-16 10:33 ` [PATCH 3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device Cristian Marussi
  2021-10-06 10:59 ` [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Sudeep Holla
  2 siblings, 0 replies; 5+ messages in thread
From: Cristian Marussi @ 2021-09-16 10:33 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Tryshnivskyy, Michael S. Tsirkin

Remove unneeded nested irqsave/irqrestore spinlocks.
Add also a few descriptive comments to explain better the system behaviour
at shutdown time.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/virtio.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 9a758d294693..633b5bc379a4 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -110,18 +110,16 @@ static void scmi_finalize_message(struct scmi_vio_channel *vioch,
 	if (vioch->is_rx) {
 		scmi_vio_feed_vq_rx(vioch, msg);
 	} else {
-		unsigned long flags;
-
-		spin_lock_irqsave(&vioch->lock, flags);
+		/* Here IRQs are assumed to be already disabled by the caller */
+		spin_lock(&vioch->lock);
 		list_add(&msg->list, &vioch->free_list);
-		spin_unlock_irqrestore(&vioch->lock, flags);
+		spin_unlock(&vioch->lock);
 	}
 }
 
 static void scmi_vio_complete_cb(struct virtqueue *vqueue)
 {
 	unsigned long ready_flags;
-	unsigned long flags;
 	unsigned int length;
 	struct scmi_vio_channel *vioch;
 	struct scmi_vio_msg *msg;
@@ -140,7 +138,8 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
 			goto unlock_ready_out;
 		}
 
-		spin_lock_irqsave(&vioch->lock, flags);
+		/* IRQs already disabled here no need to irqsave */
+		spin_lock(&vioch->lock);
 		if (cb_enabled) {
 			virtqueue_disable_cb(vqueue);
 			cb_enabled = false;
@@ -151,7 +150,7 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
 				goto unlock_out;
 			cb_enabled = true;
 		}
-		spin_unlock_irqrestore(&vioch->lock, flags);
+		spin_unlock(&vioch->lock);
 
 		if (msg) {
 			msg->rx_len = length;
@@ -161,11 +160,18 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
 			scmi_finalize_message(vioch, msg);
 		}
 
+		/*
+		 * Release ready_lock and re-enable IRQs between loop iterations
+		 * to allow virtio_chan_free() to possibly kick in and set the
+		 * flag vioch->ready to false even in between processing of
+		 * messages, so as to force outstanding messages to be ignored
+		 * when system is shutting down.
+		 */
 		spin_unlock_irqrestore(&vioch->ready_lock, ready_flags);
 	}
 
 unlock_out:
-	spin_unlock_irqrestore(&vioch->lock, flags);
+	spin_unlock(&vioch->lock);
 unlock_ready_out:
 	spin_unlock_irqrestore(&vioch->ready_lock, ready_flags);
 }
@@ -434,6 +440,13 @@ static int scmi_vio_probe(struct virtio_device *vdev)
 
 static void scmi_vio_remove(struct virtio_device *vdev)
 {
+	/*
+	 * Once we get here, virtio_chan_free() will have already been called by
+	 * the SCMI core for any existing channel and, as a consequence, all the
+	 * virtio channels will have been already marked NOT ready, causing any
+	 * outstanding message on any vqueue to be ignored by complete_cb: now
+	 * we can just stop processing buffers and destroy the vqueues.
+	 */
 	vdev->config->reset(vdev);
 	vdev->config->del_vqs(vdev);
 	scmi_vdev = NULL;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device
  2021-09-16 10:33 [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Cristian Marussi
  2021-09-16 10:33 ` [PATCH 2/3] firmware: arm_scmi: Simplify spinlocks in virtio transport Cristian Marussi
@ 2021-09-16 10:33 ` Cristian Marussi
  2021-09-20 11:09   ` Sudeep Holla
  2021-10-06 10:59 ` [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Sudeep Holla
  2 siblings, 1 reply; 5+ messages in thread
From: Cristian Marussi @ 2021-09-16 10:33 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel, virtualization, virtio-dev
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi, igor.skalkin, peter.hilber, alex.bennee,
	jean-philippe, mikhail.golubev, anton.yakovlev, Vasyl.Vavrychuk,
	Tryshnivskyy, Michael S. Tsirkin

Only one single SCMI Virtio device is currently supported by this driver
and it is referenced using a static global variable which is initialized
once for all during probing and nullified at virtio device removal.

Add proper SMP barriers to protect accesses to such device reference to
ensure that the initialzation state of such device is correctly observed by
all PEs at any time.

Return -EBUSY, instead of -EINVAL, and a descriptive error message if more
than one SCMI Virtio device is ever found and probed.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/virtio.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 633b5bc379a4..3671986ea056 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -389,8 +389,11 @@ static int scmi_vio_probe(struct virtio_device *vdev)
 	struct virtqueue *vqs[VIRTIO_SCMI_VQ_MAX_CNT];
 
 	/* Only one SCMI VirtiO device allowed */
-	if (scmi_vdev)
-		return -EINVAL;
+	if (scmi_vdev) {
+		dev_err(dev,
+			"One SCMI Virtio device was already initialized: only one allowed.\n");
+		return -EBUSY;
+	}
 
 	have_vq_rx = scmi_vio_have_vq_rx(vdev);
 	vq_cnt = have_vq_rx ? VIRTIO_SCMI_VQ_MAX_CNT : 1;
@@ -433,7 +436,8 @@ static int scmi_vio_probe(struct virtio_device *vdev)
 	}
 
 	vdev->priv = channels;
-	scmi_vdev = vdev;
+	/* Ensure initialized scmi_vdev is visible */
+	smp_store_mb(scmi_vdev, vdev);
 
 	return 0;
 }
@@ -449,7 +453,8 @@ static void scmi_vio_remove(struct virtio_device *vdev)
 	 */
 	vdev->config->reset(vdev);
 	vdev->config->del_vqs(vdev);
-	scmi_vdev = NULL;
+	/* Ensure scmi_vdev is visible as NULL */
+	smp_store_mb(scmi_vdev, NULL);
 }
 
 static int scmi_vio_validate(struct virtio_device *vdev)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device
  2021-09-16 10:33 ` [PATCH 3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device Cristian Marussi
@ 2021-09-20 11:09   ` Sudeep Holla
  0 siblings, 0 replies; 5+ messages in thread
From: Sudeep Holla @ 2021-09-20 11:09 UTC (permalink / raw)
  To: Cristian Marussi, Michael S. Tsirkin
  Cc: linux-kernel, linux-arm-kernel, virtualization, virtio-dev,
	james.quinlan, Jonathan.Cameron, f.fainelli, etienne.carriere,
	vincent.guittot, souvik.chakravarty, igor.skalkin, peter.hilber,
	alex.bennee, jean-philippe, mikhail.golubev, anton.yakovlev,
	Vasyl.Vavrychuk, Tryshnivskyy, Michael S. Tsirkin

Hi Michael,

On Thu, Sep 16, 2021 at 11:33:36AM +0100, Cristian Marussi wrote:
> Only one single SCMI Virtio device is currently supported by this driver
> and it is referenced using a static global variable which is initialized
> once for all during probing and nullified at virtio device removal.
> 
> Add proper SMP barriers to protect accesses to such device reference to
> ensure that the initialzation state of such device is correctly observed by
> all PEs at any time.
> 
> Return -EBUSY, instead of -EINVAL, and a descriptive error message if more
> than one SCMI Virtio device is ever found and probed.
> 

I was thinking of applying this patch and probably 2/3 as fix for v5.15.
Let me know if you have any objections.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages
  2021-09-16 10:33 [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Cristian Marussi
  2021-09-16 10:33 ` [PATCH 2/3] firmware: arm_scmi: Simplify spinlocks in virtio transport Cristian Marussi
  2021-09-16 10:33 ` [PATCH 3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device Cristian Marussi
@ 2021-10-06 10:59 ` Sudeep Holla
  2 siblings, 0 replies; 5+ messages in thread
From: Sudeep Holla @ 2021-10-06 10:59 UTC (permalink / raw)
  To: Cristian Marussi, virtio-dev, linux-kernel, virtualization,
	linux-arm-kernel
  Cc: Sudeep Holla, alex.bennee, jean-philippe, vincent.guittot,
	Jonathan.Cameron, anton.yakovlev, f.fainelli, mikhail.golubev,
	igor.skalkin, etienne.carriere, Tryshnivskyy, Vasyl.Vavrychuk,
	Michael S. Tsirkin, james.quinlan, peter.hilber,
	souvik.chakravarty

On Thu, 16 Sep 2021 11:33:34 +0100, Cristian Marussi wrote:
> Be more verbose avoiding to use _once flavour of dev_info/_err/_notice.
> Remove usage of __func_ to identify which vqueue is referred in some error
> messages and explicitly name the TX/RX vqueue.
> 
> 

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/3] firmware: arm_scmi: Review some virtio log messages
      https://git.kernel.org/sudeep.holla/c/0830e033c0

This one for v5.16

[2/3] firmware: arm_scmi: Simplify spinlocks in virtio transport
      https://git.kernel.org/sudeep.holla/c/a14a14595d
[3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device
      https://git.kernel.org/sudeep.holla/c/bf1acf809d

These 2 as fixes potentially for v5.15

--
Regards,
Sudeep


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-06 10:59 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 10:33 [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Cristian Marussi
2021-09-16 10:33 ` [PATCH 2/3] firmware: arm_scmi: Simplify spinlocks in virtio transport Cristian Marussi
2021-09-16 10:33 ` [PATCH 3/3] firmware: arm_scmi: Add proper barriers to scmi virtio device Cristian Marussi
2021-09-20 11:09   ` Sudeep Holla
2021-10-06 10:59 ` [PATCH 1/3] firmware: arm_scmi: Review some virtio log messages Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).