Re: [PATCH] soc: qcom: Introduce debugfs interface to smem

[Alex Elder <elder@linaro.org>]

On 11/22/20 11:21 PM, Bjorn Andersson wrote:
> Every now and then it's convenient to be able to inspect the content of
> SMEM items. Rather than carrying some hack locally let's upstream a
> driver that when inserted exposes a debugfs interface for dumping
> available items.

I have a number of comments.  I think only two are things
you really need to act on, the rest are just some suggestions
to consider.

					-Alex

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/soc/qcom/Kconfig        |   7 +++
>   drivers/soc/qcom/Makefile       |   1 +
>   drivers/soc/qcom/smem_debugfs.c | 102 ++++++++++++++++++++++++++++++++
>   3 files changed, 110 insertions(+)
>   create mode 100644 drivers/soc/qcom/smem_debugfs.c
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 3dc3e3d61ea3..7e1dd6b3f33a 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -128,6 +128,13 @@ config QCOM_SMEM
>   	  The driver provides an interface to items in a heap shared among all
>   	  processors in a Qualcomm platform.
>   
> +config QCOM_SMEM_DEBUGFS
> +	tristate "Qualcomm Shared Memory Manager (SMEM) DebugFS interface"
> +	depends on QCOM_SMEM
> +	depends on DEBUG_FS
> +	help
> +	  Provides a debugfs interface for inspecting SMEM.
> +
>   config QCOM_SMD_RPM
>   	tristate "Qualcomm Resource Power Manager (RPM) over SMD"
>   	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 93392d9dc7f7..632eefc5a897 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -15,6 +15,7 @@ qcom_rpmh-y			+= rpmh-rsc.o
>   qcom_rpmh-y			+= rpmh.o
>   obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
>   obj-$(CONFIG_QCOM_SMEM) +=	smem.o
> +obj-$(CONFIG_QCOM_SMEM_DEBUGFS) += smem_debugfs.o
>   obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>   obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
>   obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
> diff --git a/drivers/soc/qcom/smem_debugfs.c b/drivers/soc/qcom/smem_debugfs.c
> new file mode 100644
> index 000000000000..11ef29a0cada
> --- /dev/null
> +++ b/drivers/soc/qcom/smem_debugfs.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020, Linaro Ltd.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +struct smem_debugfs {
> +	struct dentry *root;
> +};

This type is never used, so get rid of it.

> +
> +static int smem_debugfs_item_show(struct seq_file *seq, void *p)
> +{
> +	unsigned long data = (unsigned long)seq->private;
> +	unsigned long item = data & 0xffff;
> +	unsigned long host = data >> 16;

You extract the item and host from the data pointer here,
and encode it below.  Maybe you could encapsulate those
two operations into a pair of trivial helper functions.
When I see something like this I wonder about why 16 bits
is the right number, and having little functions like that
provides a place to explain it.

Also, as I will say again below, I prefer not to see raw
numbers in the code without explanation, i.e., go symbolic:

	unsigned long host = data >> ITEM_SHIFT & HOST_MASK;
	unsigned long item = data & ITEM_MASK;

> +	size_t len;
> +	void *ptr;
> +
> +	ptr = qcom_smem_get(host, item, &len);
> +	if (IS_ERR(ptr))
> +		return PTR_ERR(ptr);
> +
> +	seq_hex_dump(seq, "", DUMP_PREFIX_OFFSET, 16, 1, ptr, len, true);
> +
> +	return 0;
> +}
> +
> +static int smem_debugfs_item_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, smem_debugfs_item_show, inode->i_private);
> +}
> +
> +static const struct file_operations smem_debugfs_item_ops = {
> +	.open = smem_debugfs_item_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +

You could mention that SMEM entries never go away, and
that you intentionally ignore the EEXIST error that comes
back from failed attempts to re-create existing entries
I hope you aren't spewing errors for these (look at
start_creating() in "fs/debugfs/inode.c").  I agree
with your effort to avoid tracking all item files.

> +static int smem_debugfs_rescan(struct seq_file *seq, void *p)
> +{
> +	struct dentry *root = seq->private;
> +	unsigned long item;
> +	unsigned long host;
> +	unsigned long data;
> +	char name[10];
> +	char *ptr;
> +
> +	for (host = 0; host < 10; host++) {

It would be nice if SMEM_HOST_COUNT were exposed so you could
use it here.  I prefer something symbolic anyway.

> +		for (item = 0; item < 512; item++) {

Same comment, about SMEM_ITEM_COUNT.

> +			ptr = qcom_smem_get(host, item, NULL);
> +			if (IS_ERR(ptr))
> +				continue;
> +
> +			sprintf(name, "%ld-%ld", host, item);

Use %lu for unsigned.

Is there any way you can think of that you can indicate which
items are fixed, and which are dynamically allocated?  (There
are only 8, so it's not that important.)

What about items in the global partition?  (I'm forgetting
some of the details about this right now, I'm just scanning
through the SMEM code as I review this.  So maybe this
comment doesn't make sense.)

> +
> +			data = host << 16 | item > +			debugfs_create_file(name, 0400, root,
> +					    (void *)data, &smem_debugfs_item_ops);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int smem_debugfs_rescan_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, smem_debugfs_rescan, inode->i_private);
> +}
> +
> +static const struct file_operations smem_debugfs_rescan_ops = {
> +	.open = smem_debugfs_rescan_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static struct dentry *smem_debugfs_root;
> +
> +static int __init qcom_smem_debugfs_init(void)
> +{
> +	smem_debugfs_root = debugfs_create_dir("qcom_smem", NULL);
> +	debugfs_create_file("rescan", 0400, smem_debugfs_root,
> +			    smem_debugfs_root, &smem_debugfs_rescan_ops);
> +
> +	return 0;
> +}
> +
> +static void __exit qcom_smem_debugfs_exit(void)
> +{
> +	debugfs_remove_recursive(smem_debugfs_root);
> +}
> +
> +module_init(qcom_smem_debugfs_init);
> +module_exit(qcom_smem_debugfs_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm SMEM debugfs driver");
> +MODULE_LICENSE("GPL v2");
>