Re: [PATCH v4 07/13] firmware: arm_scmi: Add notification dispatch and delivery
From: Lukasz Luba
Date: Thu Mar 12 2020 - 16:57:33 EST
On 3/12/20 7:24 PM, Cristian Marussi wrote:
On 12/03/2020 14:06, Lukasz Luba wrote:
On 3/12/20 1:51 PM, Lukasz Luba wrote:
Hi Cristian,
Hi Lukasz
just one comment below...
On 3/4/20 4:25 PM, Cristian Marussi wrote:
Add core SCMI Notifications dispatch and delivery support logic which is
able, at first, to dispatch well-known received events from the RX ISR to
the dedicated deferred worker, and then, from there, to final deliver the
events to the registered users' callbacks.
Dispatch and delivery is just added here, still not enabled.
Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
---
V3 --> V4
- dispatcher now handles dequeuing of events in chunks (header+payload):
ÂÂ handling of these in_flight events let us remove one unneeded memcpy
ÂÂ on RX interrupt path (scmi_notify)
- deferred dispatcher now access their own per-protocol handlers' table
ÂÂ reducing locking contention on the RX path
V2 --> V3
- exposing wq in sysfs via WQ_SYSFS
V1 --> V2
- splitted out of V1 patch 04
- moved from IDR maps to real HashTables to store event_handlers
- simplified delivery logic
---
 drivers/firmware/arm_scmi/notify.c | 334 ++++++++++++++++++++++++++++-
 drivers/firmware/arm_scmi/notify.h | 9 +
 2 files changed, 342 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/notify.c
b/drivers/firmware/arm_scmi/notify.c
[snip]
+
+/**
+ * scmi_notify - Queues a notification for further deferred processing
+ *
+ * This is called in interrupt context to queue a received event for
+ * deferred processing.
+ *
+ * @handle: The handle identifying the platform instance from which the
+ *ÂÂÂÂÂÂÂ dispatched event is generated
+ * @proto_id: Protocol ID
+ * @evt_id: Event ID (msgID)
+ * @buf: Event Message Payload (without the header)
+ * @len: Event Message Payload size
+ * @ts: RX Timestamp in nanoseconds (boottime)
+ *
+ * Return: 0 on Success
+ */
+int scmi_notify(const struct scmi_handle *handle, u8 proto_id, u8
evt_id,
+ÂÂÂÂÂÂÂ const void *buf, size_t len, u64 ts)
+{
+ÂÂÂ struct scmi_registered_event *r_evt;
+ÂÂÂ struct scmi_event_header eh;
+ÂÂÂ struct scmi_notify_instance *ni = handle->notify_priv;
+
+ÂÂÂ /* Ensure atomic value is updated */
+ÂÂÂ smp_mb__before_atomic();
+ÂÂÂ if (unlikely(!atomic_read(&ni->enabled)))
+ÂÂÂÂÂÂÂ return 0;
+
+ÂÂÂ r_evt = SCMI_GET_REVT(ni, proto_id, evt_id);
+ÂÂÂ if (unlikely(!r_evt))
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ if (unlikely(len > r_evt->evt->max_payld_sz)) {
+ÂÂÂÂÂÂÂ pr_err("SCMI Notifications: discard badly sized message\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+ÂÂÂ if (unlikely(kfifo_avail(&r_evt->proto->equeue.kfifo) <
+ÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(eh) + len)) {
+ÂÂÂÂÂÂÂ pr_warn("SCMI Notifications: queue full dropping proto_id:%d
evt_id:%d ts:%lld\n",
+ÂÂÂÂÂÂÂÂÂÂÂ proto_id, evt_id, ts);
+ÂÂÂÂÂÂÂ return -ENOMEM;
+ÂÂÂ }
+
+ÂÂÂ eh.timestamp = ts;
+ÂÂÂ eh.evt_id = evt_id;
+ÂÂÂ eh.payld_sz = len;
+ÂÂÂ kfifo_in(&r_evt->proto->equeue.kfifo, &eh, sizeof(eh));
+ÂÂÂ kfifo_in(&r_evt->proto->equeue.kfifo, buf, len);
+ÂÂÂ queue_work(r_evt->proto->equeue.wq,
+ÂÂÂÂÂÂÂÂÂÂ &r_evt->proto->equeue.notify_work);
Is it safe to ignore the return value from the queue_work here?
and also from the kfifo_in
kfifo_in returns the number of effectively written bytes (using __kfifo_in),
possibly capped to the effectively maximum available space in the fifo, BUT since I
absolutely cannot afford to write an incomplete/truncated event into the queue, I check
that in advance and backout on queue full:
if (unlikely(kfifo_avail(&r_evt->proto->equeue.kfifo) < sizeof(eh) + len)) {
return -ENOMEM;
and given that the ISR scmi_notify() is the only possible writer on this queue
Yes, your are right, no other IRQ will show up for this channel till
we exit mailbox rx callback and clean the bits.
I can be sure that the kfifo_in() will succeed in writing the required number of
bytes after the above check...so I don't need to check the return value.
Regards
Cristian
Regards,
Lukasz