Re: [PATCH v4 05/16] firmware: arm_scmi: Introduce delegated xfers support

From: Peter Hilber
Date: Thu Jul 01 2021 - 04:42:58 EST

On 11.06.21 18:59, Cristian Marussi wrote:
Introduce optional support for delegated xfers allocation.

An SCMI transport can optionally declare to support delegated xfers and
then use a few helper functions exposed by the core SCMI transport layer to
query the core for existing in-flight transfers matching a provided message
header or alternatively and transparently obtain a brand new xfer to handle
a freshly received notification message.
In both cases the obtained xfer is uniquely mapped into a specific xfer
through the means of the message header acting as key.

In this way such a transport can properly store its own transport specific
payload into the xfer uniquely associated to the message header before
even calling into the core scmi_rx_callback() in the usual way, so that
the transport specific message envelope structures can be freed early
and there is no more need to keep track of their status till the core
fully processes the xfer to completion or times out.

The scmi_rx_callbak() does not need to be modified to carry additional
transport-specific ancillary data related to such message envelopes since
an unique natural association is established between the xfer and the
related message header.

Existing transports that do not need anything of the above will continue
to work as before without any change.

Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
---
drivers/firmware/arm_scmi/common.h | 14 +++
drivers/firmware/arm_scmi/driver.c | 132 ++++++++++++++++++++++++++++-
2 files changed, 143 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index e64c5ca9ee7c..0edc04bc434c 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -80,6 +80,7 @@ struct scmi_msg_resp_prot_version {
* @status: Status of the transfer once it's complete
* @poll_completion: Indicate if the transfer needs to be polled for
* completion or interrupt mode is used
+ * @saved_hdr: A copy of the original msg_hdr
*/
struct scmi_msg_hdr {
u8 id;
@@ -88,6 +89,7 @@ struct scmi_msg_hdr {
u16 seq;
u32 status;
bool poll_completion;
+ u32 saved_hdr;
};
/**
@@ -154,6 +156,9 @@ struct scmi_msg {
* @rx: Receive message, the buffer should be pre-allocated to store
* message. If request-ACK protocol is used, we can reuse the same
* buffer for the rx path as we use for the tx path.
+ * @rx_raw_len: A field which can be optionally used by a specific transport
+ * to save transport specific message length
+ * It is not used by the SCMI transport core
* @done: command message transmit completion event
* @async_done: pointer to delayed response message received event completion
* @users: A refcount to track the active users for this xfer
@@ -165,6 +170,7 @@ struct scmi_xfer {
struct scmi_msg_hdr hdr;
struct scmi_msg tx;
struct scmi_msg rx;
+ size_t rx_raw_len;
struct completion done;
struct completion *async_done;
refcount_t users;
@@ -355,6 +361,9 @@ 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.
+ * @support_xfers_delegation: A flag to indicate if the described transport
+ * will handle delegated xfers, so the core can
+ * derive proper related assumptions.
*/
struct scmi_desc {
int (*init)(void);
@@ -363,6 +372,7 @@ struct scmi_desc {
int max_rx_timeout_ms;
int max_msg;
int max_msg_size;
+ bool support_xfers_delegation;
};
extern const struct scmi_desc scmi_mailbox_desc;
@@ -391,4 +401,8 @@ void scmi_notification_instance_data_set(const struct scmi_handle *handle,
void *priv);
void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
+int scmi_transfer_acquire(struct scmi_chan_info *cinfo, u32 *msg_hdr,
+ struct scmi_xfer **xfer);
+void scmi_transfer_release(struct scmi_chan_info *cinfo,
+ struct scmi_xfer *xfer);
#endif /* _SCMI_COMMON_H */
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index f0b20ddb24f4..371d3804cd79 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -432,6 +432,124 @@ scmi_xfer_lookup_unlocked(struct scmi_xfers_info *minfo, u16 xfer_id)
return xfer ?: ERR_PTR(-EINVAL);
}
+/**
+ * scmi_xfer_acquire - Helper to lookup and acquire an xfer
+ *
+ * @minfo: Pointer to Tx/Rx Message management info based on channel type
+ * @xfer_id: Token ID to lookup in @pending_xfers
+ *
+ * When a valid xfer is found for the provided @xfer_id, reference counting is
+ * properly updated.
+ *
+ * Return: A valid @xfer on Success or error otherwise.
+ */
+static struct scmi_xfer *
+scmi_xfer_acquire(struct scmi_xfers_info *minfo, u16 xfer_id)
+{
+ unsigned long flags;
+ struct scmi_xfer *xfer;
+
+ spin_lock_irqsave(&minfo->xfer_lock, flags);
+ xfer = scmi_xfer_lookup_unlocked(minfo, xfer_id);
+ if (!IS_ERR(xfer))
+ refcount_inc(&xfer->users);
+ spin_unlock_irqrestore(&minfo->xfer_lock, flags);
+
+ return xfer;
+}
+
+/**
+ * scmi_transfer_acquire - Lookup for an existing xfer or freshly allocate a
+ * new one depending on the type of the message
+ *
+ * @cinfo: A reference to the channel descriptor.
+ * @msg_hdr: A pointer to the message header to lookup.
+ * @xfer: A reference to the pre-existent or freshly allocated xfer
+ * associated with the provided *msg_hdr.
+ *
+ * This function can be used by transports supporting delegated xfers to obtain
+ * a valid @xfer associated with the provided @msg_hdr param.
+ *
+ * The nature of the resulting @xfer depends on the type of message specified in
+ * @msg_hdr:
+ * - for responses and delayed responses a pre-existent/pre-allocated in-flight
+ * xfer descriptor will be returned (properly refcounted)
+ * - for notifications a brand new xfer will be allocated; in this case the
+ * provided message header sequence number will also be mangled to match
+ * the token in the freshly allocated xfer: this is needed to establish a
+ * link between the picked xfer and the msg_hdr that will be subsequently
+ * passed back via usual scmi_rx_callback().
+ *
+ * Return: 0 if a valid xfer is returned in @xfer, error otherwise.
+ */
+int scmi_transfer_acquire(struct scmi_chan_info *cinfo, u32 *msg_hdr,
+ struct scmi_xfer **xfer)
+{
+ u8 msg_type;
+ struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
+
+ if (!xfer || !msg_hdr || !info->desc->support_xfers_delegation)
+ return -EINVAL;
+
+ msg_type = MSG_XTRACT_TYPE(*msg_hdr);
+ switch (msg_type) {
+ case MSG_TYPE_COMMAND:
+ case MSG_TYPE_DELAYED_RESP:
+ /* Grab an existing xfer for xfer_id */
+ *xfer = scmi_xfer_acquire(&info->tx_minfo,
+ MSG_XTRACT_TOKEN(*msg_hdr));
+ break;
+ case MSG_TYPE_NOTIFICATION:
+ /* Get a brand new RX xfer */
+ *xfer = scmi_xfer_get(cinfo->handle, &info->rx_minfo);
+ if (!IS_ERR(*xfer)) {
+ /* Save original msg_hdr and fix sequence number */
+ (*xfer)->hdr.saved_hdr = *msg_hdr;

The saved header isn't used anywhere.

+ *msg_hdr &= ~MSG_TOKEN_ID_MASK;
+ *msg_hdr |= FIELD_PREP(MSG_TOKEN_ID_MASK,
+ (*xfer)->hdr.seq);

This will invalidate the token set by the platform in scmi_dump_header_dbg(). Maybe it would have been more elegant to introduce a dedicated hash table key field?

+ }
+ break;
+ default:
+ *xfer = ERR_PTR(-EINVAL);
+ break;
+ }
+
+ if (IS_ERR(*xfer)) {
+ dev_err(cinfo->dev,
+ "Failed to acquire a valid xfer for hdr:0x%X\n",
+ *msg_hdr);
+ return PTR_ERR(*xfer);
+ }
+
+ /* Fix xfer->hdr.type with actual msg_hdr carried type */
+ unpack_scmi_header(*msg_hdr, &((*xfer)->hdr));
+
+ return 0;
+}
+
+/**
+ * scmi_transfer_release - Release an previously acquired xfer
+ *
+ * @cinfo: A reference to the channel descriptor.
+ * @xfer: A reference to the xfer to release.
+ */
+void scmi_transfer_release(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
+{
+ struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
+ struct scmi_xfers_info *minfo;
+
+ if (!xfer || !info->desc->support_xfers_delegation)
+ return;
+
+ if (xfer->hdr.type == MSG_TYPE_NOTIFICATION)
+ minfo = &info->rx_minfo;
+ else
+ minfo = &info->tx_minfo;
+
+ __scmi_xfer_put(minfo, xfer);
+}
+
static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
{
struct scmi_xfer *xfer;
@@ -441,7 +559,11 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
ktime_t ts;
ts = ktime_get_boottime();
- xfer = scmi_xfer_get(cinfo->handle, minfo);
+
+ if (!info->desc->support_xfers_delegation)
+ xfer = scmi_xfer_get(cinfo->handle, minfo);
+ else
+ xfer = scmi_xfer_acquire(minfo, MSG_XTRACT_TOKEN(msg_hdr));
if (IS_ERR(xfer)) {
dev_err(dev, "failed to get free message slot (%ld)\n",
PTR_ERR(xfer));
@@ -449,8 +571,11 @@ static void scmi_handle_notification(struct scmi_chan_info *cinfo, u32 msg_hdr)
return;
}
- unpack_scmi_header(msg_hdr, &xfer->hdr);
scmi_dump_header_dbg(dev, &xfer->hdr);
+
+ if (!info->desc->support_xfers_delegation)
+ unpack_scmi_header(msg_hdr, &xfer->hdr);
+

Why dump the header before unpacking?

info->desc->ops->fetch_notification(cinfo, info->desc->max_msg_size,
xfer);
scmi_notify(cinfo->handle, xfer->hdr.protocol_id,
@@ -496,7 +621,8 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
xfer_id);
info->desc->ops->clear_channel(cinfo);
/* It was unexpected, so nobody will clear the xfer if not us */
- __scmi_xfer_put(minfo, xfer);
+ if (!info->desc->support_xfers_delegation) //XXX ??? Really
+ __scmi_xfer_put(minfo, xfer);
return;
}