Message-ID: <20200608170258.GB13622@bogus>
Date:   Mon, 8 Jun 2020 18:02:58 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     Cristian Marussi <cristian.marussi@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        lukasz.luba@....com, james.quinlan@...adcom.com,
        Jonathan.Cameron@...wei.com, dave.martin@....com,
        Sudeep Holla <sudeep.holla@....com>
Subject: Re: [PATCH v8 1/9] firmware: arm_scmi: Add notification
 protocol-registration

On Wed, May 20, 2020 at 09:11:10AM +0100, Cristian Marussi 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.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> ---
>  drivers/firmware/arm_scmi/Makefile |   2 +-
>  drivers/firmware/arm_scmi/common.h |   4 +
>  drivers/firmware/arm_scmi/notify.c | 435 +++++++++++++++++++++++++++++
>  drivers/firmware/arm_scmi/notify.h |  56 ++++
>  include/linux/scmi_protocol.h      |   3 +
>  5 files changed, 499 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/notify.c b/drivers/firmware/arm_scmi/notify.c
> new file mode 100644
> index 000000000000..8b620fc7df50
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -0,0 +1,435 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Notification support
> + *
> + * Copyright (C) 2020 ARM Ltd.
> + */
> +/**
> + * DOC: Theory of operation
> + *
> + * 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 &struct 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 &struct registered_protocols and
> + * &struct registered_events arrays.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#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

[nit] Space after define instead of tab.

> +#define	SCMI_ALL_SRC_IDS		0xffffUL

Once you move {PROTO,EVT,SRC}_ID_MASK here, you can just {re-}use SRC_ID_MASK

> +/*
> + * 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)))
> +

You can use {PROTO,EVT,SRC}_ID_MASK here and FIELD_PREP in the above
macro.

> +#define MAKE_ALL_SRCS_KEY(p, e)			\
> +	MAKE_HASH_KEY((p), (e), SCMI_ALL_SRC_IDS)
> +
> +struct scmi_registered_protocol_events_desc;
> +

Just a thought here: can we rename all protocol_event* as just event*.
The term protocol is kind of implicit and makes the names too long.
I feel some are really too long :). Similar if you think protocol
is implicit elsewhere, drop it.

> +/**
> + * struct scmi_notify_instance  - Represents an instance of the notification
> + * core
> + * @gid: GroupID used for devres
> + * @handle: A reference to the platform instance
> + * @registered_protocols: A statically allocated array containing pointers to
> + *			  all the registered protocol-level specific information
> + *			  related to events' handling
> + *
> + * Each platform instance, represented by a handle, has its own instance of
> + * the notification subsystem represented by this structure.
> + */
> +struct scmi_notify_instance {
> +	void						*gid;
> +	struct scmi_handle				*handle;
> +	struct scmi_registered_protocol_events_desc	**registered_protocols;
> +};
> +
> +/**
> + * struct events_queue  - Describes a queue and its associated worker
> + * @sz: Size in bytes of the related kfifo
> + * @kfifo: A dedicated Kernel kfifo descriptor
> + *
> + * Each protocol has its own dedicated events_queue descriptor.
> + */
> +struct events_queue {
> +	size_t				sz;
> +	struct kfifo			kfifo;
> +};
> +

[nit] Add tabs only for alignment, just to keep it uniform throughout.

> +/**
> + * struct scmi_event_header  - A utility header
> + * @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
> + *
> + * This header is prepended to each received event message payload before
> + * queueing it on the related &struct events_queue.
> + */
> +struct scmi_event_header {
> +	u64	timestamp;
> +	u8	evt_id;
> +	size_t	payld_sz;
> +	u8	payld[];
> +} __packed;
> +

Is this directly read from the shmem ? Trying to understand need for
__packed.

