[RFC PATCH 00/10] Introduce SCMI transport atomic support

Hi all,

This RFC series mainly aims to introduce atomic support for transport that
can support it.

At first in [03/10], as a closely related addition, it is introduced a
common way  for a transport to signal to the SCMI core that it does not
offer completion interrupts, so that the usual polling behaviour based
on .poll_done() will be required: this can be done enabling statically
a global polling behaviour for the whole transport with flag
scmi_desc.force_polling OR dynamically enabling at runtime such polling
behaviour on a per-channel basis with scmi_chan_info.needs_polling,
typically during .chan_setup(). The usual per-command polling selection
behaviour based on hdr.poll_completion is preserved as before.

then in [04/10], a transport that supports atomic operation on its tx path
can now declare itself as .atomic_capable and as a consequence the SCMI
core will refrain itself from sleeping on the correspondent rx-path.

In [07/10] a simple method is introduced so that an SCMI driver can easily
query the core to check if the currently used transport is configured to
behave in an atomic manner: in this way, interested SCMI driver users, like
Clock framework [08/10], can optionally support atomic operations when
operating on an atomically configured transport.

Finally there are 2 *tentative" patch for SMC transport: at first [09/10]
ports SMC to use the common core completions when completion interrupt is
available or otherwise revert to use common core polling mechanism above
introduced; then in [10/10] SMC is converted to be .atomic_capable by
substituting the mutexes with busy-waiting to keep the channel 'locked'.

SMC changes have not been tested so far (I cannot), AND they are just a
proposal at this stage to try to better abstract and unify behaviour with
the SCMI core; both patches are completely intended as RFCs, though, not
only regarding their implementation but even their mere existence is RFC:
I mean maybe we just don't want to do such kind of unification/abstraction,
and I can just drop those SMC patches if unwanted; any feedback welcome.

Atomic support has been minimally tested against the upcoming virtio
transport draft(unposted) series, while polling has been tested both
with virtio and mailbox transports.

The series is based on sudeep/for-next [1] on top of commit:

commit 0aa69c9fc80d ("firmware: arm_scmi: Add compatibility checks for
		     shmem node")

Given the RFC status of the series in general I still not have CCed any
maintainer out of SCMI subsystem.

Any feedback welcome.

Thanks,

Cristian

---

[1]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi

Cristian Marussi (10):
  firmware: arm_scmi: Reset properly xfer SCMI status
  firmware: arm_scmi: Add missing xfer reinit_completion
  firmware: arm_scmi: Add configurable polling mode for transports
  firmware: arm_scmi: Add support for atomic transports
  include: trace: Add new scmi_xfer_response_wait event
  firmware: arm_scmi: Use new trace event scmi_xfer_response_wait
  firmware: arm_scmi: Add is_transport_atomic() handle method
  clk: scmi: Support atomic enable/disable API
  firmware: arm-scmi: Make smc transport use common completions
  firmware: arm-scmi: Make smc transport atomic

 drivers/clk/clk-scmi.c             |  44 +++++--
 drivers/firmware/arm_scmi/common.h |  11 ++
 drivers/firmware/arm_scmi/driver.c | 184 +++++++++++++++++++++++++----
 drivers/firmware/arm_scmi/smc.c    |  60 ++++++----
 include/linux/scmi_protocol.h      |   8 ++
 include/trace/events/scmi.h        |  28 +++++
 6 files changed, 277 insertions(+), 58 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

When an SCMI command transfer fails due to some protocol issue an SCMI
error code is reported inside the SCMI message payload itself and it is
then retrieved and transcribed by the specific transport layer into the
xfer.hdr.status field by transport specific .fetch_response().

The core SCMI transport layer never explicitly reset xfer.hdr.status,
so when an xfer is reused, if a transport misbehaved in handling such
status field, we risk to see an invalid ghost error code.

Reset xfer.hdr.status to SCMI_SUCCESS right before each transfer is
started.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index ca71568c5c41..bee33f9c2032 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -447,6 +447,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 			      xfer->hdr.protocol_id, xfer->hdr.seq,
 			      xfer->hdr.poll_completion);
 
+	xfer->hdr.status = SCMI_SUCCESS;
 	ret = info->desc->ops->send_message(cinfo, xfer);
 	if (ret < 0) {
 		dev_dbg(dev, "Failed to send message %d\n", ret);

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Reusing timed out xfers in a loop can lead to issue if completion was not
properly reinitialized.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index bee33f9c2032..759ae4a23e74 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 			      xfer->hdr.poll_completion);
 
 	xfer->hdr.status = SCMI_SUCCESS;
+	reinit_completion(&xfer->done);
 	ret = info->desc->ops->send_message(cinfo, xfer);
 	if (ret < 0) {
 		dev_dbg(dev, "Failed to send message %d\n", ret);

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

SCMI communications along TX channels can optionally be provided of a
completion interrupt; when such interrupt is not available, command
transactions should rely on polling, where the SCMI core takes care to
repeatedly evaluates the transport-specific .poll_done() function to
determine if a request was completed or timed out.

Such mechanism is already present and working on a single transfer base:
SCMI protocols can indeed enable hdr.poll_completion on specific commands
ahead of each transfer and cause that transaction to be handled with
polling.

Introduce a couple of flags to be able to enforce such polling behaviour
globally at will:

 - scmi_desc.force_polling: to statically switch the whole transport to
 			    polling mode.

 - scmi_chan_info.needs_polling: to switch a single channel dynamically to
 				 polling mode if at runtime is determined
				 that no completion interrupt was available
				 for such channel.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h | 10 ++++++++++
 drivers/firmware/arm_scmi/driver.c | 17 +++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 8685619d38f9..8f4e6ebfc0ef 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h

@@ -272,11 +272,18 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id);
  * @dev: Reference to device in the SCMI hierarchy corresponding to this
  *	 channel
  * @handle: Pointer to SCMI entity handle
