On Wed, Sep 30, 2020 at 03:50:47PM -0700, Dave Ertman wrote:
> From: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
> Co-developed-by: Fred Oh <fred.oh@xxxxxxxxxxxxxxx>
> Signed-off-by: Fred Oh <fred.oh@xxxxxxxxxxxxxxx>
> Signed-off-by: Dave Ertman <david.m.ertman@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> +//
> +
> +#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
