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 <[email protected]>
---
drivers/firmware/arm_scmi/common.h | 3 +
drivers/firmware/arm_scmi/driver.c | 167 ++++++++++++++++++++++-------
2 files changed, 132 insertions(+), 38 deletions(-)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 67c761141a48..4ab310c2eae5 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -412,6 +412,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
* @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.
+ * @atomic_capable: Flag to indicate that this transport is assured not to sleep
+ * on the TX path.
*/
struct scmi_desc {
int (*transport_init)(void);
@@ -421,6 +423,7 @@ struct scmi_desc {
int max_msg;
int max_msg_size;
bool force_polling;
+ bool atomic_capable;
};
#ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index a3700f49e8ac..2ca1602afd80 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -681,6 +681,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
scmi_clear_channel(info, cinfo);
complete(xfer->async_done);
} else {
+ /*
+ * This same xfer->done completion is used in atomic mode as a
+ * flag for polling.
+ */
complete(&xfer->done);
}
@@ -733,8 +737,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)
{
@@ -749,6 +751,90 @@ 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_before(ktime_get(), stop)) {
+ unsigned long flags;
+
+ /*
+ * Do not fetch_response if an out-of-order
+ * delayed response is being processed.
+ */
+ spin_lock_irqsave(&xfer->lock, flags);
+ if (xfer->state == SCMI_XFER_SENT_OK) {
+ info->desc->ops->fetch_response(cinfo, xfer);
+ xfer->state = SCMI_XFER_RESP_OK;
+ }
+ spin_unlock_irqrestore(&xfer->lock, flags);
+ } else {
+ dev_err(dev,
+ "timed out in resp(caller: %pS) - polling\n",
+ (void *)_RET_IP_);
+ ret = -ETIMEDOUT;
+ }
+ }
+
+ return ret;
+}
+
/**
* do_xfer() - Do one transfer
*
@@ -763,7 +849,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;
@@ -810,36 +895,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)) {
- unsigned long flags;
-
- /*
- * Do not fetch_response if an out-of-order delayed
- * response is being processed.
- */
- spin_lock_irqsave(&xfer->lock, flags);
- if (xfer->state == SCMI_XFER_SENT_OK) {
- info->desc->ops->fetch_response(cinfo, xfer);
- xfer->state = SCMI_XFER_RESP_OK;
- }
- spin_unlock_irqrestore(&xfer->lock, flags);
- } 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);
@@ -861,7 +917,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
@@ -870,22 +926,57 @@ 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->async_done = &async_response;
ret = do_xfer(ph, xfer);
if (!ret) {
- if (!wait_for_completion_timeout(xfer->async_done, timeout))
- ret = -ETIMEDOUT;
- else if (xfer->hdr.status)
+ 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_);
+ }
+
+ if (!ret && xfer->hdr.status)
ret = scmi_to_linux_errno(xfer->hdr.status);
}
--
2.17.1
On Wed, Aug 25, 2021 at 12:18:31PM -0400, Jim Quinlan wrote:
> Hi Christian,
>
Hi Jim,
thanks for the review first of all.
> On Tue, Aug 24, 2021 at 10:00 AM Cristian Marussi
> <[email protected]> wrote:
> >
> > 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 <[email protected]>
> > ---
> > drivers/firmware/arm_scmi/common.h | 3 +
> > drivers/firmware/arm_scmi/driver.c | 167 ++++++++++++++++++++++-------
> > 2 files changed, 132 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 67c761141a48..4ab310c2eae5 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -412,6 +412,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
> > * @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.
> > + * @atomic_capable: Flag to indicate that this transport is assured not to sleep
> > + * on the TX path.
> > */
> > struct scmi_desc {
> > int (*transport_init)(void);
> > @@ -421,6 +423,7 @@ struct scmi_desc {
> > int max_msg;
> > int max_msg_size;
> > bool force_polling;
> > + bool atomic_capable;
> > };
> >
> > #ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index a3700f49e8ac..2ca1602afd80 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -681,6 +681,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
> > scmi_clear_channel(info, cinfo);
> > complete(xfer->async_done);
> > } else {
> > + /*
> > + * This same xfer->done completion is used in atomic mode as a
> > + * flag for polling.
> > + */
> > complete(&xfer->done);
> > }
> >
> > @@ -733,8 +737,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)
> > {
> > @@ -749,6 +751,90 @@ 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);
> We use the SMC transport with a completion interrupt but would prefer
> for the above to use wait_for_completion(...) instead of using
> spin_for_completion(). A few of our SCMI commands can take a while to
> complete execution so we do not want to be spinning or polling while
> waiting.
Busy-waiting when using a completion IRQ is used indeed only if the
transport has been declared .atomic_capable: the idea was that if
the specific transport does not sleep, the core can avoid sleeping too
and so enable users like the clock framework to switch to 'atomic' mode
and being called from atomic context.
So if you drop indeed the SMC patch later in the series that mmake it
atomic
[PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic
you'll end up using the wait_for here when completion IRQ is used.
(instead to avoid any polling while in polling mode (O_o) the idea was
to use the new .sync_cmds_atomic_replies)
BUT the problem is that some other partner could possibly want to use
instead this same transport in atomic mode, so maybe it could be worth
introducing some sort of configurability so that atomic operations are
enabled only if (say) .atomic_capable && .atomic_enable.
Maybe this could be done on a per-channel base, so you could dedicate a
channel to a protocol and ask only for it to be atomic_enable (if the
whole transport is atomic_capable); mixing atomic/non_atomic behaviour
inside the same protocol/channel across different messages seems a bit
of a hell (...and I think a previous attempt led to a lot of issues in
the past)
Anyway, not sure really where to put this possible configuration bit
though...this not being really a FW config/description seems likely to
be a NACK for a placement in the DT :D
>
> We could probably go back to a mailbox-based transport and use delayed
> messages here for these "long" SCMI messages, but we do not have full
> control over the platform FW (plus there are backwards compatibility
> issues). FWIW, the platform FW was setup before SCMI on Linux
> implemented delayed/async messages.
>
Having a completion IRQ couldn't you use it to send delayed responses or
notification even in this smc transport ? (well it'd need FW to be
updated in fact and maybe I'm missing something here)
Thanks a lot for the feedback, may I assume this initial series it sort
of worked (beside noted limitations) in your SMC test setup :D ?
Thanks,
Cristian
Hi Christian,
On Tue, Aug 24, 2021 at 10:00 AM Cristian Marussi
<[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> drivers/firmware/arm_scmi/common.h | 3 +
> drivers/firmware/arm_scmi/driver.c | 167 ++++++++++++++++++++++-------
> 2 files changed, 132 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 67c761141a48..4ab310c2eae5 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -412,6 +412,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
> * @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.
> + * @atomic_capable: Flag to indicate that this transport is assured not to sleep
> + * on the TX path.
> */
> struct scmi_desc {
> int (*transport_init)(void);
> @@ -421,6 +423,7 @@ struct scmi_desc {
> int max_msg;
> int max_msg_size;
> bool force_polling;
> + bool atomic_capable;
> };
>
> #ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index a3700f49e8ac..2ca1602afd80 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -681,6 +681,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
> scmi_clear_channel(info, cinfo);
> complete(xfer->async_done);
> } else {
> + /*
> + * This same xfer->done completion is used in atomic mode as a
> + * flag for polling.
> + */
> complete(&xfer->done);
> }
>
> @@ -733,8 +737,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)
> {
> @@ -749,6 +751,90 @@ 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);
We use the SMC transport with a completion interrupt but would prefer
for the above to use wait_for_completion(...) instead of using
spin_for_completion(). A few of our SCMI commands can take a while to
complete execution so we do not want to be spinning or polling while
waiting.
We could probably go back to a mailbox-based transport and use delayed
messages here for these "long" SCMI messages, but we do not have full
control over the platform FW (plus there are backwards compatibility
issues). FWIW, the platform FW was setup before SCMI on Linux
implemented delayed/async messages.
Regards,
Jim Quinlan
Broadcom STB
> + 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_before(ktime_get(), stop)) {
> + unsigned long flags;
> +
> + /*
> + * Do not fetch_response if an out-of-order
> + * delayed response is being processed.
> + */
> + spin_lock_irqsave(&xfer->lock, flags);
> + if (xfer->state == SCMI_XFER_SENT_OK) {
> + info->desc->ops->fetch_response(cinfo, xfer);
> + xfer->state = SCMI_XFER_RESP_OK;
> + }
> + spin_unlock_irqrestore(&xfer->lock, flags);
> + } else {
> + dev_err(dev,
> + "timed out in resp(caller: %pS) - polling\n",
> + (void *)_RET_IP_);
> + ret = -ETIMEDOUT;
> + }
> + }
> +
> + return ret;
> +}
> +
> /**
> * do_xfer() - Do one transfer
> *
> @@ -763,7 +849,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;
> @@ -810,36 +895,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)) {
> - unsigned long flags;
> -
> - /*
> - * Do not fetch_response if an out-of-order delayed
> - * response is being processed.
> - */
> - spin_lock_irqsave(&xfer->lock, flags);
> - if (xfer->state == SCMI_XFER_SENT_OK) {
> - info->desc->ops->fetch_response(cinfo, xfer);
> - xfer->state = SCMI_XFER_RESP_OK;
> - }
> - spin_unlock_irqrestore(&xfer->lock, flags);
> - } 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);
>
> @@ -861,7 +917,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
> @@ -870,22 +926,57 @@ 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->async_done = &async_response;
>
> ret = do_xfer(ph, xfer);
> if (!ret) {
> - if (!wait_for_completion_timeout(xfer->async_done, timeout))
> - ret = -ETIMEDOUT;
> - else if (xfer->hdr.status)
> + 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_);
> + }
> +
> + if (!ret && xfer->hdr.status)
> ret = scmi_to_linux_errno(xfer->hdr.status);
> }
>
> --
> 2.17.1
>