+ * @needs_polling: Flag to indicate that this channel has no completion
+ *		   interrupt mechanism, so it needs SCMI core to poll, using
+ *		   .poll_done(), to determine when a command has completed.
+ *		   This can be dynamically set by transports at run-time inside
+ *		   their provided .chan_setup() when they determine no
+ *		   completion interrupt is available.
  * @transport_info: Transport layer related information
  */
 struct scmi_chan_info {
 	struct device *dev;
 	struct scmi_handle *handle;
+	bool needs_polling;
 	void *transport_info;
 };
 

@@ -322,12 +329,15 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  * @max_msg: Maximum number of messages that can be pending
  *	simultaneously in the system
  * @max_msg_size: Maximum size of data per message that can be handled.
+ * @force_polling: Flag to force this whole transport to use SCMI core polling
+ *		   mechanism instead of completion interrupts even if available.
  */
 struct scmi_desc {
 	const struct scmi_transport_ops *ops;
 	int max_rx_timeout_ms;
 	int max_msg;
 	int max_msg_size;
+	bool force_polling;
 };
 
 extern const struct scmi_desc scmi_mailbox_desc;

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 759ae4a23e74..c11ff49f6b62 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -319,6 +319,15 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
 	}
 
 	xfer = &minfo->xfer_block[xfer_id];
+
+	/* Discard unexpected messages when polling is active. */
+	if (msg_type != MSG_TYPE_DELAYED_RESP && xfer->hdr.poll_completion) {
+		WARN_ON_ONCE(1);
+		dev_dbg(dev,
+			"Completion IRQ received but using polling. Ignore.\n");
+		return;
+	}
+
 	/*
 	 * Even if a response was indeed expected on this slot at this point,
 	 * a buggy platform could wrongly reply feeding us an unexpected

@@ -443,6 +452,9 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	if (unlikely(!cinfo))
 		return -EINVAL;
 
+	if (info->desc->force_polling || cinfo->needs_polling)
+		xfer->hdr.poll_completion = true;
+
 	trace_scmi_xfer_begin(xfer->transfer_id, xfer->hdr.id,
 			      xfer->hdr.protocol_id, xfer->hdr.seq,
 			      xfer->hdr.poll_completion);

@@ -1102,6 +1114,11 @@ static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
 	if (ret)
 		return ret;
 
+	if (tx && (cinfo->needs_polling || info->desc->force_polling))
+		dev_info(dev,
+			 "Enabled polling mode for TX channel - prot_id:%d\n",
+			 prot_id);
+
 idr_alloc:
 	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
 	if (ret != prot_id) {

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

An SCMI transport can declare itself as .atomic_capable in order to signal
to the SCMI core that all its transmit path can be executed in atomic
context: the core as a consequence will take care not to sleep to in the
corresponding rx path while waiting for a response or a delayed response.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
---
 drivers/firmware/arm_scmi/common.h |   1 +
 drivers/firmware/arm_scmi/driver.c | 144 +++++++++++++++++++++++------
 2 files changed, 119 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 8f4e6ebfc0ef..42cf1c79c096 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h

@@ -333,6 +333,7 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  *		   mechanism instead of completion interrupts even if available.
  */
 struct scmi_desc {
+	bool atomic_capable;
 	const struct scmi_transport_ops *ops;
 	int max_rx_timeout_ms;
 	int max_msg;

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index c11ff49f6b62..276b729f2f43 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -360,6 +360,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
 		info->desc->ops->clear_channel(cinfo);
 		complete(xfer->async_done);
 	} else {
+		/*
+		 * This same xfer->done completion is used in atomic mode as a
+		 * flag for polling.
+		 */
 		complete(&xfer->done);
 	}
 }

@@ -410,8 +414,6 @@ static void xfer_put(const struct scmi_protocol_handle *ph,
 	__scmi_xfer_put(&info->tx_minfo, xfer);
 }
 
-#define SCMI_MAX_POLL_TO_NS	(100 * NSEC_PER_USEC)
-
 static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
 				      struct scmi_xfer *xfer, ktime_t stop)
 {

@@ -421,6 +423,79 @@ static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
 	       ktime_after(ktime_get(), stop);
 }
 
