| Message ID | 20201123052119.157551-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: qcom: Introduce debugfs interface to smem | expand |
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
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
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
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
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 --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");
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