Add core SCMI Notifications protocol-registration support: allow protocols
to register their own set of supported events, during their initialization
phase. Notification core can track multiple platform instances by their
handles.
Signed-off-by: Cristian Marussi <[email protected]>
---
V3 --> V4
- removed scratch ISR buffer, move scratch BH buffer into protocol
descriptor
- converted registered_protocols and registered_events from hashtables
into bare fixed-sized arrays
- removed unregister protocols' routines (never called really)
V2 --> V3
- added scmi_notify_instance to track target platform instance
V1 --> V2
- splitted out of V1 patch 04
- moved from IDR maps to real HashTables to store events
- scmi_notifications_initialized is now an atomic_t
- reviewed protocol registration/unregistration to use devres
- fixed:
drivers/firmware/arm_scmi/notify.c:483:18-23: ERROR:
reference preceded by free on line 482
Reported-by: kbuild test robot <[email protected]>
Reported-by: Julia Lawall <[email protected]>
---
drivers/firmware/arm_scmi/Makefile | 2 +-
drivers/firmware/arm_scmi/common.h | 4 +
drivers/firmware/arm_scmi/notify.c | 439 +++++++++++++++++++++++++++++
drivers/firmware/arm_scmi/notify.h | 57 ++++
include/linux/scmi_protocol.h | 9 +
5 files changed, 510 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/arm_scmi/notify.c
create mode 100644 drivers/firmware/arm_scmi/notify.h
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 6694d0d908d6..24a03a36aee4 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-y = scmi-bus.o scmi-driver.o scmi-protocols.o scmi-transport.o
scmi-bus-y = bus.o
-scmi-driver-y = driver.o
+scmi-driver-y = driver.o notify.o
scmi-transport-y = mailbox.o shmem.o
scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 3c2e5d0d7b68..2106c35195ce 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -6,6 +6,8 @@
*
* Copyright (C) 2018 ARM Ltd.
*/
+#ifndef _SCMI_COMMON_H
+#define _SCMI_COMMON_H
#include <linux/bitfield.h>
#include <linux/completion.h>
@@ -232,3 +234,5 @@ void shmem_fetch_notification(struct scmi_shared_mem __iomem *shmem,
void shmem_clear_notification(struct scmi_shared_mem __iomem *shmem);
bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
struct scmi_xfer *xfer);
+
+#endif /* _SCMI_COMMON_H */
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
new file mode 100644
index 000000000000..31e49cb7d88e
--- /dev/null
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -0,0 +1,439 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Notification support
+ *
+ * Copyright (C) 2020 ARM Ltd.
+ *
+ * SCMI Protocol specification allows the platform to signal events to
+ * interested agents via notification messages: this is an implementation
+ * of the dispatch and delivery of such notifications to the interested users
+ * inside the Linux kernel.
+ *
+ * An SCMI Notification core instance is initialized for each active platform
+ * instance identified by the means of the usual @scmi_handle.
+ *
+ * Each SCMI Protocol implementation, during its initialization, registers with
+ * this core its set of supported events using @scmi_register_protocol_events():
+ * all the needed descriptors are stored in the @registered_protocols and
+ * @registered_events arrays.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/atomic.h>
+#include <linux/bug.h>
+#include <linux/compiler.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/mutex.h>
+#include <linux/refcount.h>
+#include <linux/scmi_protocol.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "notify.h"
+
+#define SCMI_MAX_PROTO 256
+#define SCMI_ALL_SRC_IDS 0xffffUL
+/*
+ * Builds an unsigned 32bit key from the given input tuple to be used
+ * as a key in hashtables.
+ */
+#define MAKE_HASH_KEY(p, e, s) \
+ ((u32)(((p) << 24) | ((e) << 16) | ((s) & SCMI_ALL_SRC_IDS)))
+
+#define MAKE_ALL_SRCS_KEY(p, e) \
+ MAKE_HASH_KEY((p), (e), SCMI_ALL_SRC_IDS)
+
+struct scmi_registered_protocol_events_desc;
+
+/**
+ * scmi_notify_instance - Represents an instance of the notification core
+ *
+ * Each platform instance, represented by a handle, has its own instance of
+ * the notification subsystem represented by this structure.
+ *
+ * @gid: GroupID used for devres
+ * @handle: A reference to the platform instance
+ * @initialized: A flag that indicates if the core resources have been allocated
+ * and protocols are allowed to register their supported events
+ * @enabled: A flag to indicate events can be enabled and start flowing
+ * @registered_protocols: An statically allocated array containing pointers to
+ * all the registered protocol-level specific information
+ * related to events' handling
+ */
+struct scmi_notify_instance {
+ void *gid;
+ struct scmi_handle *handle;
+ atomic_t initialized;
+ atomic_t enabled;
+ struct scmi_registered_protocol_events_desc **registered_protocols;
+};
+
+/**
+ * events_queue - Describes a queue and its associated worker
+ *
+ * Each protocol has its own dedicated events_queue descriptor.
+ *
+ * @sz: Size in bytes of the related kfifo
+ * @qbuf: Pre-allocated buffer of @sz bytes to be used by the kfifo
+ * @kfifo: A dedicated Kernel kfifo descriptor
+ */
+struct events_queue {
+ size_t sz;
+ u8 *qbuf;
+ struct kfifo kfifo;
+};
+
+/**
+ * scmi_event_header - A utility header
+ *
+ * This header is prepended to each received event message payload before
+ * queueing it on the related events_queue.
+ *
+ * @timestamp: The timestamp, in nanoseconds (boottime), which was associated
+ * to this event as soon as it entered the SCMI RX ISR
+ * @evt_id: Event ID (corresponds to the Event MsgID for this Protocol)
+ * @payld_sz: Effective size of the embedded message payload which follows
+ * @payld: A reference to the embedded event payload
+ */
+struct scmi_event_header {
+ u64 timestamp;
+ u8 evt_id;
+ size_t payld_sz;
+ u8 payld[];
+} __packed;
+
+struct scmi_registered_event;
+
+/**
+ * scmi_registered_protocol_events_desc - Protocol Specific information
+ *
+ * All protocols that registers at least one event have their protocol-specific
+ * information stored here, together with the embedded allocated events_queue.
+ * These descriptors are stored in the @registered_protocols array at protocol
+ * registration time.
+ *
+ * Once these descriptors are successfully registered, they are NEVER again
+ * removed or modified since protocols do not unregister ever, so that once we
+ * safely grab a NON-NULL reference from the array we can keep it and use it.
+ *
+ * @id: Protocol ID
+ * @ops: Protocol specific and event-related operations
+ * @equeue: The embedded per-protocol events_queue
+ * @ni: A reference to the initialized instance descriptor
+ * @eh: A reference to pre-allocated buffer to be used as a scratch area by the
+ * deferred worker when fetching data from the kfifo
+ * @eh_sz: Size of the pre-allocated buffer @eh
+ * @in_flight: A reference to an in flight @scmi_registered_event
+ * @num_events: Number of events in @registered_events
+ * @registered_events: A dynamically allocated array holding all the registered
+ * events' descriptors, whose fixed-size is determined at
+ * compile time.
+ */
+struct scmi_registered_protocol_events_desc {
+ u8 id;
+ const struct scmi_protocol_event_ops *ops;
+ struct events_queue equeue;
+ struct scmi_notify_instance *ni;
+ struct scmi_event_header *eh;
+ size_t eh_sz;
+ void *in_flight;
+ int num_events;
+ struct scmi_registered_event **registered_events;
+};
+
+/**
+ * scmi_registered_event - Event Specific Information
+ *
+ * All registered events are represented by one of these structures that are
+ * stored in the @registered_events array at protocol registration time.
+ *
+ * Once these descriptors are successfully registered, they are NEVER again
+ * removed or modified since protocols do not unregister ever, so that once we
+ * safely grab a NON-NULL reference from the table we can keep it and use it.
+ *
+ * @proto: A reference to the associated protocol descriptor
+ * @evt: A reference to the associated event descriptor (as provided at
+ * registration time)
+ * @report: A pre-allocated buffer used by the deferred worker to fill a
+ * customized event report
+ * @num_sources: The number of possible sources for this event as stated at
+ * events' registration time
+ * @sources: A reference to a dynamically allocated array used to refcount the
+ * events' enable requests for all the existing sources
+ * @sources_mtx: A mutex to serialize the access to @sources
+ */
+struct scmi_registered_event {
+ struct scmi_registered_protocol_events_desc *proto;
+ const struct scmi_event *evt;
+ void *report;
+ u32 num_sources;
+ refcount_t *sources;
+ struct mutex sources_mtx;
+};
+
+/**
+ * scmi_initialize_events_queue - Allocate/Initialize a kfifo buffer
+ *
+ * Allocate a buffer for the kfifo and initialize it.
+ *
+ * @ni: A reference to the notification instance to use
+ * @equeue: The events_queue to initialize
+ * @sz: Size of the kfifo buffer to allocate
+ *
+ * Return: 0 on Success
+ */
+static int scmi_initialize_events_queue(struct scmi_notify_instance *ni,
+ struct events_queue *equeue, size_t sz)
+{
+ equeue->qbuf = devm_kzalloc(ni->handle->dev, sz, GFP_KERNEL);
+ if (!equeue->qbuf)
+ return -ENOMEM;
+ equeue->sz = sz;
+
+ return kfifo_init(&equeue->kfifo, equeue->qbuf, equeue->sz);
+}
+
+/**
+ * scmi_allocate_registered_protocol_desc - Allocate a registered protocol
+ * events' descriptor
+ *
+ * It is supposed to be called only once for each protocol at protocol
+ * initialization time, so it warns if the requested protocol is found
+ * already registered.
+ *
+ * @ni: A reference to the notification instance to use
+ * @proto_id: Protocol ID
+ * @queue_sz: Size of the associated queue to allocate
+ * @eh_sz: Size of the event header scratch area to pre-allocate
+ * @num_events: Number of events to support (size of @registered_events)
+ * @ops: Pointer to a struct holding references to protocol specific helpers
+ * needed during events handling
+ *
+ * Returns the allocated and registered descriptor on Success
+ */
+static struct scmi_registered_protocol_events_desc *
+scmi_allocate_registered_protocol_desc(struct scmi_notify_instance *ni,
+ u8 proto_id, size_t queue_sz,
+ size_t eh_sz, int num_events,
+ const struct scmi_protocol_event_ops *ops)
+{
+ int ret;
+ struct scmi_registered_protocol_events_desc *pd;
+
+ pd = READ_ONCE(ni->registered_protocols[proto_id]);
+ if (pd) {
+ WARN_ON(1);
+ return ERR_PTR(-EINVAL);
+ }
+
+ pd = devm_kzalloc(ni->handle->dev, sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return ERR_PTR(-ENOMEM);
+ pd->id = proto_id;
+ pd->ops = ops;
+ pd->ni = ni;
+
+ ret = scmi_initialize_events_queue(ni, &pd->equeue, queue_sz);
+ if (ret)
+ return ERR_PTR(ret);
+
+ pd->eh = devm_kzalloc(ni->handle->dev, eh_sz, GFP_KERNEL);
+ if (!pd->eh)
+ return ERR_PTR(-ENOMEM);
+ pd->eh_sz = eh_sz;
+
+ pd->registered_events = devm_kcalloc(ni->handle->dev, num_events,
+ sizeof(char *), GFP_KERNEL);
+ if (!pd->registered_events)
+ return ERR_PTR(-ENOMEM);
+ pd->num_events = num_events;
+
+ return pd;
+}
+
+/**
+ * scmi_register_protocol_events - Register Protocol Events with the core
+ *
+ * Used by SCMI Protocols initialization code to register with the notification
+ * core the list of supported events and their descriptors: takes care to
+ * pre-allocate and store all needed descriptors, scratch buffers and event
+ * queues.
+ *
+ * @handle: The handle identifying the platform instance against which the
+ * the protocol's events are registered
+ * @proto_id: Protocol ID
+ * @queue_sz: Size in bytes of the associated queue to be allocated
+ * @ops: Protocol specific event-related operations
+ * @evt: Event descriptor array
+ * @num_events: Number of events in @evt array
+ * @num_sources: Number of possible sources for this protocol on this
+ * platform.
+ *
+ * Return: 0 on Success
+ */
+int scmi_register_protocol_events(const struct scmi_handle *handle,
+ u8 proto_id, size_t queue_sz,
+ const struct scmi_protocol_event_ops *ops,
+ const struct scmi_event *evt, int num_events,
+ int num_sources)
+{
+ int i;
+ size_t payld_sz = 0;
+ struct scmi_registered_protocol_events_desc *pd;
+ struct scmi_notify_instance *ni = handle->notify_priv;
+
+ if (!ops || !evt || proto_id >= SCMI_MAX_PROTO)
+ return -EINVAL;
+
+ /* Ensure atomic value is updated */
+ smp_mb__before_atomic();
+ if (unlikely(!ni || !atomic_read(&ni->initialized)))
+ return -EAGAIN;
+
+ /* Attach to the notification main devres group */
+ if (!devres_open_group(ni->handle->dev, ni->gid, GFP_KERNEL))
+ return -ENOMEM;
+
+ for (i = 0; i < num_events; i++)
+ payld_sz = max_t(size_t, payld_sz, evt[i].max_payld_sz);
+ pd = scmi_allocate_registered_protocol_desc(ni, proto_id, queue_sz,
+ sizeof(struct scmi_event_header) + payld_sz,
+ num_events, ops);
+ if (IS_ERR(pd))
+ goto err;
+
+ for (i = 0; i < num_events; i++, evt++) {
+ struct scmi_registered_event *r_evt;
+
+ r_evt = devm_kzalloc(ni->handle->dev, sizeof(*r_evt),
+ GFP_KERNEL);
+ if (!r_evt)
+ goto err;
+ r_evt->proto = pd;
+ r_evt->evt = evt;
+
+ r_evt->sources = devm_kcalloc(ni->handle->dev, num_sources,
+ sizeof(refcount_t), GFP_KERNEL);
+ if (!r_evt->sources)
+ goto err;
+ r_evt->num_sources = num_sources;
+ mutex_init(&r_evt->sources_mtx);
+
+ r_evt->report = devm_kzalloc(ni->handle->dev,
+ evt->max_report_sz, GFP_KERNEL);
+ if (!r_evt->report)
+ goto err;
+
+ WRITE_ONCE(pd->registered_events[i], r_evt);
+ pr_info("SCMI Notifications: registered event - %X\n",
+ MAKE_ALL_SRCS_KEY(r_evt->proto->id, r_evt->evt->id));
+ }
+
+ /* Register protocol and events...it will never be removed */
+ WRITE_ONCE(ni->registered_protocols[proto_id], pd);
+
+ devres_close_group(ni->handle->dev, ni->gid);
+
+ return 0;
+
+err:
+ pr_warn("SCMI Notifications - Proto:%X - Registration Failed !\n",
+ proto_id);
+ /* A failing protocol registration does not trigger full failure */
+ devres_close_group(ni->handle->dev, ni->gid);
+
+ return -ENOMEM;
+}
+
+/**
+ * scmi_notification_init - Initializes Notification Core Support
+ *
+ * This function lays out all the basic resources needed by the notification
+ * core instance identified by the provided handle: once done, all of the
+ * SCMI Protocols can register their events with the core during their own
+ * initializations.
+ *
+ * Note that failing to initialize the core notifications support does not
+ * cause the whole SCMI Protocols stack to fail its initialization.
+ *
+ * SCMI Notification Initialization happens in 2 steps:
+ *
+ * - initialization: basic common allocations (this function) -> .initialized
+ * - registration: protocols asynchronously come into life and registers their
+ * own supported list of events with the core; this causes
+ * further per-protocol allocations.
+ *
+ * Any user's callback registration attempt, referring a still not registered
+ * event, will be registered as pending and finalized later (if possible)
+ * by @scmi_protocols_late_init work.
+ * This allows for lazy initialization of SCMI Protocols due to late (or
+ * missing) SCMI drivers' modules loading.
+ *
+ * @handle: The handle identifying the platform instance to initialize
+ *
+ * Return: 0 on Success
+ */
+int scmi_notification_init(struct scmi_handle *handle)
+{
+ void *gid;
+ struct scmi_notify_instance *ni;
+
+ gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
+ if (!gid)
+ return -ENOMEM;
+
+ ni = devm_kzalloc(handle->dev, sizeof(*ni), GFP_KERNEL);
+ if (!ni)
+ goto err;
+
+ ni->gid = gid;
+ ni->handle = handle;
+
+ ni->registered_protocols = devm_kcalloc(handle->dev, SCMI_MAX_PROTO,
+ sizeof(char *), GFP_KERNEL);
+ if (!ni->registered_protocols)
+ goto err;
+
+ handle->notify_priv = ni;
+
+ atomic_set(&ni->initialized, 1);
+ atomic_set(&ni->enabled, 1);
+ /* Ensure atomic values are updated */
+ smp_mb__after_atomic();
+
+ pr_info("SCMI Notifications Core Initialized.\n");
+
+ devres_close_group(handle->dev, ni->gid);
+
+ return 0;
+
+err:
+ pr_warn("SCMI Notifications - Initialization Failed.\n");
+ devres_release_group(handle->dev, NULL);
+ return -ENOMEM;
+}
+
+/**
+ * scmi_notification_exit - Shutdown and clean Notification core
+ *
+ * @handle: The handle identifying the platform instance to shutdown
+ */
+void scmi_notification_exit(struct scmi_handle *handle)
+{
+ struct scmi_notify_instance *ni = handle->notify_priv;
+
+ if (unlikely(!ni || !atomic_read(&ni->initialized)))
+ return;
+
+ atomic_set(&ni->enabled, 0);
+ /* Ensure atomic values are updated */
+ smp_mb__after_atomic();
+
+ devres_release_group(ni->handle->dev, ni->gid);
+
+ pr_info("SCMI Notifications Core Shutdown.\n");
+}
diff --git a/drivers/firmware/arm_scmi/notify.h b/drivers/firmware/arm_scmi/notify.h
new file mode 100644
index 000000000000..a7ece64e8842
--- /dev/null
+++ b/drivers/firmware/arm_scmi/notify.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * System Control and Management Interface (SCMI) Message Protocol
+ * notification header file containing some definitions, structures
+ * and function prototypes related to SCMI Notification handling.
+ *
+ * Copyright (C) 2019 ARM Ltd.
+ */
+#ifndef _SCMI_NOTIFY_H
+#define _SCMI_NOTIFY_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+
+/**
+ * scmi_event - Describes an event to be supported
+ *
+ * Each SCMI protocol, during its initialization phase, can describe the events
+ * it wishes to support in a few struct scmi_event and pass them to the core
+ * using scmi_register_protocol_events().
+ *
+ * @id: Event ID
+ * @max_payld_sz: Max possible size for the payload of a notif msg of this kind
+ * @max_report_sz: Max possible size for the report of a notif msg of this kind
+ */
+struct scmi_event {
+ u8 id;
+ size_t max_payld_sz;
+ size_t max_report_sz;
+
+};
+
+/**
+ * scmi_protocol_event_ops - Helpers called by notification core.
+ *
+ * These are called only in process context.
+ *
+ * @set_notify_enabled: Enable/disable the required evt_id/src_id notifications
+ * using the proper custom protocol commands.
+ * Return true if at least one the required src_id
+ * has been successfully enabled/disabled
+ */
+struct scmi_protocol_event_ops {
+ bool (*set_notify_enabled)(const struct scmi_handle *handle,
+ u8 evt_id, u32 src_id, bool enabled);
+};
+
+int scmi_notification_init(struct scmi_handle *handle);
+void scmi_notification_exit(struct scmi_handle *handle);
+
+int scmi_register_protocol_events(const struct scmi_handle *handle,
+ u8 proto_id, size_t queue_sz,
+ const struct scmi_protocol_event_ops *ops,
+ const struct scmi_event *evt, int num_events,
+ int num_sources);
+
+#endif /* _SCMI_NOTIFY_H */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 5c873a59b387..0679f10ab05e 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -4,6 +4,10 @@
*
* Copyright (C) 2018 ARM Ltd.
*/
+
+#ifndef _LINUX_SCMI_PROTOCOL_H
+#define _LINUX_SCMI_PROTOCOL_H
+
#include <linux/device.h>
#include <linux/types.h>
@@ -227,6 +231,8 @@ struct scmi_reset_ops {
* protocol(for internal use only)
* @reset_priv: pointer to private data structure specific to reset
* protocol(for internal use only)
+ * @notify_priv: pointer to private data structure specific to notifications
+ * (for internal use only)
*/
struct scmi_handle {
struct device *dev;
@@ -242,6 +248,7 @@ struct scmi_handle {
void *power_priv;
void *sensor_priv;
void *reset_priv;
+ void *notify_priv;
};
enum scmi_std_protocol {
@@ -319,3 +326,5 @@ static inline void scmi_driver_unregister(struct scmi_driver *driver) {}
typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *);
int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn);
void scmi_protocol_unregister(int protocol_id);
+
+#endif /* _LINUX_SCMI_PROTOCOL_H */
--
2.17.1
On Wed, 4 Mar 2020 16:25:50 +0000
Cristian Marussi <[email protected]> wrote:
> Add core SCMI Notifications protocol-registration support: allow protocols
> to register their own set of supported events, during their initialization
> phase. Notification core can track multiple platform instances by their
> handles.
>
> Signed-off-by: Cristian Marussi <[email protected]>
Hi.
A few minor things inline. Fairly sure kernel-doc needs
struct before the heading for each structure comment block.
Also, the events queue init looks like it could just be done with
a kfifo_alloc call. Perhaps that makes sense given later patches...
Thanks,
Jonathan
> ---
> V3 --> V4
> - removed scratch ISR buffer, move scratch BH buffer into protocol
> descriptor
> - converted registered_protocols and registered_events from hashtables
> into bare fixed-sized arrays
> - removed unregister protocols' routines (never called really)
> V2 --> V3
> - added scmi_notify_instance to track target platform instance
> V1 --> V2
> - splitted out of V1 patch 04
> - moved from IDR maps to real HashTables to store events
> - scmi_notifications_initialized is now an atomic_t
> - reviewed protocol registration/unregistration to use devres
> - fixed:
> drivers/firmware/arm_scmi/notify.c:483:18-23: ERROR:
> reference preceded by free on line 482
>
> Reported-by: kbuild test robot <[email protected]>
> Reported-by: Julia Lawall <[email protected]>
> ---
> drivers/firmware/arm_scmi/Makefile | 2 +-
> drivers/firmware/arm_scmi/common.h | 4 +
> drivers/firmware/arm_scmi/notify.c | 439 +++++++++++++++++++++++++++++
> drivers/firmware/arm_scmi/notify.h | 57 ++++
> include/linux/scmi_protocol.h | 9 +
> 5 files changed, 510 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/arm_scmi/notify.c
> create mode 100644 drivers/firmware/arm_scmi/notify.h
>
> diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> index 6694d0d908d6..24a03a36aee4 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> obj-y = scmi-bus.o scmi-driver.o scmi-protocols.o scmi-transport.o
> scmi-bus-y = bus.o
> -scmi-driver-y = driver.o
> +scmi-driver-y = driver.o notify.o
> scmi-transport-y = mailbox.o shmem.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 3c2e5d0d7b68..2106c35195ce 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -6,6 +6,8 @@
> *
> * Copyright (C) 2018 ARM Ltd.
> */
> +#ifndef _SCMI_COMMON_H
> +#define _SCMI_COMMON_H
>
> #include <linux/bitfield.h>
> #include <linux/completion.h>
> @@ -232,3 +234,5 @@ void shmem_fetch_notification(struct scmi_shared_mem __iomem *shmem,
> void shmem_clear_notification(struct scmi_shared_mem __iomem *shmem);
> bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem,
> struct scmi_xfer *xfer);
> +
> +#endif /* _SCMI_COMMON_H */
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> new file mode 100644
> index 000000000000..31e49cb7d88e
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -0,0 +1,439 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Notification support
> + *
> + * Copyright (C) 2020 ARM Ltd.
> + *
> + * SCMI Protocol specification allows the platform to signal events to
> + * interested agents via notification messages: this is an implementation
> + * of the dispatch and delivery of such notifications to the interested users
> + * inside the Linux kernel.
> + *
> + * An SCMI Notification core instance is initialized for each active platform
> + * instance identified by the means of the usual @scmi_handle.
> + *
> + * Each SCMI Protocol implementation, during its initialization, registers with
> + * this core its set of supported events using @scmi_register_protocol_events():
> + * all the needed descriptors are stored in the @registered_protocols and
> + * @registered_events arrays.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/atomic.h>
> +#include <linux/bug.h>
> +#include <linux/compiler.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/kfifo.h>
> +#include <linux/mutex.h>
> +#include <linux/refcount.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include "notify.h"
> +
> +#define SCMI_MAX_PROTO 256
> +#define SCMI_ALL_SRC_IDS 0xffffUL
> +/*
> + * Builds an unsigned 32bit key from the given input tuple to be used
> + * as a key in hashtables.
> + */
> +#define MAKE_HASH_KEY(p, e, s) \
> + ((u32)(((p) << 24) | ((e) << 16) | ((s) & SCMI_ALL_SRC_IDS)))
> +
> +#define MAKE_ALL_SRCS_KEY(p, e) \
> + MAKE_HASH_KEY((p), (e), SCMI_ALL_SRC_IDS)
> +
> +struct scmi_registered_protocol_events_desc;
> +
> +/**
> + * scmi_notify_instance - Represents an instance of the notification core
> + *
> + * Each platform instance, represented by a handle, has its own instance of
> + * the notification subsystem represented by this structure.
> + *
> + * @gid: GroupID used for devres
> + * @handle: A reference to the platform instance
> + * @initialized: A flag that indicates if the core resources have been allocated
> + * and protocols are allowed to register their supported events
> + * @enabled: A flag to indicate events can be enabled and start flowing
> + * @registered_protocols: An statically allocated array containing pointers to
> + * all the registered protocol-level specific information
> + * related to events' handling
> + */
> +struct scmi_notify_instance {
> + void *gid;
> + struct scmi_handle *handle;
> + atomic_t initialized;
> + atomic_t enabled;
> + struct scmi_registered_protocol_events_desc **registered_protocols;
> +};
> +
> +/**
> + * events_queue - Describes a queue and its associated worker
I guess this might become clear later, but right now this just looks like
we are open code what could be handled automatically by just using
kfifo_alloc
> + *
> + * Each protocol has its own dedicated events_queue descriptor.
> + *
> + * @sz: Size in bytes of the related kfifo
> + * @qbuf: Pre-allocated buffer of @sz bytes to be used by the kfifo
> + * @kfifo: A dedicated Kernel kfifo descriptor
> + */
> +struct events_queue {
> + size_t sz;
> + u8 *qbuf;
> + struct kfifo kfifo;
> +};
> +
> +/**
> + * scmi_event_header - A utility header
struct scmi...
> + *
> + * This header is prepended to each received event message payload before
> + * queueing it on the related events_queue.
> + *
> + * @timestamp: The timestamp, in nanoseconds (boottime), which was associated
> + * to this event as soon as it entered the SCMI RX ISR
> + * @evt_id: Event ID (corresponds to the Event MsgID for this Protocol)
> + * @payld_sz: Effective size of the embedded message payload which follows
> + * @payld: A reference to the embedded event payload
> + */
> +struct scmi_event_header {
> + u64 timestamp;
> + u8 evt_id;
> + size_t payld_sz;
> + u8 payld[];
> +} __packed;
> +
> +struct scmi_registered_event;
> +
> +/**
> + * scmi_registered_protocol_events_desc - Protocol Specific information
> + *
> + * All protocols that registers at least one event have their protocol-specific
> + * information stored here, together with the embedded allocated events_queue.
> + * These descriptors are stored in the @registered_protocols array at protocol
> + * registration time.
> + *
> + * Once these descriptors are successfully registered, they are NEVER again
> + * removed or modified since protocols do not unregister ever, so that once we
> + * safely grab a NON-NULL reference from the array we can keep it and use it.
> + *
> + * @id: Protocol ID
> + * @ops: Protocol specific and event-related operations
> + * @equeue: The embedded per-protocol events_queue
> + * @ni: A reference to the initialized instance descriptor
> + * @eh: A reference to pre-allocated buffer to be used as a scratch area by the
> + * deferred worker when fetching data from the kfifo
> + * @eh_sz: Size of the pre-allocated buffer @eh
> + * @in_flight: A reference to an in flight @scmi_registered_event
> + * @num_events: Number of events in @registered_events
> + * @registered_events: A dynamically allocated array holding all the registered
> + * events' descriptors, whose fixed-size is determined at
> + * compile time.
> + */
> +struct scmi_registered_protocol_events_desc {
> + u8 id;
> + const struct scmi_protocol_event_ops *ops;
> + struct events_queue equeue;
> + struct scmi_notify_instance *ni;
> + struct scmi_event_header *eh;
> + size_t eh_sz;
> + void *in_flight;
> + int num_events;
> + struct scmi_registered_event **registered_events;
> +};
> +
> +/**
> + * scmi_registered_event - Event Specific Information
struct scmi_registered_event - Event...
> + *
> + * All registered events are represented by one of these structures that are
> + * stored in the @registered_events array at protocol registration time.
> + *
> + * Once these descriptors are successfully registered, they are NEVER again
> + * removed or modified since protocols do not unregister ever, so that once we
> + * safely grab a NON-NULL reference from the table we can keep it and use it.
> + *
> + * @proto: A reference to the associated protocol descriptor
> + * @evt: A reference to the associated event descriptor (as provided at
> + * registration time)
> + * @report: A pre-allocated buffer used by the deferred worker to fill a
> + * customized event report
> + * @num_sources: The number of possible sources for this event as stated at
> + * events' registration time
> + * @sources: A reference to a dynamically allocated array used to refcount the
> + * events' enable requests for all the existing sources
> + * @sources_mtx: A mutex to serialize the access to @sources
> + */
> +struct scmi_registered_event {
> + struct scmi_registered_protocol_events_desc *proto;
> + const struct scmi_event *evt;
> + void *report;
> + u32 num_sources;
> + refcount_t *sources;
> + struct mutex sources_mtx;
> +};
> +
> +/**
> + * scmi_initialize_events_queue - Allocate/Initialize a kfifo buffer
> + *
> + * Allocate a buffer for the kfifo and initialize it.
> + *
> + * @ni: A reference to the notification instance to use
> + * @equeue: The events_queue to initialize
> + * @sz: Size of the kfifo buffer to allocate
> + *
> + * Return: 0 on Success
> + */
> +static int scmi_initialize_events_queue(struct scmi_notify_instance *ni,
> + struct events_queue *equeue, size_t sz)
> +{
> + equeue->qbuf = devm_kzalloc(ni->handle->dev, sz, GFP_KERNEL);
> + if (!equeue->qbuf)
> + return -ENOMEM;
> + equeue->sz = sz;
> +
> + return kfifo_init(&equeue->kfifo, equeue->qbuf, equeue->sz);
This seems like a slightly odd dance. Why not use kfifo_alloc?
If it's because of the lack of devm_kfifo_alloc, maybe use a devm_add_action_or_reset
to handle that.
> +}
> +
> +/**
> + * scmi_allocate_registered_protocol_desc - Allocate a registered protocol
> + * events' descriptor
> + *
> + * It is supposed to be called only once for each protocol at protocol
> + * initialization time, so it warns if the requested protocol is found
> + * already registered.
> + *
> + * @ni: A reference to the notification instance to use
> + * @proto_id: Protocol ID
> + * @queue_sz: Size of the associated queue to allocate
> + * @eh_sz: Size of the event header scratch area to pre-allocate
> + * @num_events: Number of events to support (size of @registered_events)
> + * @ops: Pointer to a struct holding references to protocol specific helpers
> + * needed during events handling
> + *
> + * Returns the allocated and registered descriptor on Success
> + */
> +static struct scmi_registered_protocol_events_desc *
> +scmi_allocate_registered_protocol_desc(struct scmi_notify_instance *ni,
> + u8 proto_id, size_t queue_sz,
> + size_t eh_sz, int num_events,
> + const struct scmi_protocol_event_ops *ops)
> +{
> + int ret;
> + struct scmi_registered_protocol_events_desc *pd;
> +
> + pd = READ_ONCE(ni->registered_protocols[proto_id]);
> + if (pd) {
> + WARN_ON(1);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + pd = devm_kzalloc(ni->handle->dev, sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return ERR_PTR(-ENOMEM);
> + pd->id = proto_id;
> + pd->ops = ops;
> + pd->ni = ni;
> +
> + ret = scmi_initialize_events_queue(ni, &pd->equeue, queue_sz);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + pd->eh = devm_kzalloc(ni->handle->dev, eh_sz, GFP_KERNEL);
> + if (!pd->eh)
> + return ERR_PTR(-ENOMEM);
> + pd->eh_sz = eh_sz;
> +
> + pd->registered_events = devm_kcalloc(ni->handle->dev, num_events,
> + sizeof(char *), GFP_KERNEL);
> + if (!pd->registered_events)
> + return ERR_PTR(-ENOMEM);
> + pd->num_events = num_events;
> +
> + return pd;
> +}
> +
> +/**
> + * scmi_register_protocol_events - Register Protocol Events with the core
> + *
> + * Used by SCMI Protocols initialization code to register with the notification
> + * core the list of supported events and their descriptors: takes care to
> + * pre-allocate and store all needed descriptors, scratch buffers and event
> + * queues.
> + *
> + * @handle: The handle identifying the platform instance against which the
> + * the protocol's events are registered
> + * @proto_id: Protocol ID
> + * @queue_sz: Size in bytes of the associated queue to be allocated
> + * @ops: Protocol specific event-related operations
> + * @evt: Event descriptor array
> + * @num_events: Number of events in @evt array
> + * @num_sources: Number of possible sources for this protocol on this
> + * platform.
> + *
> + * Return: 0 on Success
> + */
> +int scmi_register_protocol_events(const struct scmi_handle *handle,
> + u8 proto_id, size_t queue_sz,
> + const struct scmi_protocol_event_ops *ops,
> + const struct scmi_event *evt, int num_events,
> + int num_sources)
> +{
> + int i;
> + size_t payld_sz = 0;
> + struct scmi_registered_protocol_events_desc *pd;
> + struct scmi_notify_instance *ni = handle->notify_priv;
> +
> + if (!ops || !evt || proto_id >= SCMI_MAX_PROTO)
> + return -EINVAL;
> +
> + /* Ensure atomic value is updated */
> + smp_mb__before_atomic();
> + if (unlikely(!ni || !atomic_read(&ni->initialized)))
> + return -EAGAIN;
> +
> + /* Attach to the notification main devres group */
> + if (!devres_open_group(ni->handle->dev, ni->gid, GFP_KERNEL))
> + return -ENOMEM;
> +
> + for (i = 0; i < num_events; i++)
> + payld_sz = max_t(size_t, payld_sz, evt[i].max_payld_sz);
> + pd = scmi_allocate_registered_protocol_desc(ni, proto_id, queue_sz,
> + sizeof(struct scmi_event_header) + payld_sz,
> + num_events, ops);
> + if (IS_ERR(pd))
> + goto err;
> +
> + for (i = 0; i < num_events; i++, evt++) {
> + struct scmi_registered_event *r_evt;
> +
> + r_evt = devm_kzalloc(ni->handle->dev, sizeof(*r_evt),
> + GFP_KERNEL);
> + if (!r_evt)
> + goto err;
> + r_evt->proto = pd;
> + r_evt->evt = evt;
> +
> + r_evt->sources = devm_kcalloc(ni->handle->dev, num_sources,
> + sizeof(refcount_t), GFP_KERNEL);
> + if (!r_evt->sources)
> + goto err;
> + r_evt->num_sources = num_sources;
> + mutex_init(&r_evt->sources_mtx);
> +
> + r_evt->report = devm_kzalloc(ni->handle->dev,
> + evt->max_report_sz, GFP_KERNEL);
> + if (!r_evt->report)
> + goto err;
> +
> + WRITE_ONCE(pd->registered_events[i], r_evt);
> + pr_info("SCMI Notifications: registered event - %X\n",
> + MAKE_ALL_SRCS_KEY(r_evt->proto->id, r_evt->evt->id));
> + }
> +
> + /* Register protocol and events...it will never be removed */
> + WRITE_ONCE(ni->registered_protocols[proto_id], pd);
> +
> + devres_close_group(ni->handle->dev, ni->gid);
> +
> + return 0;
> +
> +err:
> + pr_warn("SCMI Notifications - Proto:%X - Registration Failed !\n",
> + proto_id);
> + /* A failing protocol registration does not trigger full failure */
> + devres_close_group(ni->handle->dev, ni->gid);
> +
> + return -ENOMEM;
> +}
> +
> +/**
> + * scmi_notification_init - Initializes Notification Core Support
> + *
> + * This function lays out all the basic resources needed by the notification
> + * core instance identified by the provided handle: once done, all of the
> + * SCMI Protocols can register their events with the core during their own
> + * initializations.
> + *
> + * Note that failing to initialize the core notifications support does not
> + * cause the whole SCMI Protocols stack to fail its initialization.
> + *
> + * SCMI Notification Initialization happens in 2 steps:
> + *
> + * - initialization: basic common allocations (this function) -> .initialized
> + * - registration: protocols asynchronously come into life and registers their
> + * own supported list of events with the core; this causes
> + * further per-protocol allocations.
> + *
> + * Any user's callback registration attempt, referring a still not registered
> + * event, will be registered as pending and finalized later (if possible)
> + * by @scmi_protocols_late_init work.
> + * This allows for lazy initialization of SCMI Protocols due to late (or
> + * missing) SCMI drivers' modules loading.
> + *
> + * @handle: The handle identifying the platform instance to initialize
> + *
> + * Return: 0 on Success
> + */
> +int scmi_notification_init(struct scmi_handle *handle)
> +{
> + void *gid;
> + struct scmi_notify_instance *ni;
> +
> + gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
> + if (!gid)
> + return -ENOMEM;
> +
> + ni = devm_kzalloc(handle->dev, sizeof(*ni), GFP_KERNEL);
> + if (!ni)
> + goto err;
> +
> + ni->gid = gid;
> + ni->handle = handle;
> +
> + ni->registered_protocols = devm_kcalloc(handle->dev, SCMI_MAX_PROTO,
> + sizeof(char *), GFP_KERNEL);
> + if (!ni->registered_protocols)
> + goto err;
> +
> + handle->notify_priv = ni;
> +
> + atomic_set(&ni->initialized, 1);
> + atomic_set(&ni->enabled, 1);
> + /* Ensure atomic values are updated */
> + smp_mb__after_atomic();
> +
> + pr_info("SCMI Notifications Core Initialized.\n");
> +
> + devres_close_group(handle->dev, ni->gid);
> +
> + return 0;
> +
> +err:
> + pr_warn("SCMI Notifications - Initialization Failed.\n");
> + devres_release_group(handle->dev, NULL);
> + return -ENOMEM;
> +}
> +
> +/**
> + * scmi_notification_exit - Shutdown and clean Notification core
> + *
> + * @handle: The handle identifying the platform instance to shutdown
> + */
> +void scmi_notification_exit(struct scmi_handle *handle)
> +{
> + struct scmi_notify_instance *ni = handle->notify_priv;
> +
> + if (unlikely(!ni || !atomic_read(&ni->initialized)))
> + return;
> +
> + atomic_set(&ni->enabled, 0);
> + /* Ensure atomic values are updated */
> + smp_mb__after_atomic();
> +
> + devres_release_group(ni->handle->dev, ni->gid);
> +
> + pr_info("SCMI Notifications Core Shutdown.\n");
Is this actually useful? Seems like noise to me, maybe pr_debug is more appopriate.
> +}
> diff --git a/drivers/firmware/arm_scmi/notify.h b/drivers/firmware/arm_scmi/notify.h
> new file mode 100644
> index 000000000000..a7ece64e8842
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/notify.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * System Control and Management Interface (SCMI) Message Protocol
> + * notification header file containing some definitions, structures
> + * and function prototypes related to SCMI Notification handling.
> + *
> + * Copyright (C) 2019 ARM Ltd.
Update the dates given you are still changing this stuff?
> + */
> +#ifndef _SCMI_NOTIFY_H
> +#define _SCMI_NOTIFY_H
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +
> +/**
> + * scmi_event - Describes an event to be supported
Fairly sure this isn't valid kernel-doc.
* struct scmi_event - ...
Make sure to run the kernel-doc scripts over any files you've added kernel-doc to
and tidy up the warnings.
> + *
> + * Each SCMI protocol, during its initialization phase, can describe the events
> + * it wishes to support in a few struct scmi_event and pass them to the core
> + * using scmi_register_protocol_events().
> + *
> + * @id: Event ID
> + * @max_payld_sz: Max possible size for the payload of a notif msg of this kind
> + * @max_report_sz: Max possible size for the report of a notif msg of this kind
> + */
> +struct scmi_event {
> + u8 id;
> + size_t max_payld_sz;
> + size_t max_report_sz;
> +
Nitpick: Blank line isn't adding anything
> +};
> +
> +/**
> + * scmi_protocol_event_ops - Helpers called by notification core.
> + *
> + * These are called only in process context.
> + *
> + * @set_notify_enabled: Enable/disable the required evt_id/src_id notifications
> + * using the proper custom protocol commands.
> + * Return true if at least one the required src_id
> + * has been successfully enabled/disabled
> + */
> +struct scmi_protocol_event_ops {
> + bool (*set_notify_enabled)(const struct scmi_handle *handle,
> + u8 evt_id, u32 src_id, bool enabled);
> +};
> +
> +int scmi_notification_init(struct scmi_handle *handle);
> +void scmi_notification_exit(struct scmi_handle *handle);
> +
> +int scmi_register_protocol_events(const struct scmi_handle *handle,
> + u8 proto_id, size_t queue_sz,
> + const struct scmi_protocol_event_ops *ops,
> + const struct scmi_event *evt, int num_events,
> + int num_sources);
> +
> +#endif /* _SCMI_NOTIFY_H */
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 5c873a59b387..0679f10ab05e 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -4,6 +4,10 @@
> *
> * Copyright (C) 2018 ARM Ltd.
> */
> +
> +#ifndef _LINUX_SCMI_PROTOCOL_H
> +#define _LINUX_SCMI_PROTOCOL_H
> +
> #include <linux/device.h>
> #include <linux/types.h>
>
> @@ -227,6 +231,8 @@ struct scmi_reset_ops {
> * protocol(for internal use only)
> * @reset_priv: pointer to private data structure specific to reset
> * protocol(for internal use only)
> + * @notify_priv: pointer to private data structure specific to notifications
> + * (for internal use only)
> */
> struct scmi_handle {
> struct device *dev;
> @@ -242,6 +248,7 @@ struct scmi_handle {
> void *power_priv;
> void *sensor_priv;
> void *reset_priv;
> + void *notify_priv;
> };
>
> enum scmi_std_protocol {
> @@ -319,3 +326,5 @@ static inline void scmi_driver_unregister(struct scmi_driver *driver) {}
> typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *);
> int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn);
> void scmi_protocol_unregister(int protocol_id);
> +
> +#endif /* _LINUX_SCMI_PROTOCOL_H */
Hi
On 09/03/2020 11:33, Jonathan Cameron wrote:
> On Wed, 4 Mar 2020 16:25:50 +0000
> Cristian Marussi <[email protected]> wrote:
>
>> Add core SCMI Notifications protocol-registration support: allow protocols
>> to register their own set of supported events, during their initialization
>> phase. Notification core can track multiple platform instances by their
>> handles.
>>
>> Signed-off-by: Cristian Marussi <[email protected]>
>
> Hi.
>
> A few minor things inline. Fairly sure kernel-doc needs
> struct before the heading for each structure comment block.
>
> Also, the events queue init looks like it could just be done with
> a kfifo_alloc call. Perhaps that makes sense given later patches...
>
> Thanks,
>
> Jonathan
Thanks for the review first of all !
>
>> ---
>> V3 --> V4
>> - removed scratch ISR buffer, move scratch BH buffer into protocol
>> descriptor
>> - converted registered_protocols and registered_events from hashtables
>> into bare fixed-sized arrays
>> - removed unregister protocols' routines (never called really)
>> V2 --> V3
>> - added scmi_notify_instance to track target platform instance
>> V1 --> V2
>> - splitted out of V1 patch 04
>> - moved from IDR maps to real HashTables to store events
>> - scmi_notifications_initialized is now an atomic_t
>> - reviewed protocol registration/unregistration to use devres
>> - fixed:
>> drivers/firmware/arm_scmi/notify.c:483:18-23: ERROR:
>> reference preceded by free on line 482
>>
>> Reported-by: kbuild test robot <[email protected]>
>> Reported-by: Julia Lawall <[email protected]>
>> ---
[snip]
>> +
>> +/**
>> + * scmi_notify_instance - Represents an instance of the notification core
>> + *
>> + * Each platform instance, represented by a handle, has its own instance of
>> + * the notification subsystem represented by this structure.
>> + *
>> + * @gid: GroupID used for devres
>> + * @handle: A reference to the platform instance
>> + * @initialized: A flag that indicates if the core resources have been allocated
>> + * and protocols are allowed to register their supported events
>> + * @enabled: A flag to indicate events can be enabled and start flowing
>> + * @registered_protocols: An statically allocated array containing pointers to
>> + * all the registered protocol-level specific information
>> + * related to events' handling
>> + */
>> +struct scmi_notify_instance {
>> + void *gid;
>> + struct scmi_handle *handle;
>> + atomic_t initialized;
>> + atomic_t enabled;
>> + struct scmi_registered_protocol_events_desc **registered_protocols;
>> +};
>> +
>> +/**
>> + * events_queue - Describes a queue and its associated worker
>
> I guess this might become clear later, but right now this just looks like
> we are open code what could be handled automatically by just using
> kfifo_alloc
>
In fact I switched to this split alloc/init (as you guessed later) because of the lack
of devm_ flavour (and my ignorance about the usage of devm_add_action_or_reset ...)
I'll look into it.
>> + *
>> + * Each protocol has its own dedicated events_queue descriptor.
>> + *
>> + * @sz: Size in bytes of the related kfifo
>> + * @qbuf: Pre-allocated buffer of @sz bytes to be used by the kfifo
>> + * @kfifo: A dedicated Kernel kfifo descriptor
>> + */
>> +struct events_queue {
>> + size_t sz;
>> + u8 *qbuf;
>> + struct kfifo kfifo;
>> +};
>> +
>> +/**
>> + * scmi_event_header - A utility header
>
> struct scmi...
>
I'll fix all of these and test with kernel-doc.
>> + *
>> + * This header is prepended to each received event message payload before
>> + * queueing it on the related events_queue.
>> + *
>> + * @timestamp: The timestamp, in nanoseconds (boottime), which was associated
>> + * to this event as soon as it entered the SCMI RX ISR
>> + * @evt_id: Event ID (corresponds to the Event MsgID for this Protocol)
>> + * @payld_sz: Effective size of the embedded message payload which follows
>> + * @payld: A reference to the embedded event payload
>> + */
>> +struct scmi_event_header {
>> + u64 timestamp;
>> + u8 evt_id;
>> + size_t payld_sz;
>> + u8 payld[];
>> +} __packed;
>> +
>> +struct scmi_registered_event;
>> +
>> +/**
>> + * scmi_registered_protocol_events_desc - Protocol Specific information
>> + *
>> + * All protocols that registers at least one event have their protocol-specific
>> + * information stored here, together with the embedded allocated events_queue.
>> + * These descriptors are stored in the @registered_protocols array at protocol
>> + * registration time.
>> + *
>> + * Once these descriptors are successfully registered, they are NEVER again
>> + * removed or modified since protocols do not unregister ever, so that once we
>> + * safely grab a NON-NULL reference from the array we can keep it and use it.
>> + *
>> + * @id: Protocol ID
>> + * @ops: Protocol specific and event-related operations
>> + * @equeue: The embedded per-protocol events_queue
>> + * @ni: A reference to the initialized instance descriptor
>> + * @eh: A reference to pre-allocated buffer to be used as a scratch area by the
>> + * deferred worker when fetching data from the kfifo
>> + * @eh_sz: Size of the pre-allocated buffer @eh
>> + * @in_flight: A reference to an in flight @scmi_registered_event
>> + * @num_events: Number of events in @registered_events
>> + * @registered_events: A dynamically allocated array holding all the registered
>> + * events' descriptors, whose fixed-size is determined at
>> + * compile time.
>> + */
>> +struct scmi_registered_protocol_events_desc {
>> + u8 id;
>> + const struct scmi_protocol_event_ops *ops;
>> + struct events_queue equeue;
>> + struct scmi_notify_instance *ni;
>> + struct scmi_event_header *eh;
>> + size_t eh_sz;
>> + void *in_flight;
>> + int num_events;
>> + struct scmi_registered_event **registered_events;
>> +};
>> +
>> +/**
>> + * scmi_registered_event - Event Specific Information
>
> struct scmi_registered_event - Event...
>
I'll fix
>> + *
>> + * All registered events are represented by one of these structures that are
>> + * stored in the @registered_events array at protocol registration time.
>> + *
>> + * Once these descriptors are successfully registered, they are NEVER again
>> + * removed or modified since protocols do not unregister ever, so that once we
>> + * safely grab a NON-NULL reference from the table we can keep it and use it.
>> + *
>> + * @proto: A reference to the associated protocol descriptor
>> + * @evt: A reference to the associated event descriptor (as provided at
>> + * registration time)
>> + * @report: A pre-allocated buffer used by the deferred worker to fill a
>> + * customized event report
>> + * @num_sources: The number of possible sources for this event as stated at
>> + * events' registration time
>> + * @sources: A reference to a dynamically allocated array used to refcount the
>> + * events' enable requests for all the existing sources
>> + * @sources_mtx: A mutex to serialize the access to @sources
>> + */
>> +struct scmi_registered_event {
>> + struct scmi_registered_protocol_events_desc *proto;
>> + const struct scmi_event *evt;
>> + void *report;
>> + u32 num_sources;
>> + refcount_t *sources;
>> + struct mutex sources_mtx;
>> +};
>> +
>> +/**
>> + * scmi_initialize_events_queue - Allocate/Initialize a kfifo buffer
>> + *
>> + * Allocate a buffer for the kfifo and initialize it.
>> + *
>> + * @ni: A reference to the notification instance to use
>> + * @equeue: The events_queue to initialize
>> + * @sz: Size of the kfifo buffer to allocate
>> + *
>> + * Return: 0 on Success
>> + */
>> +static int scmi_initialize_events_queue(struct scmi_notify_instance *ni,
>> + struct events_queue *equeue, size_t sz)
>> +{
>> + equeue->qbuf = devm_kzalloc(ni->handle->dev, sz, GFP_KERNEL);
>> + if (!equeue->qbuf)
>> + return -ENOMEM;
>> + equeue->sz = sz;
>> +
>> + return kfifo_init(&equeue->kfifo, equeue->qbuf, equeue->sz);
>
> This seems like a slightly odd dance. Why not use kfifo_alloc?
>
> If it's because of the lack of devm_kfifo_alloc, maybe use a devm_add_action_or_reset
> to handle that.
>
As said above exactly for the lack of devm_ flavour
>> +}
>> +
>> +/**
>> + * scmi_allocate_registered_protocol_desc - Allocate a registered protocol
>> + * events' descriptor
>> + *
>> + * It is supposed to be called only once for each protocol at protocol
>> + * initialization time, so it warns if the requested protocol is found
>> + * already registered.
>> + *
>> + * @ni: A reference to the notification instance to use
>> + * @proto_id: Protocol ID
>> + * @queue_sz: Size of the associated queue to allocate
>> + * @eh_sz: Size of the event header scratch area to pre-allocate
>> + * @num_events: Number of events to support (size of @registered_events)
>> + * @ops: Pointer to a struct holding references to protocol specific helpers
>> + * needed during events handling
>> + *
>> + * Returns the allocated and registered descriptor on Success
>> + */
>> +static struct scmi_registered_protocol_events_desc *
[snip]
>> + */
>> +void scmi_notification_exit(struct scmi_handle *handle)
>> +{
>> + struct scmi_notify_instance *ni = handle->notify_priv;
>> +
>> + if (unlikely(!ni || !atomic_read(&ni->initialized)))
>> + return;
>> +
>> + atomic_set(&ni->enabled, 0);
>> + /* Ensure atomic values are updated */
>> + smp_mb__after_atomic();
>> +
>> + devres_release_group(ni->handle->dev, ni->gid);
>> +
>> + pr_info("SCMI Notifications Core Shutdown.\n");
>
> Is this actually useful? Seems like noise to me, maybe pr_debug is more appopriate.
>
No I think in general the verbosity of the printk is still to be 'tuned' in this series
>> +}
>> diff --git a/drivers/firmware/arm_scmi/notify.h b/drivers/firmware/arm_scmi/notify.h
>> new file mode 100644
>> index 000000000000..a7ece64e8842
>> --- /dev/null
>> +++ b/drivers/firmware/arm_scmi/notify.h
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * System Control and Management Interface (SCMI) Message Protocol
>> + * notification header file containing some definitions, structures
>> + * and function prototypes related to SCMI Notification handling.
>> + *
>> + * Copyright (C) 2019 ARM Ltd.
>
> Update the dates given you are still changing this stuff?
>
Missed that. I'll fix.
>> + */
>> +#ifndef _SCMI_NOTIFY_H
>> +#define _SCMI_NOTIFY_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * scmi_event - Describes an event to be supported
>
> Fairly sure this isn't valid kernel-doc.
>
> * struct scmi_event - ...
>
> Make sure to run the kernel-doc scripts over any files you've added kernel-doc to
> and tidy up the warnings.
>
I'll do.
>> + *
>> + * Each SCMI protocol, during its initialization phase, can describe the events
>> + * it wishes to support in a few struct scmi_event and pass them to the core
>> + * using scmi_register_protocol_events().
>> + *
>> + * @id: Event ID
>> + * @max_payld_sz: Max possible size for the payload of a notif msg of this kind
>> + * @max_report_sz: Max possible size for the report of a notif msg of this kind
>> + */
>> +struct scmi_event {
>> + u8 id;
>> + size_t max_payld_sz;
>> + size_t max_report_sz;
>> +
>
> Nitpick: Blank line isn't adding anything
>
Missed. I'll fix
As a general note, this morning I was going to reply to myself (O_o) on this patch
saying that I'm inclined to review a bit the current initialization phase of
registered_protocols and registered_events in the sense of adding a few cpu barriers
which are probably lacking where I use mere compiler barriers (_ONCE).
I'll put those probably in v5
Thanks again
Cristian
>> +
>> +/**
>> + * scmi_protocol_event_ops - Helpers called by notification core.
>> + *
>> + * These are called only in process context.
>> + *
>> + * @set_notify_enabled: Enable/disable the required evt_id/src_id notifications
>> + * using the proper custom protocol commands.
>> + * Return true if at least one the required src_id
>> + * has been successfully enabled/disabled
>> + */
>> +struct scmi_protocol_event_ops {
>> + bool (*set_notify_enabled)(const struct scmi_handle *handle,
>> + u8 evt_id, u32 src_id, bool enabled);
>> +};
>> +
>> +int scmi_notification_init(struct scmi_handle *handle);
>> +void scmi_notification_exit(struct scmi_handle *handle);
>> +
>> +int scmi_register_protocol_events(const struct scmi_handle *handle,
>> + u8 proto_id, size_t queue_sz,
>> + const struct scmi_protocol_event_ops *ops,
>> + const struct scmi_event *evt, int num_events,
>> + int num_sources);
>> +
>> +#endif /* _SCMI_NOTIFY_H */
>> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
>> index 5c873a59b387..0679f10ab05e 100644
>> --- a/include/linux/scmi_protocol.h
>> +++ b/include/linux/scmi_protocol.h
>> @@ -4,6 +4,10 @@
>> *
>> * Copyright (C) 2018 ARM Ltd.
>> */
>> +
>> +#ifndef _LINUX_SCMI_PROTOCOL_H
>> +#define _LINUX_SCMI_PROTOCOL_H
>> +
>> #include <linux/device.h>
>> #include <linux/types.h>
>>
>> @@ -227,6 +231,8 @@ struct scmi_reset_ops {
>> * protocol(for internal use only)
>> * @reset_priv: pointer to private data structure specific to reset
>> * protocol(for internal use only)
>> + * @notify_priv: pointer to private data structure specific to notifications
>> + * (for internal use only)
>> */
>> struct scmi_handle {
>> struct device *dev;
>> @@ -242,6 +248,7 @@ struct scmi_handle {
>> void *power_priv;
>> void *sensor_priv;
>> void *reset_priv;
>> + void *notify_priv;
>> };
>>
>> enum scmi_std_protocol {
>> @@ -319,3 +326,5 @@ static inline void scmi_driver_unregister(struct scmi_driver *driver) {}
>> typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *);
>> int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn);
>> void scmi_protocol_unregister(int protocol_id);
>> +
>> +#endif /* _LINUX_SCMI_PROTOCOL_H */
>
>