Re: [PATCH v4 07/13] firmware: arm_scmi: Add notification dispatch and delivery

[Cristian Marussi]

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
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
>>
>>