diff mbox series

soc: qcom: Introduce debugfs interface to smem

Message ID 20201123052119.157551-1-bjorn.andersson@linaro.org
State New
Headers show
Series soc: qcom: Introduce debugfs interface to smem | expand

Commit Message

Bjorn Andersson Nov. 23, 2020, 5:21 a.m. UTC
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.

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

-- 
2.29.2

Comments

Vinod Koul Nov. 24, 2020, 3:34 p.m. UTC | #1
On 22-11-20, 23:21, 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.

> 

> 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.


Do we need additional debugfs entry, maybe better to depend on DEBUG_FS
being enabled and this file part of QCOM_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;

> +};

> +

> +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;

> +	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,

> +};


How about using DEFINE_SHOW_ATTRIBUTE() instead? That will help cut down
this boiler plate code..

> +

> +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++) {

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

> +			ptr = qcom_smem_get(host, item, NULL);

> +			if (IS_ERR(ptr))

> +				continue;

> +

> +			sprintf(name, "%ld-%ld", host, item);

> +

> +			data = host << 16 | item;

> +			debugfs_create_file(name, 0400, root,

> +					    (void *)data, &smem_debugfs_item_ops);


So IIUC user invokes scan file which creates additional files, right?
Additional invoke will do that as well..?

> +		}

> +	}

> +

> +	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,

> +};


Here as well?

> +

> +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");

> -- 

> 2.29.2


-- 
~Vinod
Bjorn Andersson Nov. 24, 2020, 4:39 p.m. UTC | #2
On Tue 24 Nov 09:34 CST 2020, Vinod Koul wrote:

> On 22-11-20, 23:21, 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.

> > 

> > 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.

> 

> Do we need additional debugfs entry, maybe better to depend on DEBUG_FS

> being enabled and this file part of QCOM_SMEM?

> 


We don't need this in any form of production system, so rather than
tainting qcom_smem.c I put it in a separate driver that isn't even
automatically loaded.

> > +

> >  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;

> > +};

> > +

> > +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;

> > +	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,

> > +};

> 

> How about using DEFINE_SHOW_ATTRIBUTE() instead? That will help cut down

> this boiler plate code..

> 


Forgot about that, thank you.

> > +

> > +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++) {

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

> > +			ptr = qcom_smem_get(host, item, NULL);

> > +			if (IS_ERR(ptr))

> > +				continue;

> > +

> > +			sprintf(name, "%ld-%ld", host, item);

> > +

> > +			data = host << 16 | item;

> > +			debugfs_create_file(name, 0400, root,

> > +					    (void *)data, &smem_debugfs_item_ops);

> 

> So IIUC user invokes scan file which creates additional files, right?

> Additional invoke will do that as well..?

> 


Yes, so if you run it a second time debugfs_create_file() will fail for
any items that was present during the last invocation.

I did consider adding some logic to keep track of what items we have
already registered, but it is just debugging code and given that after a
few second of operations the set of items has stabilized you typically
don't run this repeatedly.

So I don't think it's worth the memory occupied by an idr or 5000+ bits
in a map.

> > +		}

> > +	}

> > +

> > +	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,

> > +};

> 

> Here as well?

> 


Will fix.

Thank you,
Bjorn

> > +

> > +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");

> > -- 

> > 2.29.2

> 

> -- 

> ~Vinod
Vinod Koul Nov. 24, 2020, 5:04 p.m. UTC | #3
On 24-11-20, 10:39, Bjorn Andersson wrote:
> On Tue 24 Nov 09:34 CST 2020, Vinod Koul wrote:

> 

> > On 22-11-20, 23:21, 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.

> > > 

> > > 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.

> > 

> > Do we need additional debugfs entry, maybe better to depend on DEBUG_FS

> > being enabled and this file part of QCOM_SMEM?

> > 

> 

> We don't need this in any form of production system, so rather than

> tainting qcom_smem.c I put it in a separate driver that isn't even

> automatically loaded.


Debugfs in production :D

I would leave it to you to decide.. lazy me needs to select another
option!

> > > +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++) {

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

> > > +			ptr = qcom_smem_get(host, item, NULL);

> > > +			if (IS_ERR(ptr))

> > > +				continue;

> > > +

> > > +			sprintf(name, "%ld-%ld", host, item);

> > > +

> > > +			data = host << 16 | item;

> > > +			debugfs_create_file(name, 0400, root,

> > > +					    (void *)data, &smem_debugfs_item_ops);

> > 

> > So IIUC user invokes scan file which creates additional files, right?

> > Additional invoke will do that as well..?

> > 

> 

> Yes, so if you run it a second time debugfs_create_file() will fail for

> any items that was present during the last invocation.

> 

> I did consider adding some logic to keep track of what items we have

> already registered, but it is just debugging code and given that after a

> few second of operations the set of items has stabilized you typically

> don't run this repeatedly.

> 

> So I don't think it's worth the memory occupied by an idr or 5000+ bits

> in a map.


Okay sounds good to me

-- 
~Vinod
Bjorn Andersson Nov. 24, 2020, 5:26 p.m. UTC | #4
On Tue 24 Nov 11:04 CST 2020, Vinod Koul wrote:

> On 24-11-20, 10:39, Bjorn Andersson wrote:

> > On Tue 24 Nov 09:34 CST 2020, Vinod Koul wrote:

> > 

> > > On 22-11-20, 23:21, 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.

> > > > 

> > > > 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.

> > > 

> > > Do we need additional debugfs entry, maybe better to depend on DEBUG_FS

> > > being enabled and this file part of QCOM_SMEM?

> > > 

> > 

> > We don't need this in any form of production system, so rather than

> > tainting qcom_smem.c I put it in a separate driver that isn't even

> > automatically loaded.

> 

> Debugfs in production :D

> 

> I would leave it to you to decide.. lazy me needs to select another

> option!

> 


We can still leave it =m in defconfig, lazy you would have to type
"modprobe qcom_smem_debugfs" when you want this thing.

In all other cases I will waste a little bit of disk, but none of your
ram or cpu cycles.

Regards,
Bjorn

> > > > +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++) {

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

> > > > +			ptr = qcom_smem_get(host, item, NULL);

> > > > +			if (IS_ERR(ptr))

> > > > +				continue;

> > > > +

> > > > +			sprintf(name, "%ld-%ld", host, item);

> > > > +

> > > > +			data = host << 16 | item;

> > > > +			debugfs_create_file(name, 0400, root,

> > > > +					    (void *)data, &smem_debugfs_item_ops);

> > > 

> > > So IIUC user invokes scan file which creates additional files, right?

> > > Additional invoke will do that as well..?

> > > 

> > 

> > Yes, so if you run it a second time debugfs_create_file() will fail for

> > any items that was present during the last invocation.

> > 

> > I did consider adding some logic to keep track of what items we have

> > already registered, but it is just debugging code and given that after a

> > few second of operations the set of items has stabilized you typically

> > don't run this repeatedly.

> > 

> > So I don't think it's worth the memory occupied by an idr or 5000+ bits

> > in a map.

> 

> Okay sounds good to me

> 

> -- 

> ~Vinod
Alex Elder Nov. 25, 2020, 2:45 p.m. UTC | #5
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");

>
diff mbox series

Patch

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;
+};
+
+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;
+	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,
+};
+
+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++) {
+		for (item = 0; item < 512; item++) {
+			ptr = qcom_smem_get(host, item, NULL);
+			if (IS_ERR(ptr))
+				continue;
+
+			sprintf(name, "%ld-%ld", host, item);
+
+			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");