> +struct scmi_registered_event;
> +
> +/**
> + * struct scmi_registered_protocol_events_desc  - Protocol Specific information
> + * @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 &struct 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.
> + *
> + * All protocols that register 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.
> + */
> +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;
> +};
> +
> +/**
> + * struct scmi_registered_event  - Event Specific Information
> + * @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
> + *
> + * 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.
> + */
> +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_kfifo_free()  - Devres action helper to free the kfifo
> + * @kfifo: The kfifo to free
> + */
> +static void scmi_kfifo_free(void *kfifo)
> +{
> +	kfifo_free((struct kfifo *)kfifo);
> +}
> +
> +/**
> + * scmi_initialize_events_queue()  - Allocate/Initialize a kfifo buffer
> + * @ni: A reference to the notification instance to use
> + * @equeue: The events_queue to initialize
> + * @sz: Size of the kfifo buffer to allocate
> + *
> + * Allocate a buffer for the kfifo and initialize it.
> + *
> + * Return: 0 on Success
> + */
> +static int scmi_initialize_events_queue(struct scmi_notify_instance *ni,
> +					struct events_queue *equeue, size_t sz)
> +{
> +	if (kfifo_alloc(&equeue->kfifo, sz, GFP_KERNEL))
> +		return -ENOMEM;
> +	/* Size could have been roundup to power-of-two */
> +	equeue->sz = kfifo_size(&equeue->kfifo);
> +
> +	return devm_add_action_or_reset(ni->handle->dev, scmi_kfifo_free,
> +					&equeue->kfifo);
> +}
> +
> +/**
> + * scmi_allocate_registered_protocol_desc()  - Allocate a registered protocol
> + * events' descriptor
> + * @ni: A reference to the &struct scmi_notify_instance 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
> + *
> + * 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.
> + *
> + * Return: 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;
> +
> +	/* Ensure protocols are up to date */
> +	smp_rmb();
> +	if (ni->registered_protocols[proto_id]) {
> +		WARN_ON(1);

Can't this be if (WARN_ON(ni->registered_protocols[proto_id])) ?

> +		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
> + * @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.
> + *
> + * 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.
> + *
> + * 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;
> +
> +	if (!ops || !evt || proto_id >= SCMI_MAX_PROTO)
> +		return -EINVAL;
> +
> +	/* Ensure notify_priv is updated */
> +	smp_rmb();
> +	if (unlikely(!handle->notify_priv))
> +		return -ENOMEM;
> +	ni = handle->notify_priv;
> +
> +	/* 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;
> +
> +		pd->registered_events[i] = r_evt;
> +		/* Ensure events are updated */
> +		smp_wmb();
> +		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 */
> +	ni->registered_protocols[proto_id] = pd;
> +	/* Ensure protocols are updated */
> +	smp_wmb();
> +
> +	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
> + * @handle: The handle identifying the platform instance to initialize
> + *
> + * 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)
> + * * 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.
> + *
> + * 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);

May not be too expensive, do we have to allocate for all 256 possible
protocols ? Will it help if we share list of implemented protocols.
I know this may get complex once we add support for registering protocols
later, but do we need it now ?

> +	if (!ni->registered_protocols)
> +		goto err;
> +
> +	handle->notify_priv = ni;
> +	/* Ensure handle is up to date */
> +	smp_wmb();
> +
> +	pr_info("SCMI Notifications Core Enabled.\n");
> +
> +	devres_close_group(handle->dev, ni->gid);
> +
> +	return 0;
> +
> +err:
> +	pr_warn("SCMI Notifications - Initialization Failed.\n");

You have defined pr_fmt, do you still need "SCMI Notifications" for
all the logging(err/warn/info) everywhere in the file. Making use of
it will help you trim the messages without loosing it in the log. Update
pr_fmt if needed.

> +	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;
> +
> +	/* Ensure notify_priv is updated */
> +	smp_rmb();
> +	if (unlikely(!handle->notify_priv))
> +		return;
> +	ni = handle->notify_priv;
> +
> +	devres_release_group(ni->handle->dev, ni->gid);
> +}
> diff --git a/drivers/firmware/arm_scmi/notify.h b/drivers/firmware/arm_scmi/notify.h
> new file mode 100644
> index 000000000000..54094aaf812a
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/notify.h
> @@ -0,0 +1,56 @@
> +/* 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) 2020 ARM Ltd.
> + */
> +#ifndef _SCMI_NOTIFY_H
> +#define _SCMI_NOTIFY_H
> +
> +#include <linux/device.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct scmi_event  - Describes an event to be supported
> + * @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

:s/notif msg of this kind/notification message/

--
Regards,
Sudeep

Powered by blists - more mailing lists