+static bool xfer_complete_or_timeout(struct completion *done, ktime_t stop)
+{
+	return try_wait_for_completion(done) || ktime_after(ktime_get(), stop);
+}
+
+static int spin_for_completion_timeout(struct completion *done, int timeout_ms)
+{
+	ktime_t stop = ktime_add_ms(ktime_get(), timeout_ms);
+
+	spin_until_cond(xfer_complete_or_timeout(done, stop));
+	if (ktime_after(ktime_get(), stop))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+/**
+ * scmi_wait_for_message_response  - An helper to group all the possible ways of
+ * waiting for a synchronous message response.
+ *
+ * @cinfo: SCMI channel info
+ * @xfer: Reference to the transfer being waited for.
+ *
+ * Chooses waiting strategy (sleep-waiting vs busy-waiting) depending on flags
+ * configuration like xfer->hdr.poll_completion and scmi_desc.atomic.capable.
+ *
+ * Return: 0 on Success, error otherwise.
+ */
+static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
+					  struct scmi_xfer *xfer)
+{
+	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
+	struct device *dev = info->dev;
+	int ret = 0, timeout_ms = info->desc->max_rx_timeout_ms;
+
+	if (!xfer->hdr.poll_completion) {
+		if (!info->desc->atomic_capable) {
+			if (!wait_for_completion_timeout(&xfer->done,
+							 msecs_to_jiffies(timeout_ms))) {
+				dev_err(dev, "timed out in resp(caller: %pS)\n",
+					(void *)_RET_IP_);
+				ret = -ETIMEDOUT;
+			}
+		} else {
+			/* Poll on xfer->done waiting for completion by interrupt */
+			ret = spin_for_completion_timeout(&xfer->done,
+							  timeout_ms);
+			if (ret)
+				dev_err(dev,
+					"timed out in resp(caller: %pS) - atomic\n",
+					(void *)_RET_IP_);
+		}
+	} else {
+		/*
+		 * Poll on xfer using transport provided .poll_done();
+		 * assumes no completion interrupt was available.
+		 */
+		ktime_t stop = ktime_add_ms(ktime_get(), timeout_ms);
+
+		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
+		if (ktime_after(ktime_get(), stop)) {
+			dev_err(dev,
+				"timed out in resp(caller: %pS) - polling\n",
+				(void *)_RET_IP_);
+			ret = -ETIMEDOUT;
+		} else {
+			info->desc->ops->fetch_response(cinfo, xfer);
+		}
+	}
+
+	return ret;
+}
+
 /**
  * do_xfer() - Do one transfer
  *

@@ -435,7 +510,6 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 		   struct scmi_xfer *xfer)
 {
 	int ret;
-	int timeout;
 	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
 	struct scmi_info *info = handle_to_scmi_info(pi->handle);
 	struct device *dev = info->dev;

@@ -467,25 +541,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 		return ret;
 	}
 
-	if (xfer->hdr.poll_completion) {
-		ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
-
-		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
-
-		if (ktime_before(ktime_get(), stop))
-			info->desc->ops->fetch_response(cinfo, xfer);
-		else
-			ret = -ETIMEDOUT;
-	} else {
-		/* And we wait for the response. */
-		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
-		if (!wait_for_completion_timeout(&xfer->done, timeout)) {
-			dev_err(dev, "timed out in resp(caller: %pS)\n",
-				(void *)_RET_IP_);
-			ret = -ETIMEDOUT;
-		}
-	}
-
+	ret = scmi_wait_for_message_response(cinfo, xfer);
 	if (!ret && xfer->hdr.status)
 		ret = scmi_to_linux_errno(xfer->hdr.status);
 

@@ -507,7 +563,7 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph,
 	xfer->rx.len = info->desc->max_msg_size;
 }
 
-#define SCMI_MAX_RESPONSE_TIMEOUT	(2 * MSEC_PER_SEC)
+#define SCMI_DRESP_TIMEOUT	(2 * MSEC_PER_SEC)
 
 /**
  * do_xfer_with_response() - Do one transfer and wait until the delayed

@@ -516,14 +572,33 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph,
  * @ph: Pointer to SCMI protocol handle
  * @xfer: Transfer to initiate and wait for response
  *
+ * Avois sleeping in favour of busy-waiting if the underlying transport was
+ * declared as .atomic_capable.
+ *
+ * Note that using asynchronous commands when running on top of atomic
+ * transports should be avoided since it could cause long busy-waiting here,
+ * but, once a transport is declared atomic, upper layers using the SCMI stack
+ * can freely make assumptions about the 'non-sleeping' nature of the stack
+ * (e.g. Clock framework) and it cannot be excluded that asynchronous commands
+ * could be exposed by the platform and so used.
+ *
+ * The only other option would have been to refrain from using any asynchronous
+ * command even if made available, when an atomic transport is detected, and
+ * instead forcibly use the synchronous version (thing that can be easily
+ * attained at the protocol layer), but this would also have led to longer
+ * stalls of the channel for synchronous commands and possibly timeouts.
+ * (in other words there is usually a good reason if a platform provides an
+ *  asynchronous version of a command and we should prefer to use it)
+ *
  * Return: -ETIMEDOUT in case of no delayed response, if transmit error,
  *	return corresponding error, else if all goes well, return 0.
  */
 static int do_xfer_with_response(const struct scmi_protocol_handle *ph,
 				 struct scmi_xfer *xfer)
 {
-	int ret, timeout = msecs_to_jiffies(SCMI_MAX_RESPONSE_TIMEOUT);
+	int ret, timeout = msecs_to_jiffies(SCMI_DRESP_TIMEOUT);
 	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
+	struct scmi_info *info = handle_to_scmi_info(pi->handle);
 	DECLARE_COMPLETION_ONSTACK(async_response);
 
 	xfer->hdr.protocol_id = pi->proto->id;

@@ -531,8 +606,25 @@ static int do_xfer_with_response(const struct scmi_protocol_handle *ph,
 	xfer->async_done = &async_response;
 
 	ret = do_xfer(ph, xfer);
-	if (!ret && !wait_for_completion_timeout(xfer->async_done, timeout))
-		ret = -ETIMEDOUT;
+	if (!ret) {
+		if (!info->desc->atomic_capable) {
+			if (!wait_for_completion_timeout(xfer->async_done,
+							 timeout)) {
+				dev_err(ph->dev,
+					"timed out in delayed resp(caller: %pS)\n",
+					(void *)_RET_IP_);
+				ret = -ETIMEDOUT;
+			}
+		} else {
+			ret = spin_for_completion_timeout(xfer->async_done,
+							  SCMI_DRESP_TIMEOUT);
+		}
+
+		if (ret)
+			dev_err(ph->dev,
+				"timed out in delayed resp(caller: %pS) - atomic\n",
+				(void *)_RET_IP_);
+	}
 
 	xfer->async_done = NULL;
 	return ret;

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Having a new step to trace SCMI stack while it waits for synchronous
responses is useful to analyze system performance when changing waiting
mode between polling and interrupt completion.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/trace/events/scmi.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/trace/events/scmi.h b/include/trace/events/scmi.h
index f3a4b4d60714..ba82082f30d7 100644
--- a/include/trace/events/scmi.h
+++ b/include/trace/events/scmi.h

@@ -33,6 +33,34 @@ TRACE_EVENT(scmi_xfer_begin,
 		__entry->seq, __entry->poll)
 );
 
+TRACE_EVENT(scmi_xfer_response_wait,
+	TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq,
+		 bool poll, bool atomic),
+	TP_ARGS(transfer_id, msg_id, protocol_id, seq, poll, atomic),
+
+	TP_STRUCT__entry(
+		__field(int, transfer_id)
+		__field(u8, msg_id)
+		__field(u8, protocol_id)
+		__field(u16, seq)
+		__field(bool, poll)
+		__field(bool, atomic)
+	),
+
+	TP_fast_assign(
+		__entry->transfer_id = transfer_id;
+		__entry->msg_id = msg_id;
+		__entry->protocol_id = protocol_id;
+		__entry->seq = seq;
+		__entry->poll = poll;
+		__entry->atomic = atomic;
+	),
+
+	TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u poll=%u atomic=%u",
+		__entry->transfer_id, __entry->msg_id, __entry->protocol_id,
+		__entry->seq, __entry->poll, __entry->atomic)
+);
+
 TRACE_EVENT(scmi_xfer_end,
 	TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq,
 		 int status),

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Use new trace event to mark start of waiting for response section.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 276b729f2f43..11bea548947f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -458,6 +458,11 @@ static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
 	struct device *dev = info->dev;
 	int ret = 0, timeout_ms = info->desc->max_rx_timeout_ms;
 
