Re: [PATCH v4 15/16] [RFC][REWORK] firmware: arm_scmi: make virtio-scmi use delegated xfers
From: Peter Hilber
Date: Thu Jul 01 2021 - 04:43:20 EST
On 11.06.21 18:59, Cristian Marussi wrote:
Draft changes to virtio-scmi to use new support for core delegated xfers
in an attempt to simplify the interactions between virtio-scmi transport
and the SCMI core transport layer.
These changes seem to make xfers delegation mandatory for
message-passing transports, so that might be documented.
TODO:
- Polling is still not supported.
- Probe/remove sequence still to be reviewed.
- Concurrent or inverted reception of related responses and delayed
responses is still not addressed.
(it will be addressed in the SCMI core anyway most probably)
Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
---
drivers/firmware/arm_scmi/common.h | 5 +
drivers/firmware/arm_scmi/msg.c | 35 +++++
drivers/firmware/arm_scmi/virtio.c | 212 +++++++++++++++--------------
3 files changed, 152 insertions(+), 100 deletions(-)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index c143a449d278..22e5532fc698 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -428,6 +428,11 @@ void msg_fetch_response(struct scmi_msg_payld *msg, size_t len,
void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len,
size_t max_len, struct scmi_xfer *xfer);
+void msg_fetch_raw_payload(struct scmi_msg_payld *msg, size_t msg_len,
+ size_t max_len, struct scmi_xfer *xfer);
+void msg_fetch_raw_response(struct scmi_xfer *xfer);
+void msg_fetch_raw_notification(struct scmi_xfer *xfer);
+
void scmi_notification_instance_data_set(const struct scmi_handle *handle,
void *priv);
void *scmi_notification_instance_data_get(const struct scmi_handle *handle);
diff --git a/drivers/firmware/arm_scmi/msg.c b/drivers/firmware/arm_scmi/msg.c
index 8a2d3303d281..3ed3ad0961ae 100644
--- a/drivers/firmware/arm_scmi/msg.c
+++ b/drivers/firmware/arm_scmi/msg.c
@@ -74,6 +74,17 @@ u32 msg_read_header(struct scmi_msg_payld *msg)
return le32_to_cpu(msg->msg_header);
}
+void msg_fetch_raw_payload(struct scmi_msg_payld *msg, size_t msg_len,
+ size_t max_len, struct scmi_xfer *xfer)
+{
+ xfer->rx_raw_len = min_t(size_t, max_len,
+ msg_len >= sizeof(*msg) ?
+ msg_len - sizeof(*msg) : 0);
+
+ /* Take a copy to the rx buffer.. */
+ memcpy(xfer->rx.buf, msg->msg_payload, xfer->rx_raw_len);
In the usage throughout arm-scmi so far, scmi_desc.max_msg_size seems to
consistently refer to the payload excluding the return status.
scmi_desc.max_msg_size is the actual max_len parameter to this function.
So the logic here would reduce the maximum payload length for responses
to (scmi_desc.max_msg_size - sizeof(return status)).
+}
+
/**
* msg_fetch_response() - Fetch response SCMI payload from transport SDU.
*
@@ -94,6 +105,25 @@ void msg_fetch_response(struct scmi_msg_payld *msg, size_t len,
memcpy(xfer->rx.buf, &msg->msg_payload[1], xfer->rx.len);
}
+void msg_fetch_raw_response(struct scmi_xfer *xfer)
+{
+ __le32 *msg_payload = xfer->rx.buf;
+
+ if (xfer->rx_raw_len < sizeof(xfer->hdr.status)) {
+ xfer->rx.len = 0;
+ return;
+ }
+
+ xfer->hdr.status = le32_to_cpu(msg_payload[0]);
+ /*
+ * rx.len has been already pre-calculated by fetch_raw
+ * for the whole payload including status, so shrink it
+ */
+ xfer->rx.len = xfer->rx_raw_len - sizeof(xfer->hdr.status);
+ /* Carveout status 4-byte field */
+ memmove(xfer->rx.buf, &msg_payload[1], xfer->rx.len);
Wouldn't it be feasible to align properly in msg_fetch_raw_payload()
already? That could also get rid of .rx_raw_len.
+}
+
/**
* msg_fetch_notification() - Fetch notification payload from transport SDU.
*
@@ -111,3 +141,8 @@ void msg_fetch_notification(struct scmi_msg_payld *msg, size_t len,
/* Take a copy to the rx buffer.. */
memcpy(xfer->rx.buf, msg->msg_payload, xfer->rx.len);
}
+
+void msg_fetch_raw_notification(struct scmi_xfer *xfer)
+{
+ xfer->rx.len = xfer->rx_raw_len;
+}
diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 20972adf6dc7..4412bc590ca7 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -37,23 +37,24 @@
/**
* struct scmi_vio_channel - Transport channel information
*
- * @lock: Protects access to all members except ready.
- * @ready_lock: Protects access to ready. If required, it must be taken before
- * lock.
* @vqueue: Associated virtqueue
* @cinfo: SCMI Tx or Rx channel
* @free_list: List of unused scmi_vio_msg, maintained for Tx channels only
* @is_rx: Whether channel is an Rx channel
* @ready: Whether transport user is ready to hear about channel
+ * @lock: Protects access to all members except ready.
+ * @ready_lock: Protects access to ready. If required, it must be taken before
+ * lock.
*/
struct scmi_vio_channel {
- spinlock_t lock;
- spinlock_t ready_lock;
struct virtqueue *vqueue;
struct scmi_chan_info *cinfo;
struct list_head free_list;
- u8 is_rx;
- u8 ready;
+ bool is_rx;
+ bool ready;
+ unsigned int max_msg;
+ spinlock_t lock;
+ spinlock_t ready_lock;
};
/**
@@ -100,6 +101,73 @@ static int scmi_vio_feed_vq_rx(struct scmi_vio_channel *vioch,
return rc;
}
+static void scmi_finalize_message(struct scmi_vio_channel *vioch,
+ struct scmi_vio_msg *msg)
+{
+ unsigned long flags;
+
+ if (vioch->is_rx) {
+ scmi_vio_feed_vq_rx(vioch, msg);
+ } else {
+ spin_lock_irqsave(&vioch->lock, flags);
+ list_add(&msg->list, &vioch->free_list);
+ spin_unlock_irqrestore(&vioch->lock, flags);
+ }
+}
+
+static void scmi_process_vqueue_input(struct scmi_vio_channel *vioch,
+ struct scmi_vio_msg *msg)
+{
+ u32 msg_hdr;
+ int ret;
+ struct scmi_xfer *xfer = NULL;
+
+ msg_hdr = msg_read_header(msg->input);
+ /*
+ * Acquire from the core transport layer a currently valid xfer
+ * descriptor associated to the received msg_hdr: this could be a
+ * previously allocated xfer for responses and delayed responses to
+ * in-flight commands, or a freshly allocated new xfer for a just
+ * received notification.
+ *
+ * In case of responses and delayed_responses the acquired xfer, at
+ * the time scmi_transfer_acquire() succcessfully returns is guaranteed
+ * to be still associated with a valid (not timed-out nor stale)
+ * descriptor and proper refcounting is kept in the core along this xfer
+ * so that should the core time out the xfer concurrently to this receive
+ * path the xfer will be properly deallocated only once the last user is
+ * done with it. (and this code path will terminate normally even though
+ * all the processing related to the timed out xfer will be discarded).
+ */
This comment would better fit to scmi_transfer_acquire().
+ ret = scmi_transfer_acquire(vioch->cinfo, &msg_hdr, &xfer);
+ if (ret) {
+ dev_err(vioch->cinfo->dev,
+ "Cannot find matching xfer for hdr:0x%X\n", msg_hdr);
+ scmi_finalize_message(vioch, msg);
+ return;
+ }
+
+ dev_dbg(vioch->cinfo->dev,
+ "VQUEUE[%d] - INPUT MSG_RX_LEN:%d - HDR:0x%X TYPE:%d XFER_ID:%d XFER:%px\n",
+ vioch->vqueue->index, msg->rx_len, msg_hdr, xfer->hdr.type,
+ xfer->hdr.seq, xfer);
+
+ msg_fetch_raw_payload(msg->input, msg->rx_len,
+ scmi_virtio_desc.max_msg_size, xfer); > +
+ /* Drop processed virtio message anyway */
+ scmi_finalize_message(vioch, msg);
+
+ if (vioch->is_rx || !xfer->hdr.poll_completion)
+ scmi_rx_callback(vioch->cinfo, msg_hdr);
+ else
+ dev_warn(vioch->cinfo->dev,
+ "Polling mode NOT supported. Dropped hdr:0X%X\n",
+ msg_hdr);
+
+ scmi_transfer_release(vioch->cinfo, xfer);
+}
+
static void scmi_vio_complete_cb(struct virtqueue *vqueue)
{
unsigned long ready_flags;
@@ -138,15 +206,9 @@ static void scmi_vio_complete_cb(struct virtqueue *vqueue)
if (msg) {
msg->rx_len = length;
-
- /*
- * Hold the ready_lock during the callback to avoid
- * races when the arm-scmi driver is unbinding while
- * the virtio device is not quiesced yet.
- */
- scmi_rx_callback(vioch->cinfo,
- msg_read_header(msg->input), msg);
+ scmi_process_vqueue_input(vioch, msg);
}
+
spin_unlock_irqrestore(&vioch->ready_lock, ready_flags);
}
@@ -163,27 +225,11 @@ static vq_callback_t *scmi_vio_complete_callbacks[] = {
scmi_vio_complete_cb
};
-static unsigned int virtio_get_max_msg(bool tx,
- struct scmi_chan_info *base_cinfo)
+static unsigned int virtio_get_max_msg(struct scmi_chan_info *base_cinfo)
{
struct scmi_vio_channel *vioch = base_cinfo->transport_info;
- unsigned int ret;
- ret = virtqueue_get_vring_size(vioch->vqueue);
-
- /* Tx messages need multiple descriptors. */
- if (tx)
- ret /= DESCRIPTORS_PER_TX_MSG;
-
- if (ret > MSG_TOKEN_MAX) {
- dev_info_once(
- base_cinfo->dev,
- "Only %ld messages can be pending simultaneously, while the %s virtqueue could hold %d\n",
- MSG_TOKEN_MAX, tx ? "tx" : "rx", ret);
- ret = MSG_TOKEN_MAX;
- }
-
- return ret;
+ return vioch->max_msg;
}
static int scmi_vio_match_any_dev(struct device *dev, const void *data)
@@ -195,13 +241,14 @@ static struct virtio_driver virtio_scmi_driver; /* Forward declaration */
static int virtio_link_supplier(struct device *dev)
{
- struct device *vdev = driver_find_device(
- &virtio_scmi_driver.driver, NULL, NULL, scmi_vio_match_any_dev);
+ struct device *vdev;
+
+ vdev = driver_find_device(&virtio_scmi_driver.driver,
+ NULL, NULL, scmi_vio_match_any_dev);
if (!vdev) {
- dev_notice_once(
- dev,
- "Deferring probe after not finding a bound scmi-virtio device\n");
+ dev_notice_once(dev,
+ "Deferring probe after not finding a bound scmi-virtio device\n");
return -EPROBE_DEFER;
}
@@ -245,12 +292,8 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
struct virtio_device *vdev;
struct scmi_vio_channel *vioch;
int index = tx ? VIRTIO_SCMI_VQ_TX : VIRTIO_SCMI_VQ_RX;
- int max_msg;
int i;
- if (!virtio_chan_available(dev, index))
- return -ENODEV;
-
vdev = scmi_get_transport_info(dev);
vioch = &((struct scmi_vio_channel *)vdev->priv)[index];
@@ -259,9 +302,7 @@ static int virtio_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
vioch->cinfo = cinfo;
spin_unlock_irqrestore(&vioch->lock, flags);
- max_msg = virtio_get_max_msg(tx, cinfo);
-
- for (i = 0; i < max_msg; i++) {
+ for (i = 0; i < vioch->max_msg; i++) {
struct scmi_vio_msg *msg;
msg = devm_kzalloc(cinfo->dev, sizeof(*msg), GFP_KERNEL);
@@ -322,13 +363,6 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
int rc;
struct scmi_vio_msg *msg;
- /*
- * TODO: For now, we don't support polling. But it should not be
- * difficult to add support.
- */
- if (xfer->hdr.poll_completion)
- return -EINVAL;
-
spin_lock_irqsave(&vioch->lock, flags);
if (list_empty(&vioch->free_list)) {
@@ -351,6 +385,11 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
"%s() failed to add to virtqueue (%d)\n", __func__,
rc);
} else {
+ dev_dbg(vioch->cinfo->dev,
+ "VQUEUE[%d] - REQUEST - PROTO:0x%X ID:0x%X XFER_ID:%d XFER:%px RX_LEN:%zd\n",
+ vioch->vqueue->index, xfer->hdr.protocol_id,
+ xfer->hdr.id, xfer->hdr.seq, xfer, xfer->rx.len);
Indentation appears to be inconsistent.
+
virtqueue_kick(vioch->vqueue);
}
@@ -360,36 +399,15 @@ static int virtio_send_message(struct scmi_chan_info *cinfo,
}
static void virtio_fetch_response(struct scmi_chan_info *cinfo,
- struct scmi_xfer *xfer, void *msg_handle)
+ struct scmi_xfer *xfer)
{
- struct scmi_vio_msg *msg = msg_handle;
- struct scmi_vio_channel *vioch = cinfo->transport_info;
-
- if (!msg) {
- dev_dbg_once(&vioch->vqueue->vdev->dev,
- "Ignoring %s() call with NULL msg_handle\n",
- __func__);
- return;
- }
-
- msg_fetch_response(msg->input, msg->rx_len, xfer);
+ msg_fetch_raw_response(xfer);
}
static void virtio_fetch_notification(struct scmi_chan_info *cinfo,
- size_t max_len, struct scmi_xfer *xfer,
- void *msg_handle)
+ size_t max_len, struct scmi_xfer *xfer)
{
- struct scmi_vio_msg *msg = msg_handle;
- struct scmi_vio_channel *vioch = cinfo->transport_info;
-
- if (!msg) {
- dev_dbg_once(&vioch->vqueue->vdev->dev,
- "Ignoring %s() call with NULL msg_handle\n",
- __func__);
- return;
- }
-
- msg_fetch_notification(msg->input, msg->rx_len, max_len, xfer);
+ msg_fetch_raw_notification(xfer);
}
static void dummy_clear_channel(struct scmi_chan_info *cinfo)
@@ -402,28 +420,6 @@ static bool dummy_poll_done(struct scmi_chan_info *cinfo,
return false;
}
-static void virtio_drop_message(struct scmi_chan_info *cinfo, void *msg_handle)
-{
- unsigned long flags;
- struct scmi_vio_channel *vioch = cinfo->transport_info;
- struct scmi_vio_msg *msg = msg_handle;
-
- if (!msg) {
- dev_dbg_once(&vioch->vqueue->vdev->dev,
- "Ignoring %s() call with NULL msg_handle\n",
- __func__);
- return;
- }
-
- if (vioch->is_rx) {
- scmi_vio_feed_vq_rx(vioch, msg);
- } else {
- spin_lock_irqsave(&vioch->lock, flags);
- list_add(&msg->list, &vioch->free_list);
- spin_unlock_irqrestore(&vioch->lock, flags);
- }
-}
-
static const struct scmi_transport_ops scmi_virtio_ops = {
.link_supplier = virtio_link_supplier,
.chan_available = virtio_chan_available,
@@ -435,7 +431,6 @@ static const struct scmi_transport_ops scmi_virtio_ops = {
.fetch_notification = virtio_fetch_notification,
.clear_channel = dummy_clear_channel,
.poll_done = dummy_poll_done,
- .drop_message = virtio_drop_message,
};
static int scmi_vio_probe(struct virtio_device *vdev)
@@ -467,10 +462,26 @@ static int scmi_vio_probe(struct virtio_device *vdev)
dev_info(dev, "Found %d virtqueue(s)\n", vq_cnt);
for (i = 0; i < vq_cnt; i++) {
+ unsigned int sz;
+
spin_lock_init(&channels[i].lock);
spin_lock_init(&channels[i].ready_lock);
INIT_LIST_HEAD(&channels[i].free_list);
channels[i].vqueue = vqs[i];
+
+ sz = virtqueue_get_vring_size(channels[i].vqueue);
+ /* Tx messages need multiple descriptors. */
+ if (!channels[i].is_rx)
+ 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);
+ sz = MSG_TOKEN_MAX;
+ }
+ channels[i].max_msg = sz;
}
vdev->priv = channels;
@@ -520,4 +531,5 @@ const struct scmi_desc scmi_virtio_desc = {
.max_rx_timeout_ms = 60000, /* for non-realtime virtio devices */
.max_msg = 0, /* overridden by virtio_get_max_msg() */
.max_msg_size = VIRTIO_SCMI_MAX_MSG_SIZE,
+ .support_xfers_delegation = true,
};