[PATCH 2/6] ASoC: SOF: Introduce descriptors for SOF client

[Greg KH]

On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
> From: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> 
> A client in the SOF (Sound Open Firmware) context is a
> device that needs to communicate with the DSP via IPC
> messages. The SOF core is responsible for serializing the
> IPC messages to the DSP from the different clients. One
> example of an SOF client would be an IPC test client that
> floods the DSP with test IPC messages to validate if the
> serialization works as expected. Multi-client support will
> also add the ability to split the existing audio cards
> into multiple ones, so as to e.g. to deal with HDMI with a
> dedicated client instead of adding HDMI to all cards.
> 
> This patch introduces descriptors for SOF client driver
> and SOF client device along with APIs for registering
> and unregistering a SOF client driver, sending IPCs from
> a client device and accessing the SOF core debugfs root entry.
> 
> Along with this, add a couple of new members to struct
> snd_sof_dev that will be used for maintaining the list of
> clients.
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> Co-developed-by: Fred Oh <fred.oh at linux.intel.com>
> Signed-off-by: Fred Oh <fred.oh at linux.intel.com>
> Signed-off-by: Dave Ertman <david.m.ertman at intel.com>
> ---
>  sound/soc/sof/Kconfig      |  19 ++++++
>  sound/soc/sof/Makefile     |   3 +
>  sound/soc/sof/core.c       |   2 +
>  sound/soc/sof/sof-client.c | 117 +++++++++++++++++++++++++++++++++++++
>  sound/soc/sof/sof-client.h |  65 +++++++++++++++++++++
>  sound/soc/sof/sof-priv.h   |   6 ++
>  6 files changed, 212 insertions(+)
>  create mode 100644 sound/soc/sof/sof-client.c
>  create mode 100644 sound/soc/sof/sof-client.h

As you are creating new sysfs directories, you should have some
documentation for them :(

> 
> diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
> index 4dda4b62509f..cea7efedafef 100644
> --- a/sound/soc/sof/Kconfig
> +++ b/sound/soc/sof/Kconfig
> @@ -50,6 +50,24 @@ config SND_SOC_SOF_DEBUG_PROBES
>  	  Say Y if you want to enable probes.
>  	  If unsure, select "N".
>  
> +config SND_SOC_SOF_CLIENT
> +	tristate
> +	select ANCILLARY_BUS
> +	help
> +	  This option is not user-selectable but automagically handled by
> +	  'select' statements at a higher level
> +
> +config SND_SOC_SOF_CLIENT_SUPPORT
> +	bool "SOF enable clients"
> +	depends on SND_SOC_SOF
> +	help
> +	  This adds support for ancillary client devices to separate out the debug
> +	  functionality for IPC tests, probes etc. into separate devices. This
> +	  option would also allow adding client devices based on DSP FW
> +	  capabilities and ACPI/OF device information.
> +	  Say Y if you want to enable clients with SOF.
> +	  If unsure select "N".
> +
>  config SND_SOC_SOF_DEVELOPER_SUPPORT
>  	bool "SOF developer options support"
>  	depends on EXPERT
> @@ -186,6 +204,7 @@ endif ## SND_SOC_SOF_DEVELOPER_SUPPORT
>  
>  config SND_SOC_SOF
>  	tristate
> +	select SND_SOC_SOF_CLIENT if SND_SOC_SOF_CLIENT_SUPPORT
>  	select SND_SOC_TOPOLOGY
>  	select SND_SOC_SOF_NOCODEC if SND_SOC_SOF_NOCODEC_SUPPORT
>  	help
> diff --git a/sound/soc/sof/Makefile b/sound/soc/sof/Makefile
> index 05718dfe6cd2..5e46f25a3851 100644
> --- a/sound/soc/sof/Makefile
> +++ b/sound/soc/sof/Makefile
> @@ -2,6 +2,7 @@
>  
>  snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
>  		control.o trace.o utils.o sof-audio.o
> +snd-sof-client-objs := sof-client.o
>  snd-sof-$(CONFIG_SND_SOC_SOF_DEBUG_PROBES) += probe.o compress.o
>  
>  snd-sof-pci-objs := sof-pci-dev.o
> @@ -18,6 +19,8 @@ obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o
>  obj-$(CONFIG_SND_SOC_SOF_OF) += snd-sof-of.o
>  obj-$(CONFIG_SND_SOC_SOF_PCI) += snd-sof-pci.o
>  
> +obj-$(CONFIG_SND_SOC_SOF_CLIENT) += snd-sof-client.o
> +
>  obj-$(CONFIG_SND_SOC_SOF_INTEL_TOPLEVEL) += intel/
>  obj-$(CONFIG_SND_SOC_SOF_IMX_TOPLEVEL) += imx/
>  obj-$(CONFIG_SND_SOC_SOF_XTENSA) += xtensa/
> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> index adc7c37145d6..72a97219395f 100644
> --- a/sound/soc/sof/core.c
> +++ b/sound/soc/sof/core.c
> @@ -314,8 +314,10 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
>  	INIT_LIST_HEAD(&sdev->widget_list);
>  	INIT_LIST_HEAD(&sdev->dai_list);
>  	INIT_LIST_HEAD(&sdev->route_list);
> +	INIT_LIST_HEAD(&sdev->client_list);
>  	spin_lock_init(&sdev->ipc_lock);
>  	spin_lock_init(&sdev->hw_lock);
> +	mutex_init(&sdev->client_mutex);
>  
>  	if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
>  		INIT_WORK(&sdev->probe_work, sof_probe_work);
> diff --git a/sound/soc/sof/sof-client.c b/sound/soc/sof/sof-client.c
> new file mode 100644
> index 000000000000..f7e476d99ff6
> --- /dev/null
> +++ b/sound/soc/sof/sof-client.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright(c) 2020 Intel Corporation. All rights reserved.
> +//
> +// Author: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> +//
> +
> +#include <linux/debugfs.h>
> +#include <linux/errno.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include "sof-client.h"
> +#include "sof-priv.h"
> +
> +static void sof_client_ancildev_release(struct device *dev)
> +{
> +	struct ancillary_device *ancildev = to_ancillary_dev(dev);
> +	struct sof_client_dev *cdev = ancillary_dev_to_sof_client_dev(ancildev);
> +
> +	ida_simple_remove(cdev->client_ida, ancildev->id);
> +	kfree(cdev);
> +}
> +
> +static struct sof_client_dev *sof_client_dev_alloc(struct snd_sof_dev *sdev, const char *name,
> +						   struct ida *client_ida)
> +{
> +	struct sof_client_dev *cdev;
> +	struct ancillary_device *ancildev;
> +	int ret;
> +
> +	cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
> +	if (!cdev)
> +		return NULL;
> +
> +	cdev->sdev = sdev;

No reference counting?  How can you guarantee the lifespan is ok?

> +	cdev->client_ida = client_ida;
> +	ancildev = &cdev->ancildev;
> +	ancildev->name = name;
> +	ancildev->dev.parent = sdev->dev;

Ah, you guarantee it by making it the parent.  Sneaky, but is it really
needed?

> +	ancildev->dev.release = sof_client_ancildev_release;
> +
> +	ancildev->id = ida_alloc(client_ida, GFP_KERNEL);
> +	if (ancildev->id < 0) {
> +		dev_err(sdev->dev, "error: get IDA idx for ancillary device %s failed\n", name);
> +		ret = ancildev->id;
> +		goto err_free;
> +	}
> +
> +	ret = ancillary_device_initialize(ancildev);
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: failed to initialize client dev %s\n", name);
> +		ida_simple_remove(client_ida, ancildev->id);
> +		goto err_free;
> +	}
> +
> +	return cdev;
> +
> +err_free:
> +	kfree(cdev);
> +	return NULL;
> +}
> +
> +int sof_client_dev_register(struct snd_sof_dev *sdev, const char *name, struct ida *client_ida)
> +{
> +	struct sof_client_dev *cdev;
> +	int ret;
> +
> +	cdev = sof_client_dev_alloc(sdev, name, client_ida);
> +	if (!cdev)
> +		return -ENODEV;
> +
> +	ret = ancillary_device_add(&cdev->ancildev);

Why have you split this up into two calls?  Why not just
"sof_client_dev_create() or something like that?

Having to make a sof_* call, and then a separate ancillary_device_* call
feels pretty ackward, right?


> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: failed to add client dev %s\n", name);
> +		put_device(&cdev->ancildev.dev);

Ugh that's a deep knowledge of the ancil code, would be nicer if the
caller function handled all of that for you, right?

> +		return ret;
> +	}
> +
> +	/* add to list of SOF client devices */
> +	mutex_lock(&sdev->client_mutex);
> +	list_add(&cdev->list, &sdev->client_list);
> +	mutex_unlock(&sdev->client_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(sof_client_dev_register, SND_SOC_SOF_CLIENT);
> +
> +void sof_client_dev_unregister(struct sof_client_dev *cdev)
> +{
> +	struct snd_sof_dev *sdev = cdev->sdev;
> +
> +	/* remove from list of SOF client devices */
> +	mutex_lock(&sdev->client_mutex);
> +	list_del(&cdev->list);
> +	mutex_unlock(&sdev->client_mutex);

So you add and remove things from the list, but do not do anything with
that list?  Why a list at all?

> +
> +	ancillary_device_unregister(&cdev->ancildev);

Does this free the expected memory?  I think so, but commenting that it
does is nice :)

> +}
> +EXPORT_SYMBOL_NS_GPL(sof_client_dev_unregister, SND_SOC_SOF_CLIENT);
> +
> +int sof_client_ipc_tx_message(struct sof_client_dev *cdev, u32 header, void *msg_data,
> +			      size_t msg_bytes, void *reply_data, size_t reply_bytes)
> +{
> +	return sof_ipc_tx_message(cdev->sdev->ipc, header, msg_data, msg_bytes,
> +				  reply_data, reply_bytes);
> +}
> +EXPORT_SYMBOL_NS_GPL(sof_client_ipc_tx_message, SND_SOC_SOF_CLIENT);
> +
> +struct dentry *sof_client_get_debugfs_root(struct sof_client_dev *cdev)
> +{
> +	return cdev->sdev->debugfs_root;
> +}
> +EXPORT_SYMBOL_NS_GPL(sof_client_get_debugfs_root, SND_SOC_SOF_CLIENT);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/sound/soc/sof/sof-client.h b/sound/soc/sof/sof-client.h
> new file mode 100644
> index 000000000000..62212f69c236
> --- /dev/null
> +++ b/sound/soc/sof/sof-client.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only) */

Why the "()"?

thanks,

greg k-h