+	trace_scmi_xfer_response_wait(xfer->transfer_id, xfer->hdr.id,
+				      xfer->hdr.protocol_id, xfer->hdr.seq,
+				      xfer->hdr.poll_completion,
+				      info->desc->atomic_capable);
+
 	if (!xfer->hdr.poll_completion) {
 		if (!info->desc->atomic_capable) {
 			if (!wait_for_completion_timeout(&xfer->done,

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add a method to check if the underlying transport configured for an SCMI
instance is configured to support atomic transaction of SCMI commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 16 ++++++++++++++++
 include/linux/scmi_protocol.h      |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 11bea548947f..c1bf4c4f60d1 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -1062,6 +1062,21 @@ static void scmi_devm_protocol_put(struct scmi_device *sdev, u8 protocol_id)
 	WARN_ON(ret);
 }
 
+/**
+ * scmi_is_transport_atomic  - Method to check if underlying transport for an
+ * SCMI instance is configured as atomic.
+ *
+ * @handle: A reference to the SCMI platform instance.
+ *
+ * Return: True if transport is configured as atomic
+ */
+static bool scmi_is_transport_atomic(const struct scmi_handle *handle)
+{
+	struct scmi_info *info = handle_to_scmi_info(handle);
+
+	return info->desc->atomic_capable;
+}
+
 static inline
 struct scmi_handle *scmi_handle_get_from_info_unlocked(struct scmi_info *info)
 {

@@ -1535,6 +1550,7 @@ static int scmi_probe(struct platform_device *pdev)
 	handle->version = &info->version;
 	handle->devm_protocol_get = scmi_devm_protocol_get;
 	handle->devm_protocol_put = scmi_devm_protocol_put;
+	handle->is_transport_atomic = scmi_is_transport_atomic;
 
 	ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
 	if (ret)

diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 79d0a1237e6c..b28eb7c104e7 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h

@@ -608,6 +608,13 @@ struct scmi_notify_ops {
  * @devm_protocol_get: devres managed method to acquire a protocol and get specific
  *		       operations and a dedicated protocol handler
  * @devm_protocol_put: devres managed method to release a protocol
+ * @is_transport_atomic: method to check if the underlying transport for this
+ *			 instance handle is configured to support atomic
+ *			 transactions for commands.
+ *			 Some users of the SCMI stack in the upper layers could
+ *			 be interested to know if they can assume SCMI
+ *			 command transactions associated to this handle will
+ *			 never sleep and act accordingly.
  * @notify_ops: pointer to set of notifications related operations
  */
 struct scmi_handle {

@@ -618,6 +625,7 @@ struct scmi_handle {
 		(*devm_protocol_get)(struct scmi_device *sdev, u8 proto,
 				     struct scmi_protocol_handle **ph);
 	void (*devm_protocol_put)(struct scmi_device *sdev, u8 proto);
+	bool (*is_transport_atomic)(const struct scmi_handle *handle);
 
 	const struct scmi_notify_ops *notify_ops;
 };

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Support enable/disable clk_ops instead of prepare/unprepare when the
underlying SCMI transport is configured to support atomic transactions for
synchronous commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/clk/clk-scmi.c | 44 +++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 1e357d364ca2..a07117d5e5bd 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c

@@ -88,21 +88,42 @@ static void scmi_clk_disable(struct clk_hw *hw)
 	scmi_proto_clk_ops->disable(clk->ph, clk->id);
 }
 
+/*
+ * We can provide enable/disable atomic callbacks only if the underlying SCMI
+ * transport for this SCMI instance is configured to handle synchronous commands
+ * in an atomic manner.
+ *
+ * When no SCMI atomic transport support is available we instead provide the
+ * prepare/unprepare API, as allowed by the clock framework where atomic calls
+ * are not available.
+ *
+ * Note that the provided clk_ops implementations are the same in both cases,
+ * using indeed the same underlying SCMI clock_protocol operations, it's just
+ * that they are assured to act in an atomic manner or not depending on the
+ * actual underlying SCMI transport configuration.
+ *
+ * Two distinct sets of clk_ops are provided since we could have multiple SCMI
+ * instances with different underlying transport quality, so they cannot be
+ * shared.
+ */
 static const struct clk_ops scmi_clk_ops = {
 	.recalc_rate = scmi_clk_recalc_rate,
 	.round_rate = scmi_clk_round_rate,
 	.set_rate = scmi_clk_set_rate,
-	/*
-	 * We can't provide enable/disable callback as we can't perform the same
-	 * in atomic context. Since the clock framework provides standard API
-	 * clk_prepare_enable that helps cases using clk_enable in non-atomic
-	 * context, it should be fine providing prepare/unprepare.
-	 */
 	.prepare = scmi_clk_enable,
 	.unprepare = scmi_clk_disable,
 };
 
-static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
+static const struct clk_ops scmi_atomic_clk_ops = {
+	.recalc_rate = scmi_clk_recalc_rate,
+	.round_rate = scmi_clk_round_rate,
+	.set_rate = scmi_clk_set_rate,
+	.enable = scmi_clk_enable,
+	.disable = scmi_clk_disable,
+};
+
+static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
+			     const struct clk_ops *scmi_ops)
 {
 	int ret;
 	unsigned long min_rate, max_rate;

@@ -110,7 +131,7 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
 	struct clk_init_data init = {
 		.flags = CLK_GET_RATE_NOCACHE,
 		.num_parents = 0,
-		.ops = &scmi_clk_ops,
+		.ops = scmi_ops,
 		.name = sclk->info->name,
 	};
 

@@ -145,6 +166,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	struct device_node *np = dev->of_node;
 	const struct scmi_handle *handle = sdev->handle;
 	struct scmi_protocol_handle *ph;
+	bool is_atomic;
 
 	if (!handle)
 		return -ENODEV;

@@ -168,6 +190,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	clk_data->num = count;
 	hws = clk_data->hws;
 
+	is_atomic = handle->is_transport_atomic(handle);
+
 	for (idx = 0; idx < count; idx++) {
 		struct scmi_clk *sclk;
 

@@ -184,7 +208,9 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		sclk->id = idx;
 		sclk->ph = ph;
 
-		err = scmi_clk_ops_init(dev, sclk);
+		err = scmi_clk_ops_init(dev, sclk,
+					is_atomic ? &scmi_atomic_clk_ops :
+					&scmi_clk_ops);
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);
 			devm_kfree(dev, sclk);

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

When a completion irq is available use it and delegate command completion
handling to the core SCMI completion mechanism.

If no completion irq is available revert to polling, using the core common
polling machinery.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 40 ++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index bed5596c7209..cab1d0d50d8e 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c

@@ -25,8 +25,6 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
- * @irq: Optional; employed when platforms indicates msg completion by intr.
- * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {

@@ -34,15 +32,13 @@ struct scmi_smc {
 	struct scmi_shared_mem __iomem *shmem;
 	struct mutex shmem_lock;
 	u32 func_id;
-	int irq;
-	struct completion tx_complete;
 };
 
 static irqreturn_t smc_msg_done_isr(int irq, void *data)
 {
 	struct scmi_smc *scmi_info = data;
 
-	complete(&scmi_info->tx_complete);
+	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
 	return IRQ_HANDLED;
 }

@@ -111,8 +107,9 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 			dev_err(dev, "failed to setup SCMI smc irq\n");
 			return ret;
 		}
-		init_completion(&scmi_info->tx_complete);
-		scmi_info->irq = irq;
+	} else {
+		/* No completion interrupt available */
+		cinfo->needs_polling = true;
 	}
 
 	scmi_info->func_id = func_id;

@@ -142,25 +139,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
 
+	/*
+	 * Channel lock will be released only once response has been
+	 * surely fully retrieved, so after .mark_txdone()
+	 */
 	mutex_lock(&scmi_info->shmem_lock);
-
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
-	if (scmi_info->irq)
-		reinit_completion(&scmi_info->tx_complete);
-
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
-
-	if (scmi_info->irq)
-		wait_for_completion(&scmi_info->tx_complete);
-
-	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
-
-	mutex_unlock(&scmi_info->shmem_lock);
-
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
-	if (res.a0)
+	if (res.a0) {
+		mutex_unlock(&scmi_info->shmem_lock);
 		return -EOPNOTSUPP;
+	}
+
 	return 0;
 }
 

@@ -172,6 +164,13 @@ static void smc_fetch_response(struct scmi_chan_info *cinfo,
 	shmem_fetch_response(scmi_info->shmem, xfer);
 }
 
+static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
+{
+	struct scmi_smc *scmi_info = cinfo->transport_info;
+
+	mutex_unlock(&scmi_info->shmem_lock);
+}
+
 static bool
 smc_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {

@@ -185,6 +184,7 @@ static const struct scmi_transport_ops scmi_smc_ops = {
 	.chan_setup = smc_chan_setup,
 	.chan_free = smc_chan_free,
 	.send_message = smc_send_message,
+	.mark_txdone = smc_mark_txdone,
 	.fetch_response = smc_fetch_response,
 	.poll_done = smc_poll_done,
 };

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Substitute mutex usages with busy-waiting and declare smc transport as
.atomic_capable.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index cab1d0d50d8e..e33985a02068 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c

@@ -7,6 +7,7 @@
  */
 
 #include <linux/arm-smccc.h>
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>

@@ -14,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/processor.h>
 #include <linux/slab.h>
 
 #include "common.h"

@@ -23,14 +25,15 @@
  *
  * @cinfo: SCMI channel info
  * @shmem: Transmit/Receive shared memory area
- * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
+ * @inflight: Atomic flag to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
  */
 
 struct scmi_smc {
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
-	struct mutex shmem_lock;
+#define INFLIGHT_NONE	MSG_TOKEN_MAX
+	atomic_t inflight;
 	u32 func_id;
 };
 

@@ -114,7 +117,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 
 	scmi_info->func_id = func_id;
 	scmi_info->cinfo = cinfo;
-	mutex_init(&scmi_info->shmem_lock);
+	atomic_set(&scmi_info->inflight, INFLIGHT_NONE);
 	cinfo->transport_info = scmi_info;
 
 	return 0;

@@ -133,6 +136,15 @@ static int smc_chan_free(int id, void *p, void *data)
 	return 0;
 }
 
+static bool smc_xfer_inflight(struct scmi_xfer *xfer, atomic_t *inflight)
+{
+	int ret;
+
+	ret = atomic_cmpxchg(inflight, INFLIGHT_NONE, xfer->hdr.seq);
+
+	return ret == INFLIGHT_NONE;
+}
+
 static int smc_send_message(struct scmi_chan_info *cinfo,
 			    struct scmi_xfer *xfer)
 {

@@ -140,16 +152,17 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	struct arm_smccc_res res;
 
 	/*
-	 * Channel lock will be released only once response has been
+	 * Channel will be released only once response has been
 	 * surely fully retrieved, so after .mark_txdone()
 	 */
-	mutex_lock(&scmi_info->shmem_lock);
+	spin_until_cond(smc_xfer_inflight(xfer, &scmi_info->inflight));
+
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
 	if (res.a0) {
-		mutex_unlock(&scmi_info->shmem_lock);
+		atomic_set(&scmi_info->inflight, INFLIGHT_NONE);
 		return -EOPNOTSUPP;
 	}
 

@@ -168,7 +181,7 @@ static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
 {
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 
-	mutex_unlock(&scmi_info->shmem_lock);
+	atomic_set(&scmi_info->inflight, INFLIGHT_NONE);
 }
 
 static bool

@@ -194,4 +207,5 @@ const struct scmi_desc scmi_smc_desc = {
 	.max_rx_timeout_ms = 30,
 	.max_msg = 20,
 	.max_msg_size = 128,
+	.atomic_capable = true,
 };

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Sun, Jun 06, 2021 at 11:12:23PM +0100, Cristian Marussi wrote:

When an SCMI command transfer fails due to some protocol issue an SCMI
error code is reported inside the SCMI message payload itself and it is
then retrieved and transcribed by the specific transport layer into the
xfer.hdr.status field by transport specific .fetch_response().

The core SCMI transport layer never explicitly reset xfer.hdr.status,
so when an xfer is reused, if a transport misbehaved in handling such
status field, we risk to see an invalid ghost error code.

Reset xfer.hdr.status to SCMI_SUCCESS right before each transfer is
started.

Any particular reason why it can't be part of xfer_get_init which has other
initialisations ? If none, please move it there.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Sun, Jun 06, 2021 at 11:12:24PM +0100, Cristian Marussi wrote:

What could happen after xfer_get_init->scmi_xfer_get->reinit_completion
that it needs to be re-initialised again. I don't see any reason for this ?
If there are, please state them explicitly. If this is needed, I would drop
the one in scmi_xfer_get().

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jun 07, 2021 at 06:38:09PM +0100, Sudeep Holla wrote:

On Sun, Jun 06, 2021 at 11:12:23PM +0100, Cristian Marussi wrote:

quoted

When an SCMI command transfer fails due to some protocol issue an SCMI
error code is reported inside the SCMI message payload itself and it is
then retrieved and transcribed by the specific transport layer into the
xfer.hdr.status field by transport specific .fetch_response().

The core SCMI transport layer never explicitly reset xfer.hdr.status,
so when an xfer is reused, if a transport misbehaved in handling such
status field, we risk to see an invalid ghost error code.

Reset xfer.hdr.status to SCMI_SUCCESS right before each transfer is
started.

Any particular reason why it can't be part of xfer_get_init which has other
initialisations ? If none, please move it there.

Well it was there initially then I moved it here.

The reason is mostly the same as the reason for the other patch in this
series that adds a reinit_completion() in this same point: the core does
not forbid to reuse an xfer multiple times, once obtained with xfer_get()
or xfer_get_init(), and indeed some protocols do such a thing: they
implements such do_xfer looping and bails out on error.

In the way that it is implemented now in protocols poses no problem
indeed because the do_xfer loop bails out on error and the xfer is put,
but as soon as some protocol is implemented that violates this common
practice and it just keeps on reuse an xfer after an error fo other
do_xfers() this breaks...so it seemed more defensive to just reinit the
completion and the status before each send.

Thanks,
Cristian

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi,

On Mon, Jun 07, 2021 at 06:42:57PM +0100, Sudeep Holla wrote:

On Sun, Jun 06, 2021 at 11:12:24PM +0100, Cristian Marussi wrote:

quoted

Reusing timed out xfers in a loop can lead to issue if completion was not
properly reinitialized.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index bee33f9c2032..759ae4a23e74 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 			      xfer->hdr.poll_completion);

 	xfer->hdr.status = SCMI_SUCCESS;
+	reinit_completion(&xfer->done);

What could happen after xfer_get_init->scmi_xfer_get->reinit_completion
that it needs to be re-initialised again. I don't see any reason for this ?
If there are, please state them explicitly. If this is needed, I would drop
the one in scmi_xfer_get().

The reason, like I explained in the other reply in hdr.status, is the
possibility of do_xfer loops and being more defensive.

I agree that if what I blabbed in the other email is acceptable, I could
drop the reinit_completion in xfer_get() and just use it before
send_message().

Thanks,
Cristian

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jun 07, 2021 at 07:01:37PM +0100, Cristian Marussi wrote:

On Mon, Jun 07, 2021 at 06:38:09PM +0100, Sudeep Holla wrote:

quoted

On Sun, Jun 06, 2021 at 11:12:23PM +0100, Cristian Marussi wrote:

quoted

When an SCMI command transfer fails due to some protocol issue an SCMI
error code is reported inside the SCMI message payload itself and it is
then retrieved and transcribed by the specific transport layer into the
xfer.hdr.status field by transport specific .fetch_response().

The core SCMI transport layer never explicitly reset xfer.hdr.status,
so when an xfer is reused, if a transport misbehaved in handling such
status field, we risk to see an invalid ghost error code.

Reset xfer.hdr.status to SCMI_SUCCESS right before each transfer is
started.

Any particular reason why it can't be part of xfer_get_init which has other
initialisations ? If none, please move it there.

Well it was there initially then I moved it here.

The reason is mostly the same as the reason for the other patch in this
series that adds a reinit_completion() in this same point: the core does
not forbid to reuse an xfer multiple times, once obtained with xfer_get()
or xfer_get_init(), and indeed some protocols do such a thing: they
implements such do_xfer looping and bails out on error.

Makes sense. But it is okay to retain xfer->transfer_id for every transfer
in such a loop ?

In the way that it is implemented now in protocols poses no problem
indeed because the do_xfer loop bails out on error and the xfer is put,
but as soon as some protocol is implemented that violates this common
practice and it just keeps on reuse an xfer after an error fo other
do_xfers() this breaks...so it seemed more defensive to just reinit the
completion and the status before each send.

Fair enough. But they use it to send same message I guess, may be if it
gave error or something ? I would like to really know such a sequence
instead of assisting that 😉. 

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Jun 07, 2021 at 07:04:34PM +0100, Cristian Marussi wrote:

Hi,

On Mon, Jun 07, 2021 at 06:42:57PM +0100, Sudeep Holla wrote:

quoted

On Sun, Jun 06, 2021 at 11:12:24PM +0100, Cristian Marussi wrote:

quoted

Reusing timed out xfers in a loop can lead to issue if completion was not
properly reinitialized.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index bee33f9c2032..759ae4a23e74 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c

@@ -448,6 +448,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 			      xfer->hdr.poll_completion);

 	xfer->hdr.status = SCMI_SUCCESS;
+	reinit_completion(&xfer->done);

What could happen after xfer_get_init->scmi_xfer_get->reinit_completion
that it needs to be re-initialised again. I don't see any reason for this ?
If there are, please state them explicitly. If this is needed, I would drop
the one in scmi_xfer_get().

The reason, like I explained in the other reply in hdr.status, is the
possibility of do_xfer loops and being more defensive.

Understood.

I agree that if what I blabbed in the other email is acceptable, I could
drop the reinit_completion in xfer_get() and just use it before
send_message().

I wonder if it is possible to reset only on error if possible and not
expensive of-course ?

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Sudeep,

On Mon, Jun 07, 2021 at 07:27:54PM +0100, Sudeep Holla wrote:

On Mon, Jun 07, 2021 at 07:01:37PM +0100, Cristian Marussi wrote:

quoted

On Mon, Jun 07, 2021 at 06:38:09PM +0100, Sudeep Holla wrote:

quoted

On Sun, Jun 06, 2021 at 11:12:23PM +0100, Cristian Marussi wrote:

quoted

When an SCMI command transfer fails due to some protocol issue an SCMI
error code is reported inside the SCMI message payload itself and it is
then retrieved and transcribed by the specific transport layer into the
xfer.hdr.status field by transport specific .fetch_response().

The core SCMI transport layer never explicitly reset xfer.hdr.status,
so when an xfer is reused, if a transport misbehaved in handling such
status field, we risk to see an invalid ghost error code.

Reset xfer.hdr.status to SCMI_SUCCESS right before each transfer is
started.

Any particular reason why it can't be part of xfer_get_init which has other
initialisations ? If none, please move it there.

Well it was there initially then I moved it here.

The reason is mostly the same as the reason for the other patch in this
series that adds a reinit_completion() in this same point: the core does
not forbid to reuse an xfer multiple times, once obtained with xfer_get()
or xfer_get_init(), and indeed some protocols do such a thing: they
implements such do_xfer looping and bails out on error.

Makes sense. But it is okay to retain xfer->transfer_id for every transfer
in such a loop ?

No you are right and indeed I saw that anomaly, but I have not addressed
it since, even if wrong, it is harmless and transfer_id is really used
only for debugging/profiling, while the missing reinit_completion is
potentially broken.

quoted

In the way that it is implemented now in protocols poses no problem
indeed because the do_xfer loop bails out on error and the xfer is put,
but as soon as some protocol is implemented that violates this common
practice and it just keeps on reuse an xfer after an error fo other
do_xfers() this breaks...so it seemed more defensive to just reinit the
completion and the status before each send.

Fair enough. But they use it to send same message I guess, may be if it
gave error or something ? I would like to really know such a sequence
instead of assisting that 😉. 

So the current real 'looping do_xfer' behavior is safe and so this missing
reinit is only potentially broken in the future, and we cannot really
know now in advance about some future protocol needs, but it seems as of now
wrong that you'll want to keep going on and reuse an xfer for the same command
after an error in your loop.

On the other side we allow such behaviour, so I thought was good to
provide a safe net if it is misused.

But, beside this patches, that, as said, are more defensive that strictly
needed as of now, I think now it's worth mentioning that this same 'issue'
affects also, as an example, the new mechanism I introduced later in this
same series to always use monotonically increasing sequence number for
outgoing messages.

In that case I stick to the current behavior and I assign such monotonically
increasing sequence numbers to message during xfer_get, but the potential
issue is the same: if a do_xfer loop is used you end up reusing the same
seq_num for multiple do_xfers (so defeating really the mechanism itself
that aims not to reuse immediately the most recently used seq_num).

In that case I did this to keep it simple and to avoid placing more burden
on tx path by picking and assigning a seq_num upon each transfer...but, again,
also this behavior of picking a seq_num only at xfer_get is NOT really broken
as of now even for do_xfer loops since we bail out on error and you won't
really reuse that xfer.

It's just that in this seq_num selection case seems to add a lot of burden
and complexity if moved to the do_xfer phase, while status/reinit seemed
to me cheaper to move it in the do_xfer so I tried to play defensive.

At the end, in general I would say that all of these ops (status/reinit/
seq_nums/transfer_id) DO really belong logically to the do_xfer phase more than
to the xfer_get/xfer_get_init, but in reality we can cope with having them
@xfer_get/get_init and this keeps things simple and reduce burden, especially
in the monotonic seq_nums case: so I am not so sure anymore if it is fine to
move reinit/status to the do_xfer, as proposed here, while keeping seq_nums
(for good reasons) to the xfer_get phase, because we'd use 2 different strategies
to address similar issues.

I would say: just keep reinit and status in the xfer_get phase instead and
maybe warn somehow if a failed xfer is detected being reused. (but this
would anyway need a check in every tx transaction to see if status != SUCCESS
so is it worth ?)

Lot of overthinking for a one-liner :D ... sorry

Thanks,
Cristian

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jun 08, 2021 at 11:10:48AM +0100, Cristian Marussi wrote:

Hi Sudeep,

On Mon, Jun 07, 2021 at 07:27:54PM +0100, Sudeep Holla wrote:

quoted

On Mon, Jun 07, 2021 at 07:01:37PM +0100, Cristian Marussi wrote:

quoted

On Mon, Jun 07, 2021 at 06:38:09PM +0100, Sudeep Holla wrote:

quoted

On Sun, Jun 06, 2021 at 11:12:23PM +0100, Cristian Marussi wrote:

quoted

When an SCMI command transfer fails due to some protocol issue an SCMI
error code is reported inside the SCMI message payload itself and it is
then retrieved and transcribed by the specific transport layer into the
xfer.hdr.status field by transport specific .fetch_response().

The core SCMI transport layer never explicitly reset xfer.hdr.status,
so when an xfer is reused, if a transport misbehaved in handling such
status field, we risk to see an invalid ghost error code.

Reset xfer.hdr.status to SCMI_SUCCESS right before each transfer is
started.

Any particular reason why it can't be part of xfer_get_init which has other
initialisations ? If none, please move it there.

Well it was there initially then I moved it here.

The reason is mostly the same as the reason for the other patch in this
series that adds a reinit_completion() in this same point: the core does
not forbid to reuse an xfer multiple times, once obtained with xfer_get()
or xfer_get_init(), and indeed some protocols do such a thing: they
implements such do_xfer looping and bails out on error.

Makes sense. But it is okay to retain xfer->transfer_id for every transfer
in such a loop ?

No you are right and indeed I saw that anomaly, but I have not addressed
it since, even if wrong, it is harmless and transfer_id is really used
only for debugging/profiling, while the missing reinit_completion is
potentially broken.

No agreed, just wanted to make it clear that if do_xfer is used in loops
the transfer_id remains same. I am fine with that.

quoted

quoted

In the way that it is implemented now in protocols poses no problem
indeed because the do_xfer loop bails out on error and the xfer is put,
but as soon as some protocol is implemented that violates this common
practice and it just keeps on reuse an xfer after an error fo other
do_xfers() this breaks...so it seemed more defensive to just reinit the
completion and the status before each send.

Fair enough. But they use it to send same message I guess, may be if it
gave error or something ? I would like to really know such a sequence
instead of assisting that 😉. 

So the current real 'looping do_xfer' behavior is safe and so this missing
reinit is only potentially broken in the future, and we cannot really
know now in advance about some future protocol needs, but it seems as of now
wrong that you'll want to keep going on and reuse an xfer for the same command
after an error in your loop.

Fair enough.

On the other side we allow such behaviour, so I thought was good to
provide a safe net if it is misused.

Agreed.

But, beside this patches, that, as said, are more defensive that strictly
needed as of now, I think now it's worth mentioning that this same 'issue'
affects also, as an example, the new mechanism I introduced later in this
same series to always use monotonically increasing sequence number for
outgoing messages.

OK, I haven't seen that yet.

In that case I stick to the current behavior and I assign such monotonically
increasing sequence numbers to message during xfer_get, but the potential
issue is the same: if a do_xfer loop is used you end up reusing the same
seq_num for multiple do_xfers (so defeating really the mechanism itself
that aims not to reuse immediately the most recently used seq_num).

I assumed the do_xfer loop is to avoid those overheads with compromise of
reusing seq_num.

In that case I did this to keep it simple and to avoid placing more burden
on tx path by picking and assigning a seq_num upon each transfer...but, again,
also this behavior of picking a seq_num only at xfer_get is NOT really broken
as of now even for do_xfer loops since we bail out on error and you won't
really reuse that xfer.

OK.

It's just that in this seq_num selection case seems to add a lot of burden
and complexity if moved to the do_xfer phase, while status/reinit seemed
to me cheaper to move it in the do_xfer so I tried to play defensive.

I assumed the same as mentioned above.

At the end, in general I would say that all of these ops (status/reinit/
seq_nums/transfer_id) DO really belong logically to the do_xfer phase more than
to the xfer_get/xfer_get_init, but in reality we can cope with having them
@xfer_get/get_init and this keeps things simple and reduce burden, especially
in the monotonic seq_nums case: so I am not so sure anymore if it is fine to
move reinit/status to the do_xfer, as proposed here, while keeping seq_nums
(for good reasons) to the xfer_get phase, because we'd use 2 different strategies
to address similar issues.

I almost agreed with the change just to read here you think otherwise now 😄.

I would say: just keep reinit and status in the xfer_get phase instead and
maybe warn somehow if a failed xfer is detected being reused. (but this
would anyway need a check in every tx transaction to see if status != SUCCESS
so is it worth ?)

I have started thinking why do we need to reset the status. Since it is
always read from the shmem, do we really have to ?

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Sun, 6 Jun 2021 23:12:22 +0100, Cristian Marussi wrote:

Reusing timed out xfers in a loop can lead to issue if completion was not
properly reinitialized.

Applied to sudeep.holla/linux (master), thanks!

[02/10] firmware: arm_scmi: Add missing xfer reinit_completion
        https://git.kernel.org/sudeep.holla/c/e30d91d4ff

--
Regards,
Sudeep


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel