[PATCH v1 0/4] Add debugfs support for fastrpc driver

[PATCH v1 0/4] Add debugfs support for fastrpc driver

[Ekansh Gupta]

Following changes are getting added as part of this patch series:
  - Move fastrpc driver to /misc/driver/ to enable maintainance of new
    files to support additional features.
  - Move macro and structure definitions to a new header file. These
    date are consumed by features to be added in new files.
  - Add support for debugfs for fastrpc driver. Most of the fastrpc
    user and channel context information are getting capture as part of
    patch.

Ekansh Gupta (4):
  misc: fastrpc: Move fastrpc driver to misc/fastrpc/
  misc: fastrpc: Rename fastrpc.c to fastrpc_main.c
  misc: fastrpc: Introduce fastrpc_shared.h header
  misc: fastrpc: Add debugfs support for fastrpc

 MAINTAINERS                                   |   2 +-
 drivers/misc/Kconfig                          |  13 +-
 drivers/misc/Makefile                         |   2 +-
 drivers/misc/fastrpc/Kconfig                  |  16 ++
 drivers/misc/fastrpc/Makefile                 |   4 +
 drivers/misc/fastrpc/fastrpc_debug.c          | 156 ++++++++++++++++++
 drivers/misc/fastrpc/fastrpc_debug.h          |  31 ++++
 .../{fastrpc.c => fastrpc/fastrpc_main.c}     | 154 ++---------------
 drivers/misc/fastrpc/fastrpc_shared.h         | 155 +++++++++++++++++
 9 files changed, 382 insertions(+), 151 deletions(-)
 create mode 100644 drivers/misc/fastrpc/Kconfig
 create mode 100644 drivers/misc/fastrpc/Makefile
 create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
 create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
 rename drivers/misc/{fastrpc.c => fastrpc/fastrpc_main.c} (94%)
 create mode 100644 drivers/misc/fastrpc/fastrpc_shared.h

-- 
2.34.1

[PATCH v1 1/4] misc: fastrpc: Move fastrpc driver to misc/fastrpc/

[Ekansh Gupta]

Move fastrpc.c from misc/ to misc/fastrpc/. New C files are planned
to be added for PD notifications and other missing features. Adding
and maintaining new files from within fastrpc directory would be easy.

Example of feature that is being planned to be introduced in a new C
file:
https://lore.kernel.org/all/20240606165939.12950-6-quic_ekangupt@quicinc.com/

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 MAINTAINERS                          |  2 +-
 drivers/misc/Kconfig                 | 13 +------------
 drivers/misc/Makefile                |  2 +-
 drivers/misc/fastrpc/Kconfig         | 16 ++++++++++++++++
 drivers/misc/fastrpc/Makefile        |  2 ++
 drivers/misc/{ => fastrpc}/fastrpc.c |  0
 6 files changed, 21 insertions(+), 14 deletions(-)
 create mode 100644 drivers/misc/fastrpc/Kconfig
 create mode 100644 drivers/misc/fastrpc/Makefile
 rename drivers/misc/{ => fastrpc}/fastrpc.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index b878ddc99f94..280f07d722b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19036,7 +19036,7 @@ L:	linux-arm-msm@vger.kernel.org
 L:	dri-devel@lists.freedesktop.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
-F:	drivers/misc/fastrpc.c
+F:	drivers/misc/fastrpc/
 F:	include/uapi/misc/fastrpc.h
 
 QUALCOMM HEXAGON ARCHITECTURE
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3fe7e2a9bd29..fd02acc3f444 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -286,18 +286,6 @@ config QCOM_COINCELL
 	  to maintain PMIC register and RTC state in the absence of
 	  external power.
 
-config QCOM_FASTRPC
-	tristate "Qualcomm FastRPC"
-	depends on ARCH_QCOM || COMPILE_TEST
-	depends on RPMSG
-	select DMA_SHARED_BUFFER
-	select QCOM_SCM
-	help
-	  Provides a communication mechanism that allows for clients to
-	  make remote method invocations across processor boundary to
-	  applications DSP processor. Say M if you want to enable this
-	  module.
-
 config SGI_GRU
 	tristate "SGI GRU driver"
 	depends on X86_UV && SMP
@@ -628,4 +616,5 @@ source "drivers/misc/uacce/Kconfig"
 source "drivers/misc/pvpanic/Kconfig"
 source "drivers/misc/mchp_pci1xxxx/Kconfig"
 source "drivers/misc/keba/Kconfig"
+source "drivers/misc/fastrpc/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a9f94525e181..efbf69c3b47c 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -17,7 +17,6 @@ obj-$(CONFIG_TIFM_7XX1)       	+= tifm_7xx1.o
 obj-$(CONFIG_PHANTOM)		+= phantom.o
 obj-$(CONFIG_RPMB)		+= rpmb-core.o
 obj-$(CONFIG_QCOM_COINCELL)	+= qcom-coincell.o
-obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
 obj-$(CONFIG_SENSORS_BH1770)	+= bh1770glc.o
 obj-$(CONFIG_SENSORS_APDS990X)	+= apds990x.o
 obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
@@ -72,3 +71,4 @@ obj-$(CONFIG_TPS6594_PFSM)	+= tps6594-pfsm.o
 obj-$(CONFIG_NSM)		+= nsm.o
 obj-$(CONFIG_MARVELL_CN10K_DPI)	+= mrvl_cn10k_dpi.o
 obj-y				+= keba/
+obj-y				+= fastrpc/
diff --git a/drivers/misc/fastrpc/Kconfig b/drivers/misc/fastrpc/Kconfig
new file mode 100644
index 000000000000..7179a44eda84
--- /dev/null
+++ b/drivers/misc/fastrpc/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Qualcomm FastRPC devices
+#
+
+config QCOM_FASTRPC
+	tristate "Qualcomm FastRPC"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on RPMSG
+	select DMA_SHARED_BUFFER
+	select QCOM_SCM
+	help
+	  Provides a communication mechanism that facilitate high-speed
+	  Remote Procedure Call (RPC) mechanisms between the host CPU and
+	  offload processors Qualcomm Digital Signal Processors (DSPs).
+	  Say M if you want to enable this module.
\ No newline at end of file
diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
new file mode 100644
index 000000000000..77fd2b763b6b
--- /dev/null
+++ b/drivers/misc/fastrpc/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc/fastrpc.c
similarity index 100%
rename from drivers/misc/fastrpc.c
rename to drivers/misc/fastrpc/fastrpc.c
-- 
2.34.1

[PATCH v1 2/4] misc: fastrpc: Rename fastrpc.c to fastrpc_main.c

[Ekansh Gupta]

Rename the main fastrpc source file to accomodate new files to be
compiled in the same kernel object.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc/Makefile                      | 1 +
 drivers/misc/fastrpc/{fastrpc.c => fastrpc_main.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/misc/fastrpc/{fastrpc.c => fastrpc_main.c} (100%)

diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
index 77fd2b763b6b..020d30789a80 100644
--- a/drivers/misc/fastrpc/Makefile
+++ b/drivers/misc/fastrpc/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
+fastrpc-objs	:= fastrpc_main.o
\ No newline at end of file
diff --git a/drivers/misc/fastrpc/fastrpc.c b/drivers/misc/fastrpc/fastrpc_main.c
similarity index 100%
rename from drivers/misc/fastrpc/fastrpc.c
rename to drivers/misc/fastrpc/fastrpc_main.c
-- 
2.34.1

Re: [PATCH v1 2/4] misc: fastrpc: Rename fastrpc.c to fastrpc_main.c

[Greg KH]

On Mon, Nov 18, 2024 at 02:10:44PM +0530, Ekansh Gupta wrote:
> Rename the main fastrpc source file to accomodate new files to be
> compiled in the same kernel object.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc/Makefile                      | 1 +
>  drivers/misc/fastrpc/{fastrpc.c => fastrpc_main.c} | 0
>  2 files changed, 1 insertion(+)
>  rename drivers/misc/fastrpc/{fastrpc.c => fastrpc_main.c} (100%)
> 
> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> index 77fd2b763b6b..020d30789a80 100644
> --- a/drivers/misc/fastrpc/Makefile
> +++ b/drivers/misc/fastrpc/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
> +fastrpc-objs	:= fastrpc_main.o
> \ No newline at end of file
> diff --git a/drivers/misc/fastrpc/fastrpc.c b/drivers/misc/fastrpc/fastrpc_main.c
> similarity index 100%
> rename from drivers/misc/fastrpc/fastrpc.c
> rename to drivers/misc/fastrpc/fastrpc_main.c

Why not just "main.c"?  You are in your own subdir, no need for the
fastrpc_* prefix anymore, right?

thanks,

greg "naming is hard" k-h

Re: [PATCH v1 2/4] misc: fastrpc: Rename fastrpc.c to fastrpc_main.c

[Ekansh Gupta]

On 11/18/2024 7:28 PM, Greg KH wrote:
> On Mon, Nov 18, 2024 at 02:10:44PM +0530, Ekansh Gupta wrote:
>> Rename the main fastrpc source file to accomodate new files to be
>> compiled in the same kernel object.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc/Makefile                      | 1 +
>>  drivers/misc/fastrpc/{fastrpc.c => fastrpc_main.c} | 0
>>  2 files changed, 1 insertion(+)
>>  rename drivers/misc/fastrpc/{fastrpc.c => fastrpc_main.c} (100%)
>>
>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>> index 77fd2b763b6b..020d30789a80 100644
>> --- a/drivers/misc/fastrpc/Makefile
>> +++ b/drivers/misc/fastrpc/Makefile
>> @@ -1,2 +1,3 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
>> +fastrpc-objs	:= fastrpc_main.o
>> \ No newline at end of file
>> diff --git a/drivers/misc/fastrpc/fastrpc.c b/drivers/misc/fastrpc/fastrpc_main.c
>> similarity index 100%
>> rename from drivers/misc/fastrpc/fastrpc.c
>> rename to drivers/misc/fastrpc/fastrpc_main.c
> Why not just "main.c"?  You are in your own subdir, no need for the
> fastrpc_* prefix anymore, right?
Ack. I will update the names in next patchset. Thanks.

--ekansh
> thanks,
>
> greg "naming is hard" k-h

[PATCH v1 3/4] misc: fastrpc: Introduce fastrpc_shared.h header

[Ekansh Gupta]

Move fastrpc structures and MACRO definitions to a new header file.
These definitions are consumed by other upcoming features like
debugfs, PDR support etc.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc/fastrpc_main.c   | 136 +---------------------
 drivers/misc/fastrpc/fastrpc_shared.h | 155 ++++++++++++++++++++++++++
 2 files changed, 156 insertions(+), 135 deletions(-)
 create mode 100644 drivers/misc/fastrpc/fastrpc_shared.h

diff --git a/drivers/misc/fastrpc/fastrpc_main.c b/drivers/misc/fastrpc/fastrpc_main.c
index 74181b8c386b..3163b4159de7 100644
--- a/drivers/misc/fastrpc/fastrpc_main.c
+++ b/drivers/misc/fastrpc/fastrpc_main.c
@@ -22,6 +22,7 @@
 #include <linux/firmware/qcom/qcom_scm.h>
 #include <uapi/misc/fastrpc.h>
 #include <linux/of_reserved_mem.h>
+#include "fastrpc_shared.h"
 
 #define ADSP_DOMAIN_ID (0)
 #define MDSP_DOMAIN_ID (1)
@@ -29,8 +30,6 @@
 #define CDSP_DOMAIN_ID (3)
 #define CDSP1_DOMAIN_ID (4)
 #define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
-#define FASTRPC_MAX_SESSIONS	14
-#define FASTRPC_MAX_VMIDS	16
 #define FASTRPC_ALIGN		128
 #define FASTRPC_MAX_FDLIST	16
 #define FASTRPC_MAX_CRCLIST	64
@@ -55,9 +54,6 @@
 #define ADSP_MMAP_ADD_PAGES_LLC 0x3000,
 
 #define DSP_UNSUPPORTED_API (0x80000414)
-/* MAX NUMBER of DSP ATTRIBUTES SUPPORTED */
-#define FASTRPC_MAX_DSP_ATTRIBUTES (256)
-#define FASTRPC_MAX_DSP_ATTRIBUTES_LEN (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES)
 
 /* Retrives number of input buffers from the scalars parameter */
 #define REMOTE_SCALARS_INBUFS(sc)	(((sc) >> 16) & 0x0ff)
@@ -118,22 +114,6 @@ struct fastrpc_invoke_buf {
 	u32 pgidx;		/* index to start of contiguous region */
 };
 
-struct fastrpc_remote_dmahandle {
-	s32 fd;		/* dma handle fd */
-	u32 offset;	/* dma handle offset */
-	u32 len;	/* dma handle length */
-};
-
-struct fastrpc_remote_buf {
-	u64 pv;		/* buffer pointer */
-	u64 len;	/* length of buffer */
-};
-
-union fastrpc_remote_arg {
-	struct fastrpc_remote_buf buf;
-	struct fastrpc_remote_dmahandle dma;
-};
-
 struct fastrpc_mmap_rsp_msg {
 	u64 vaddr;
 };
@@ -168,16 +148,6 @@ struct fastrpc_mem_unmap_req_msg {
 	u64 len;
 };
 
-struct fastrpc_msg {
-	int pid;		/* process group id */
-	int tid;		/* thread id */
-	u64 ctx;		/* invoke caller context */
-	u32 handle;	/* handle to invoke */
-	u32 sc;		/* scalars structure describing the data */
-	u64 addr;		/* physical address */
-	u64 size;		/* size of contiguous region */
-};
-
 struct fastrpc_invoke_rsp {
 	u64 ctx;		/* invoke caller context */
 	int retval;		/* invoke return value */
@@ -192,122 +162,18 @@ struct fastrpc_buf_overlap {
 	u64 offset;
 };
 
-struct fastrpc_buf {
-	struct fastrpc_user *fl;
-	struct dma_buf *dmabuf;
-	struct device *dev;
-	void *virt;
-	u64 phys;
-	u64 size;
-	/* Lock for dma buf attachments */
-	struct mutex lock;
-	struct list_head attachments;
-	/* mmap support */
-	struct list_head node; /* list of user requested mmaps */
-	uintptr_t raddr;
-};
-
 struct fastrpc_dma_buf_attachment {
 	struct device *dev;
 	struct sg_table sgt;
 	struct list_head node;
 };
 
-struct fastrpc_map {
-	struct list_head node;
-	struct fastrpc_user *fl;
-	int fd;
-	struct dma_buf *buf;
-	struct sg_table *table;
-	struct dma_buf_attachment *attach;
-	u64 phys;
-	u64 size;
-	void *va;
-	u64 len;
-	u64 raddr;
-	u32 attr;
-	struct kref refcount;
-};
-
-struct fastrpc_invoke_ctx {
-	int nscalars;
-	int nbufs;
-	int retval;
-	int pid;
-	int tgid;
-	u32 sc;
-	u32 *crc;
-	u64 ctxid;
-	u64 msg_sz;
-	struct kref refcount;
-	struct list_head node; /* list of ctxs */
-	struct completion work;
-	struct work_struct put_work;
-	struct fastrpc_msg msg;
-	struct fastrpc_user *fl;
-	union fastrpc_remote_arg *rpra;
-	struct fastrpc_map **maps;
-	struct fastrpc_buf *buf;
-	struct fastrpc_invoke_args *args;
-	struct fastrpc_buf_overlap *olaps;
-	struct fastrpc_channel_ctx *cctx;
-};
-
-struct fastrpc_session_ctx {
-	struct device *dev;
-	int sid;
-	bool used;
-	bool valid;
-};
-
-struct fastrpc_channel_ctx {
-	int domain_id;
-	int sesscount;
-	int vmcount;
-	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
-	struct rpmsg_device *rpdev;
-	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
-	spinlock_t lock;
-	struct idr ctx_idr;
-	struct list_head users;
-	struct kref refcount;
-	/* Flag if dsp attributes are cached */
-	bool valid_attributes;
-	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
-	struct fastrpc_device *secure_fdevice;
-	struct fastrpc_device *fdevice;
-	struct fastrpc_buf *remote_heap;
-	struct list_head invoke_interrupted_mmaps;
-	bool secure;
-	bool unsigned_support;
-	u64 dma_mask;
-};
-
 struct fastrpc_device {
 	struct fastrpc_channel_ctx *cctx;
 	struct miscdevice miscdev;
 	bool secure;
 };
 
-struct fastrpc_user {
-	struct list_head user;
-	struct list_head maps;
-	struct list_head pending;
-	struct list_head mmaps;
-
-	struct fastrpc_channel_ctx *cctx;
-	struct fastrpc_session_ctx *sctx;
-	struct fastrpc_buf *init_mem;
-
-	int tgid;
-	int pd;
-	bool is_secure_dev;
-	/* Lock for lists */
-	spinlock_t lock;
-	/* lock for allocations */
-	struct mutex mutex;
-};
-
 static void fastrpc_free_map(struct kref *ref)
 {
 	struct fastrpc_map *map;
diff --git a/drivers/misc/fastrpc/fastrpc_shared.h b/drivers/misc/fastrpc/fastrpc_shared.h
new file mode 100644
index 000000000000..8f1f1320a4c2
--- /dev/null
+++ b/drivers/misc/fastrpc/fastrpc_shared.h
@@ -0,0 +1,155 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+// Copyright (c) 2024 Qualcomm Innovation Center.
+#ifndef FASTRPC_SHARED_H
+#define FASTRPC_SHARED_H
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+#include <linux/idr.h>
+#include <linux/list.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+
+#define FASTRPC_MAX_SESSIONS	14
+#define FASTRPC_MAX_VMIDS	16
+/* MAX NUMBER of DSP ATTRIBUTES SUPPORTED */
+#define FASTRPC_MAX_DSP_ATTRIBUTES (256)
+#define FASTRPC_MAX_DSP_ATTRIBUTES_LEN (sizeof(u32) * FASTRPC_MAX_DSP_ATTRIBUTES)
+
+struct fastrpc_remote_dmahandle {
+	s32 fd;		/* dma handle fd */
+	u32 offset;	/* dma handle offset */
+	u32 len;	/* dma handle length */
+};
+
+struct fastrpc_remote_buf {
+	u64 pv;		/* buffer pointer */
+	u64 len;	/* length of buffer */
+};
+
+union fastrpc_remote_arg {
+	struct fastrpc_remote_buf buf;
+	struct fastrpc_remote_dmahandle dma;
+};
+
+struct fastrpc_buf {
+	struct fastrpc_user *fl;
+	struct dma_buf *dmabuf;
+	struct device *dev;
+	void *virt;
+	u64 phys;
+	u64 size;
+	/* Lock for dma buf attachments */
+	struct mutex lock;
+	struct list_head attachments;
+	/* mmap support */
+	struct list_head node; /* list of user requested mmaps */
+	uintptr_t raddr;
+};
+
+struct fastrpc_map {
+	struct list_head node;
+	struct fastrpc_user *fl;
+	int fd;
+	struct dma_buf *buf;
+	struct sg_table *table;
+	struct dma_buf_attachment *attach;
+	u64 phys;
+	u64 size;
+	void *va;
+	u64 len;
+	u64 raddr;
+	u32 attr;
+	struct kref refcount;
+};
+
+struct fastrpc_msg {
+	int pid;		/* process group id */
+	int tid;		/* thread id */
+	u64 ctx;		/* invoke caller context */
+	u32 handle;	/* handle to invoke */
+	u32 sc;		/* scalars structure describing the data */
+	u64 addr;		/* physical address */
+	u64 size;		/* size of contiguous region */
+};
+
+struct fastrpc_invoke_ctx {
+	int nscalars;
+	int nbufs;
+	int retval;
+	int pid;
+	int tgid;
+	u32 sc;
+	u32 *crc;
+	u64 ctxid;
+	u64 msg_sz;
+	struct kref refcount;
+	struct list_head node; /* list of ctxs */
+	struct completion work;
+	struct work_struct put_work;
+	struct fastrpc_msg msg;
+	struct fastrpc_user *fl;
+	union fastrpc_remote_arg *rpra;
+	struct fastrpc_map **maps;
+	struct fastrpc_buf *buf;
+	struct fastrpc_invoke_args *args;
+	struct fastrpc_buf_overlap *olaps;
+	struct fastrpc_channel_ctx *cctx;
+};
+
+struct fastrpc_session_ctx {
+	struct device *dev;
+	int sid;
+	bool used;
+	bool valid;
+};
+
+struct fastrpc_channel_ctx {
+	int domain_id;
+	int sesscount;
+	int vmcount;
+	struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS];
+	struct rpmsg_device *rpdev;
+	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
+	spinlock_t lock;
+	struct idr ctx_idr;
+	struct list_head users;
+	struct kref refcount;
+	/* Flag if dsp attributes are cached */
+	bool valid_attributes;
+	u32 dsp_attributes[FASTRPC_MAX_DSP_ATTRIBUTES];
+	struct fastrpc_device *secure_fdevice;
+	struct fastrpc_device *fdevice;
+	struct fastrpc_buf *remote_heap;
+	struct list_head invoke_interrupted_mmaps;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs_dir;
+#endif
+	bool secure;
+	bool unsigned_support;
+	u64 dma_mask;
+};
+
+struct fastrpc_user {
+	struct list_head user;
+	struct list_head maps;
+	struct list_head pending;
+	struct list_head mmaps;
+
+	struct fastrpc_channel_ctx *cctx;
+	struct fastrpc_session_ctx *sctx;
+	struct fastrpc_buf *init_mem;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *debugfs_file;
+#endif
+
+	int tgid;
+	int pd;
+	bool is_secure_dev;
+	/* Lock for lists */
+	spinlock_t lock;
+	/* lock for allocations */
+	struct mutex mutex;
+};
+
+#endif /* FASTRPC_SHARED_H */
-- 
2.34.1

Re: [PATCH v1 3/4] misc: fastrpc: Introduce fastrpc_shared.h header

[Greg KH]

On Mon, Nov 18, 2024 at 02:10:45PM +0530, Ekansh Gupta wrote:
> Move fastrpc structures and MACRO definitions to a new header file.
> These definitions are consumed by other upcoming features like
> debugfs, PDR support etc.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc/fastrpc_main.c   | 136 +---------------------
>  drivers/misc/fastrpc/fastrpc_shared.h | 155 ++++++++++++++++++++++++++

Again, why not just "fastrpc.h"?  No need for the huge prefix here,
right?

thanks,

greg k-h

Re: [PATCH v1 3/4] misc: fastrpc: Introduce fastrpc_shared.h header

[Ekansh Gupta]

On 11/18/2024 7:28 PM, Greg KH wrote:
> On Mon, Nov 18, 2024 at 02:10:45PM +0530, Ekansh Gupta wrote:
>> Move fastrpc structures and MACRO definitions to a new header file.
>> These definitions are consumed by other upcoming features like
>> debugfs, PDR support etc.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc/fastrpc_main.c   | 136 +---------------------
>>  drivers/misc/fastrpc/fastrpc_shared.h | 155 ++++++++++++++++++++++++++
> Again, why not just "fastrpc.h"?  No need for the huge prefix here,
> right?
yes, right

--ekansh
>
> thanks,
>
> greg k-h

[PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Ekansh Gupta]

Add changes to support debugfs. The fastrpc directory will be
created which will carry debugfs files for all fastrpc processes.
The information of fastrpc user and channel contexts are getting
captured as part of this change.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
 drivers/misc/fastrpc/Makefile        |   3 +-
 drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
 drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
 drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
 4 files changed, 205 insertions(+), 3 deletions(-)
 create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
 create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h

diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
index 020d30789a80..4ff6b64166ae 100644
--- a/drivers/misc/fastrpc/Makefile
+++ b/drivers/misc/fastrpc/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
-fastrpc-objs	:= fastrpc_main.o
\ No newline at end of file
+fastrpc-objs	:= fastrpc_main.o \
+		fastrpc_debug.o
diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
new file mode 100644
index 000000000000..cdb4fc6845a8
--- /dev/null
+++ b/drivers/misc/fastrpc/fastrpc_debug.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2024 Qualcomm Innovation Center.
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include "fastrpc_shared.h"
+#include "fastrpc_debug.h"
+
+#ifdef CONFIG_DEBUG_FS
+
+static void print_buf_info(struct seq_file *s_file, struct fastrpc_buf *buf)
+{
+	seq_printf(s_file, "\n %s %2s 0x%p", "virt", ":", buf->virt);
+	seq_printf(s_file, "\n %s %2s 0x%llx", "phys", ":", buf->phys);
+	seq_printf(s_file, "\n %s %2s 0x%lx", "raddr", ":", buf->raddr);
+	seq_printf(s_file, "\n %s %2s 0x%llx", "size", ":", buf->size);
+}
+
+static void print_ctx_info(struct seq_file *s_file, struct fastrpc_invoke_ctx *ctx)
+{
+	seq_printf(s_file, "\n %s %7s %d", "nscalars", ":", ctx->nscalars);
+	seq_printf(s_file, "\n %s %10s %d", "nbufs", ":", ctx->nbufs);
+	seq_printf(s_file, "\n %s %10s %d", "retval", ":", ctx->retval);
+	seq_printf(s_file, "\n %s %12s %p", "crc", ":", ctx->crc);
+	seq_printf(s_file, "\n %s %12s %d", "pid", ":", ctx->pid);
+	seq_printf(s_file, "\n %s %11s %d", "tgid", ":", ctx->tgid);
+	seq_printf(s_file, "\n %s %13s 0x%x", "sc", ":", ctx->sc);
+	seq_printf(s_file, "\n %s %10s %llu", "ctxid", ":", ctx->ctxid);
+	seq_printf(s_file, "\n %s %9s %llu", "msg_sz", ":", ctx->msg_sz);
+}
+
+static void print_sctx_info(struct seq_file *s_file, struct fastrpc_session_ctx *sctx)
+{
+	seq_printf(s_file, "%s %13s %d\n", "sid", ":", sctx->sid);
+	seq_printf(s_file, "%s %12s %d\n", "used", ":", sctx->used);
+	seq_printf(s_file, "%s %11s %d\n", "valid", ":", sctx->valid);
+}
+
+static void print_cctx_info(struct seq_file *s_file, struct fastrpc_channel_ctx *cctx)
+{
+	seq_printf(s_file, "%s %8s %d\n", "domain_id", ":", cctx->domain_id);
+	seq_printf(s_file, "%s %8s %d\n", "sesscount", ":", cctx->sesscount);
+	seq_printf(s_file, "%s %10s %d\n", "vmcount", ":", cctx->vmcount);
+	seq_printf(s_file, "%s %s %d\n", "valid_attributes", ":", cctx->valid_attributes);
+	seq_printf(s_file, "%s %11s %d\n", "secure", ":", cctx->secure);
+	seq_printf(s_file, "%s %s %d\n", "unsigned_support", ":", cctx->unsigned_support);
+}
+
+static void print_map_info(struct seq_file *s_file, struct fastrpc_map *map)
+{
+	seq_printf(s_file, "%s %4s %d\n", "fd", ":", map->fd);
+	seq_printf(s_file, "%s %s 0x%llx\n", "phys", ":", map->phys);
+	seq_printf(s_file, "%s %s 0x%llx\n", "size", ":", map->size);
+	seq_printf(s_file, "%s %4s 0x%p\n", "va", ":", map->va);
+	seq_printf(s_file, "%s %3s 0x%llx\n", "len", ":", map->len);
+	seq_printf(s_file, "%s %2s 0x%llx\n", "raddr", ":", map->raddr);
+	seq_printf(s_file, "%s %2s 0x%x\n", "attr", ":", map->attr);
+}
+
+static int fastrpc_debugfs_show(struct seq_file *s_file, void *data)
+{
+	struct fastrpc_user *fl = s_file->private;
+	struct fastrpc_map *map;
+	struct fastrpc_channel_ctx *cctx;
+	struct fastrpc_session_ctx *sctx = NULL;
+	struct fastrpc_invoke_ctx *ctx, *m;
+	struct fastrpc_buf *buf;
+
+	if (fl != NULL) {
+		seq_printf(s_file, "%s %12s %d\n", "tgid", ":", fl->tgid);
+		seq_printf(s_file, "%s %3s %d\n", "is_secure_dev", ":", fl->is_secure_dev);
+		seq_printf(s_file, "%s %9s %d\n", "pd_type", ":", fl->pd);
+
+		if (fl->cctx) {
+			seq_puts(s_file, "\n=============== Channel Context ===============\n");
+			cctx = fl->cctx;
+			print_cctx_info(s_file, cctx);
+		}
+		if (fl->sctx) {
+			seq_puts(s_file, "\n=============== Session Context ===============\n");
+			sctx = fl->sctx;
+			print_sctx_info(s_file, sctx);
+		}
+		spin_lock(&fl->lock);
+		if (fl->init_mem) {
+			seq_puts(s_file, "\n=============== Init Mem ===============\n");
+			buf = fl->init_mem;
+			print_buf_info(s_file, buf);
+		}
+		spin_unlock(&fl->lock);
+		seq_puts(s_file, "\n=============== User space maps ===============\n");
+		spin_lock(&fl->lock);
+		list_for_each_entry(map, &fl->maps, node) {
+			if (map)
+				print_map_info(s_file, map);
+		}
+		seq_puts(s_file, "\n=============== Kernel allocated buffers ===============\n");
+		list_for_each_entry(map, &fl->mmaps, node) {
+			if (map)
+				print_map_info(s_file, map);
+		}
+		seq_puts(s_file, "\n=============== Pending contexts ===============\n");
+		list_for_each_entry_safe(ctx, m, &fl->pending, node) {
+			if (ctx)
+				print_ctx_info(s_file, ctx);
+		}
+		spin_unlock(&fl->lock);
+	}
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(fastrpc_debugfs);
+
+void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
+{
+	char cur_comm[TASK_COMM_LEN];
+	int domain_id, size;
+	char *debugfs_buf;
+	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
+
+	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
+	cur_comm[TASK_COMM_LEN-1] = '\0';
+	if (debugfs_dir != NULL) {
+		domain_id = fl->cctx->domain_id;
+		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
+				current->pid, fl->tgid, domain_id) + 1;
+		debugfs_buf = kzalloc(size, GFP_KERNEL);
+		if (debugfs_buf == NULL)
+			return;
+		/*
+		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
+		 * domain_id in debugfs filename to create unique file name
+		 */
+		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
+			cur_comm, current->pid, fl->tgid, domain_id);
+		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
+				debugfs_dir, fl, &fastrpc_debugfs_fops);
+		kfree(debugfs_buf);
+	}
+}
+
+void fastrpc_remove_user_debugfs(struct fastrpc_user *fl)
+{
+	debugfs_remove(fl->debugfs_file);
+}
+
+struct dentry *fastrpc_create_debugfs_dir(const char *name)
+{
+	return debugfs_create_dir(name, NULL);
+}
+
+void fastrpc_remove_debugfs_dir(struct dentry *cctx_debugfs)
+{
+	debugfs_remove_recursive(cctx_debugfs);
+}
+#endif
diff --git a/drivers/misc/fastrpc/fastrpc_debug.h b/drivers/misc/fastrpc/fastrpc_debug.h
new file mode 100644
index 000000000000..916a5c668308
--- /dev/null
+++ b/drivers/misc/fastrpc/fastrpc_debug.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+// Copyright (c) 2024 Qualcomm Innovation Center.
+#ifndef FASTRPC_DEBUG_H
+#define FASTRPC_DEBUG_H
+
+#include <linux/debugfs.h>
+#include "fastrpc_shared.h"
+#include "fastrpc_debug.h"
+
+#define DEBUGFS_DIRLEN	16
+#ifdef CONFIG_DEBUG_FS
+void fastrpc_create_user_debugfs(struct fastrpc_user *fl);
+void fastrpc_remove_user_debugfs(struct fastrpc_user *fl);
+struct dentry *fastrpc_create_debugfs_dir(const char *name);
+void fastrpc_remove_debugfs_dir(struct dentry *cctx_debugfs);
+#else
+static inline void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
+{
+}
+static inline void fastrpc_remove_user_debugfs(struct fastrpc_user *fl)
+{
+}
+static inline struct dentry *fastrpc_create_debugfs_dir(const char *name)
+{
+	return NULL;
+}
+static inline void fastrpc_remove_debugfs_dir(struct dentry *cctx_debugfs)
+{
+}
+#endif	/* CONFIG_DEBUG_FS */
+#endif	/* FASTRPC_DEBUG_H */
diff --git a/drivers/misc/fastrpc/fastrpc_main.c b/drivers/misc/fastrpc/fastrpc_main.c
index 3163b4159de7..f758af1d2f8e 100644
--- a/drivers/misc/fastrpc/fastrpc_main.c
+++ b/drivers/misc/fastrpc/fastrpc_main.c
@@ -23,6 +23,7 @@
 #include <uapi/misc/fastrpc.h>
 #include <linux/of_reserved_mem.h>
 #include "fastrpc_shared.h"
+#include "fastrpc_debug.h"
 
 #define ADSP_DOMAIN_ID (0)
 #define MDSP_DOMAIN_ID (1)
@@ -1185,6 +1186,7 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 				      sc, args);
 	if (err)
 		goto err_invoke;
+	fastrpc_create_user_debugfs(fl);
 
 	kfree(args);
 	kfree(name);
@@ -1318,6 +1320,7 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 				      sc, args);
 	if (err)
 		goto err_invoke;
+	fastrpc_create_user_debugfs(fl);
 
 	kfree(args);
 
@@ -1412,6 +1415,7 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	}
 
 	fastrpc_session_free(cctx, fl->sctx);
+	fastrpc_remove_user_debugfs(fl);
 	fastrpc_channel_ctx_put(cctx);
 
 	mutex_destroy(&fl->mutex);
@@ -1513,7 +1517,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
 {
 	struct fastrpc_invoke_args args[1];
-	int tgid = fl->tgid;
+	int err, tgid = fl->tgid;
 	u32 sc;
 
 	args[0].ptr = (u64)(uintptr_t) &tgid;
@@ -1522,8 +1526,13 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
 	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
 	fl->pd = pd;
 
-	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
+	err = fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
 				       sc, &args[0]);
+	if (err)
+		return err;
+	fastrpc_create_user_debugfs(fl);
+
+	return 0;
 }
 
 static int fastrpc_invoke(struct fastrpc_user *fl, char __user *argp)
@@ -2125,6 +2134,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	struct fastrpc_channel_ctx *data;
 	int i, err, domain_id = -1, vmcount;
 	const char *domain;
+	char dir_name[DEBUGFS_DIRLEN];
 	bool secure_dsp;
 	struct device_node *rmem_node;
 	struct reserved_mem *rmem;
@@ -2228,6 +2238,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	idr_init(&data->ctx_idr);
 	data->domain_id = domain_id;
 	data->rpdev = rpdev;
+	snprintf(dir_name, sizeof(dir_name), "%s_%s", "fastrpc", domain);
+	data->debugfs_dir = fastrpc_create_debugfs_dir(dir_name);
 
 	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
 	if (err)
@@ -2284,6 +2296,8 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev)
 	if (cctx->remote_heap)
 		fastrpc_buf_free(cctx->remote_heap);
 
+	fastrpc_remove_debugfs_dir(cctx->debugfs_dir);
+
 	of_platform_depopulate(&rpdev->dev);
 
 	fastrpc_channel_ctx_put(cctx);
-- 
2.34.1

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Greg KH]

On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> Add changes to support debugfs. The fastrpc directory will be
> created which will carry debugfs files for all fastrpc processes.
> The information of fastrpc user and channel contexts are getting
> captured as part of this change.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
>  drivers/misc/fastrpc/Makefile        |   3 +-
>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>  4 files changed, 205 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> 
> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> index 020d30789a80..4ff6b64166ae 100644
> --- a/drivers/misc/fastrpc/Makefile
> +++ b/drivers/misc/fastrpc/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
> -fastrpc-objs	:= fastrpc_main.o
> \ No newline at end of file
> +fastrpc-objs	:= fastrpc_main.o \
> +		fastrpc_debug.o

Only build this file if debugfs is enabled.

And again, "debug.c"?

> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> new file mode 100644
> index 000000000000..cdb4fc6845a8
> --- /dev/null
> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2024 Qualcomm Innovation Center.
> +
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include "fastrpc_shared.h"
> +#include "fastrpc_debug.h"
> +
> +#ifdef CONFIG_DEBUG_FS

Please put the #ifdef in the .h file, not in the .c file.

> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> +{
> +	char cur_comm[TASK_COMM_LEN];
> +	int domain_id, size;
> +	char *debugfs_buf;
> +	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> +
> +	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> +	cur_comm[TASK_COMM_LEN-1] = '\0';
> +	if (debugfs_dir != NULL) {
> +		domain_id = fl->cctx->domain_id;
> +		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> +				current->pid, fl->tgid, domain_id) + 1;
> +		debugfs_buf = kzalloc(size, GFP_KERNEL);
> +		if (debugfs_buf == NULL)
> +			return;
> +		/*
> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> +		 * domain_id in debugfs filename to create unique file name
> +		 */
> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> +			cur_comm, current->pid, fl->tgid, domain_id);
> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> +				debugfs_dir, fl, &fastrpc_debugfs_fops);

Why are you saving the debugfs file?  What do you need to do with it
that you can't just delete the whole directory, or look up the name
again in the future when removing it?

> +		kfree(debugfs_buf);
> +	}
> +}
> +
> +void fastrpc_remove_user_debugfs(struct fastrpc_user *fl)
> +{
> +	debugfs_remove(fl->debugfs_file);

Why remove just the file and not the whole directory?

> +}
> +
> +struct dentry *fastrpc_create_debugfs_dir(const char *name)
> +{
> +	return debugfs_create_dir(name, NULL);

At the root of debugfs?  Why is this function even needed?

> +}
> +
> +void fastrpc_remove_debugfs_dir(struct dentry *cctx_debugfs)
> +{
> +	debugfs_remove_recursive(cctx_debugfs);

See, you don't need the debugfs file reference at all, you don't do
anything with it.

And again, why are you wrapping basic debugfs functions with your own
version?  Please don't do that.

thanks,

greg k-h

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Ekansh Gupta]

On 11/18/2024 7:32 PM, Greg KH wrote:
> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>> Add changes to support debugfs. The fastrpc directory will be
>> created which will carry debugfs files for all fastrpc processes.
>> The information of fastrpc user and channel contexts are getting
>> captured as part of this change.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>>  drivers/misc/fastrpc/Makefile        |   3 +-
>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>  4 files changed, 205 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>
>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>> index 020d30789a80..4ff6b64166ae 100644
>> --- a/drivers/misc/fastrpc/Makefile
>> +++ b/drivers/misc/fastrpc/Makefile
>> @@ -1,3 +1,4 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
>> -fastrpc-objs	:= fastrpc_main.o
>> \ No newline at end of file
>> +fastrpc-objs	:= fastrpc_main.o \
>> +		fastrpc_debug.o
> Only build this file if debugfs is enabled.
>
> And again, "debug.c"?
I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
few more debug specific changes, maybe then I'll need to change the build rules again.
>
>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>> new file mode 100644
>> index 000000000000..cdb4fc6845a8
>> --- /dev/null
>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/seq_file.h>
>> +#include "fastrpc_shared.h"
>> +#include "fastrpc_debug.h"
>> +
>> +#ifdef CONFIG_DEBUG_FS
> Please put the #ifdef in the .h file, not in the .c file.
Ack
>
>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>> +{
>> +	char cur_comm[TASK_COMM_LEN];
>> +	int domain_id, size;
>> +	char *debugfs_buf;
>> +	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>> +
>> +	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>> +	cur_comm[TASK_COMM_LEN-1] = '\0';
>> +	if (debugfs_dir != NULL) {
>> +		domain_id = fl->cctx->domain_id;
>> +		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>> +				current->pid, fl->tgid, domain_id) + 1;
>> +		debugfs_buf = kzalloc(size, GFP_KERNEL);
>> +		if (debugfs_buf == NULL)
>> +			return;
>> +		/*
>> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>> +		 * domain_id in debugfs filename to create unique file name
>> +		 */
>> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>> +			cur_comm, current->pid, fl->tgid, domain_id);
>> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>> +				debugfs_dir, fl, &fastrpc_debugfs_fops);
> Why are you saving the debugfs file?  What do you need to do with it
> that you can't just delete the whole directory, or look up the name
> again in the future when removing it?
fl structure is specific to a process using fastrpc driver. The reason to save
this debugfs file is to delete is when the process releases fastrpc device.
If the file is not deleted, it might flood multiple files in debugfs directory.

As part of this change, only the file that is getting created by a process is
getting removed when process is releasing device and I don't think we
can clean up the whole directory at this point.

Do you suggest that looking up the name is a better approach that saving
the file?
>
>> +		kfree(debugfs_buf);
>> +	}
>> +}
>> +
>> +void fastrpc_remove_user_debugfs(struct fastrpc_user *fl)
>> +{
>> +	debugfs_remove(fl->debugfs_file);
> Why remove just the file and not the whole directory?
As mentioned above, file process specific and is getting created when
any process uses fastrpc driver. It is getting deleted when the process
releases the device.
>
>> +}
>> +
>> +struct dentry *fastrpc_create_debugfs_dir(const char *name)
>> +{
>> +	return debugfs_create_dir(name, NULL);
> At the root of debugfs?  Why is this function even needed?
creating a dir named "fastrpc_adsp", "fastrpc_cdsp" etc. to create debugfs
file for the processes using adsp, cdsp etc.
>
>> +}
>> +
>> +void fastrpc_remove_debugfs_dir(struct dentry *cctx_debugfs)
>> +{
>> +	debugfs_remove_recursive(cctx_debugfs);
> See, you don't need the debugfs file reference at all, you don't do
> anything with it.
right, I can try with look up.
> And again, why are you wrapping basic debugfs functions with your own
> version?  Please don't do that.
Ack. Thanks for reviewing this change.

--ekansh
>
> thanks,
>
> greg k-h

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Greg KH]

On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> On 11/18/2024 7:32 PM, Greg KH wrote:
> > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> >> +		/*
> >> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> >> +		 * domain_id in debugfs filename to create unique file name
> >> +		 */
> >> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> >> +			cur_comm, current->pid, fl->tgid, domain_id);
> >> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> >> +				debugfs_dir, fl, &fastrpc_debugfs_fops);
> > Why are you saving the debugfs file?  What do you need to do with it
> > that you can't just delete the whole directory, or look up the name
> > again in the future when removing it?
> fl structure is specific to a process using fastrpc driver. The reason to save
> this debugfs file is to delete is when the process releases fastrpc device.
> If the file is not deleted, it might flood multiple files in debugfs directory.
> 
> As part of this change, only the file that is getting created by a process is
> getting removed when process is releasing device and I don't think we
> can clean up the whole directory at this point.
> 
> Do you suggest that looking up the name is a better approach that saving
> the file?

Yes.

> >> +}
> >> +
> >> +struct dentry *fastrpc_create_debugfs_dir(const char *name)
> >> +{
> >> +	return debugfs_create_dir(name, NULL);
> > At the root of debugfs?  Why is this function even needed?
> creating a dir named "fastrpc_adsp", "fastrpc_cdsp" etc. to create debugfs
> file for the processes using adsp, cdsp etc.

Then just call debugfs_create_dir() you do not need a wrapper function
for this.

thanks,

greg k-h

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> 
> 
> On 11/18/2024 7:32 PM, Greg KH wrote:
> > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> >> Add changes to support debugfs. The fastrpc directory will be
> >> created which will carry debugfs files for all fastrpc processes.
> >> The information of fastrpc user and channel contexts are getting
> >> captured as part of this change.
> >>
> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >> ---
> >>  drivers/misc/fastrpc/Makefile        |   3 +-
> >>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> >>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> >>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> >>  4 files changed, 205 insertions(+), 3 deletions(-)
> >>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> >>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> >>
> >> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> >> index 020d30789a80..4ff6b64166ae 100644
> >> --- a/drivers/misc/fastrpc/Makefile
> >> +++ b/drivers/misc/fastrpc/Makefile
> >> @@ -1,3 +1,4 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
> >> -fastrpc-objs	:= fastrpc_main.o
> >> \ No newline at end of file
> >> +fastrpc-objs	:= fastrpc_main.o \
> >> +		fastrpc_debug.o
> > Only build this file if debugfs is enabled.
> >
> > And again, "debug.c"?
> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> few more debug specific changes, maybe then I'll need to change the build rules again.
> >
> >> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> >> new file mode 100644
> >> index 000000000000..cdb4fc6845a8
> >> --- /dev/null
> >> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> >> @@ -0,0 +1,156 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// Copyright (c) 2024 Qualcomm Innovation Center.
> >> +
> >> +#include <linux/debugfs.h>
> >> +#include <linux/seq_file.h>
> >> +#include "fastrpc_shared.h"
> >> +#include "fastrpc_debug.h"
> >> +
> >> +#ifdef CONFIG_DEBUG_FS
> > Please put the #ifdef in the .h file, not in the .c file.
> Ack
> >
> >> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> >> +{
> >> +	char cur_comm[TASK_COMM_LEN];
> >> +	int domain_id, size;
> >> +	char *debugfs_buf;
> >> +	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> >> +
> >> +	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> >> +	cur_comm[TASK_COMM_LEN-1] = '\0';
> >> +	if (debugfs_dir != NULL) {
> >> +		domain_id = fl->cctx->domain_id;
> >> +		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> >> +				current->pid, fl->tgid, domain_id) + 1;
> >> +		debugfs_buf = kzalloc(size, GFP_KERNEL);
> >> +		if (debugfs_buf == NULL)
> >> +			return;
> >> +		/*
> >> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> >> +		 * domain_id in debugfs filename to create unique file name
> >> +		 */
> >> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> >> +			cur_comm, current->pid, fl->tgid, domain_id);
> >> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> >> +				debugfs_dir, fl, &fastrpc_debugfs_fops);
> > Why are you saving the debugfs file?  What do you need to do with it
> > that you can't just delete the whole directory, or look up the name
> > again in the future when removing it?
> fl structure is specific to a process using fastrpc driver. The reason to save
> this debugfs file is to delete is when the process releases fastrpc device.
> If the file is not deleted, it might flood multiple files in debugfs directory.
> 
> As part of this change, only the file that is getting created by a process is
> getting removed when process is releasing device and I don't think we
> can clean up the whole directory at this point.

My 2c: it might be better to create a single file that conains
information for all the processes instead of that. Or use fdinfo data to
export process / FD information to userspace.


-- 
With best wishes
Dmitry

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Ekansh Gupta]

On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
>>
>> On 11/18/2024 7:32 PM, Greg KH wrote:
>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>>>> Add changes to support debugfs. The fastrpc directory will be
>>>> created which will carry debugfs files for all fastrpc processes.
>>>> The information of fastrpc user and channel contexts are getting
>>>> captured as part of this change.
>>>>
>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>> ---
>>>>  drivers/misc/fastrpc/Makefile        |   3 +-
>>>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>>>  4 files changed, 205 insertions(+), 3 deletions(-)
>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>>>
>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>>>> index 020d30789a80..4ff6b64166ae 100644
>>>> --- a/drivers/misc/fastrpc/Makefile
>>>> +++ b/drivers/misc/fastrpc/Makefile
>>>> @@ -1,3 +1,4 @@
>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
>>>> -fastrpc-objs	:= fastrpc_main.o
>>>> \ No newline at end of file
>>>> +fastrpc-objs	:= fastrpc_main.o \
>>>> +		fastrpc_debug.o
>>> Only build this file if debugfs is enabled.
>>>
>>> And again, "debug.c"?
>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
>> few more debug specific changes, maybe then I'll need to change the build rules again.
>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>>>> new file mode 100644
>>>> index 000000000000..cdb4fc6845a8
>>>> --- /dev/null
>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>>>> @@ -0,0 +1,156 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>>>> +
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/seq_file.h>
>>>> +#include "fastrpc_shared.h"
>>>> +#include "fastrpc_debug.h"
>>>> +
>>>> +#ifdef CONFIG_DEBUG_FS
>>> Please put the #ifdef in the .h file, not in the .c file.
>> Ack
>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>>>> +{
>>>> +	char cur_comm[TASK_COMM_LEN];
>>>> +	int domain_id, size;
>>>> +	char *debugfs_buf;
>>>> +	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>>>> +
>>>> +	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>>>> +	cur_comm[TASK_COMM_LEN-1] = '\0';
>>>> +	if (debugfs_dir != NULL) {
>>>> +		domain_id = fl->cctx->domain_id;
>>>> +		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>>>> +				current->pid, fl->tgid, domain_id) + 1;
>>>> +		debugfs_buf = kzalloc(size, GFP_KERNEL);
>>>> +		if (debugfs_buf == NULL)
>>>> +			return;
>>>> +		/*
>>>> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>>>> +		 * domain_id in debugfs filename to create unique file name
>>>> +		 */
>>>> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>>>> +			cur_comm, current->pid, fl->tgid, domain_id);
>>>> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>>>> +				debugfs_dir, fl, &fastrpc_debugfs_fops);
>>> Why are you saving the debugfs file?  What do you need to do with it
>>> that you can't just delete the whole directory, or look up the name
>>> again in the future when removing it?
>> fl structure is specific to a process using fastrpc driver. The reason to save
>> this debugfs file is to delete is when the process releases fastrpc device.
>> If the file is not deleted, it might flood multiple files in debugfs directory.
>>
>> As part of this change, only the file that is getting created by a process is
>> getting removed when process is releasing device and I don't think we
>> can clean up the whole directory at this point.
> My 2c: it might be better to create a single file that conains
> information for all the processes instead of that. Or use fdinfo data to
> export process / FD information to userspace.
Thanks for your review. The reason of not having single file for all processes is that
I can run 100s of iteration for any process(say calculator) and every time the properties
of the process can differ(like buffer, session etc.). For this reason, I'm creating and
deleting the debugfs files for every process run.

Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
the information(like in debugfs) here.

--ekansh
>
>

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> 
> 
> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> >>
> >> On 11/18/2024 7:32 PM, Greg KH wrote:
> >>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> >>>> Add changes to support debugfs. The fastrpc directory will be
> >>>> created which will carry debugfs files for all fastrpc processes.
> >>>> The information of fastrpc user and channel contexts are getting
> >>>> captured as part of this change.
> >>>>
> >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >>>> ---
> >>>>  drivers/misc/fastrpc/Makefile        |   3 +-
> >>>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> >>>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> >>>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> >>>>  4 files changed, 205 insertions(+), 3 deletions(-)
> >>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> >>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> >>>> index 020d30789a80..4ff6b64166ae 100644
> >>>> --- a/drivers/misc/fastrpc/Makefile
> >>>> +++ b/drivers/misc/fastrpc/Makefile
> >>>> @@ -1,3 +1,4 @@
> >>>>  # SPDX-License-Identifier: GPL-2.0
> >>>>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
> >>>> -fastrpc-objs	:= fastrpc_main.o
> >>>> \ No newline at end of file
> >>>> +fastrpc-objs	:= fastrpc_main.o \
> >>>> +		fastrpc_debug.o
> >>> Only build this file if debugfs is enabled.
> >>>
> >>> And again, "debug.c"?
> >> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> >> few more debug specific changes, maybe then I'll need to change the build rules again.
> >>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>> new file mode 100644
> >>>> index 000000000000..cdb4fc6845a8
> >>>> --- /dev/null
> >>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>> @@ -0,0 +1,156 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
> >>>> +
> >>>> +#include <linux/debugfs.h>
> >>>> +#include <linux/seq_file.h>
> >>>> +#include "fastrpc_shared.h"
> >>>> +#include "fastrpc_debug.h"
> >>>> +
> >>>> +#ifdef CONFIG_DEBUG_FS
> >>> Please put the #ifdef in the .h file, not in the .c file.
> >> Ack
> >>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> >>>> +{
> >>>> +	char cur_comm[TASK_COMM_LEN];
> >>>> +	int domain_id, size;
> >>>> +	char *debugfs_buf;
> >>>> +	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> >>>> +
> >>>> +	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> >>>> +	cur_comm[TASK_COMM_LEN-1] = '\0';
> >>>> +	if (debugfs_dir != NULL) {
> >>>> +		domain_id = fl->cctx->domain_id;
> >>>> +		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> >>>> +				current->pid, fl->tgid, domain_id) + 1;
> >>>> +		debugfs_buf = kzalloc(size, GFP_KERNEL);
> >>>> +		if (debugfs_buf == NULL)
> >>>> +			return;
> >>>> +		/*
> >>>> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> >>>> +		 * domain_id in debugfs filename to create unique file name
> >>>> +		 */
> >>>> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> >>>> +			cur_comm, current->pid, fl->tgid, domain_id);
> >>>> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> >>>> +				debugfs_dir, fl, &fastrpc_debugfs_fops);
> >>> Why are you saving the debugfs file?  What do you need to do with it
> >>> that you can't just delete the whole directory, or look up the name
> >>> again in the future when removing it?
> >> fl structure is specific to a process using fastrpc driver. The reason to save
> >> this debugfs file is to delete is when the process releases fastrpc device.
> >> If the file is not deleted, it might flood multiple files in debugfs directory.
> >>
> >> As part of this change, only the file that is getting created by a process is
> >> getting removed when process is releasing device and I don't think we
> >> can clean up the whole directory at this point.
> > My 2c: it might be better to create a single file that conains
> > information for all the processes instead of that. Or use fdinfo data to
> > export process / FD information to userspace.
> Thanks for your review. The reason of not having single file for all processes is that
> I can run 100s of iteration for any process(say calculator) and every time the properties
> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> deleting the debugfs files for every process run.
> 
> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> the information(like in debugfs) here.

Which information is actually useful / interesting for application
developers? If not for the fdinfo, I might still vote for a single file
rather than a pile of per-process data.

> 
> --ekansh
> >
> >
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Ekansh Gupta]

On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
>>
>> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
>>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
>>>> On 11/18/2024 7:32 PM, Greg KH wrote:
>>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>>>>>> Add changes to support debugfs. The fastrpc directory will be
>>>>>> created which will carry debugfs files for all fastrpc processes.
>>>>>> The information of fastrpc user and channel contexts are getting
>>>>>> captured as part of this change.
>>>>>>
>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>> ---
>>>>>>  drivers/misc/fastrpc/Makefile        |   3 +-
>>>>>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>>>>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>>>>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>>>>>  4 files changed, 205 insertions(+), 3 deletions(-)
>>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>>>>>
>>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>>>>>> index 020d30789a80..4ff6b64166ae 100644
>>>>>> --- a/drivers/misc/fastrpc/Makefile
>>>>>> +++ b/drivers/misc/fastrpc/Makefile
>>>>>> @@ -1,3 +1,4 @@
>>>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>>>  obj-$(CONFIG_QCOM_FASTRPC)	+= fastrpc.o
>>>>>> -fastrpc-objs	:= fastrpc_main.o
>>>>>> \ No newline at end of file
>>>>>> +fastrpc-objs	:= fastrpc_main.o \
>>>>>> +		fastrpc_debug.o
>>>>> Only build this file if debugfs is enabled.
>>>>>
>>>>> And again, "debug.c"?
>>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
>>>> few more debug specific changes, maybe then I'll need to change the build rules again.
>>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..cdb4fc6845a8
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>> @@ -0,0 +1,156 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>>>>>> +
>>>>>> +#include <linux/debugfs.h>
>>>>>> +#include <linux/seq_file.h>
>>>>>> +#include "fastrpc_shared.h"
>>>>>> +#include "fastrpc_debug.h"
>>>>>> +
>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>> Please put the #ifdef in the .h file, not in the .c file.
>>>> Ack
>>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>>>>>> +{
>>>>>> +	char cur_comm[TASK_COMM_LEN];
>>>>>> +	int domain_id, size;
>>>>>> +	char *debugfs_buf;
>>>>>> +	struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>>>>>> +
>>>>>> +	memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>>>>>> +	cur_comm[TASK_COMM_LEN-1] = '\0';
>>>>>> +	if (debugfs_dir != NULL) {
>>>>>> +		domain_id = fl->cctx->domain_id;
>>>>>> +		size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>>>>>> +				current->pid, fl->tgid, domain_id) + 1;
>>>>>> +		debugfs_buf = kzalloc(size, GFP_KERNEL);
>>>>>> +		if (debugfs_buf == NULL)
>>>>>> +			return;
>>>>>> +		/*
>>>>>> +		 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>>>>>> +		 * domain_id in debugfs filename to create unique file name
>>>>>> +		 */
>>>>>> +		snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>>>>>> +			cur_comm, current->pid, fl->tgid, domain_id);
>>>>>> +		fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>>>>>> +				debugfs_dir, fl, &fastrpc_debugfs_fops);
>>>>> Why are you saving the debugfs file?  What do you need to do with it
>>>>> that you can't just delete the whole directory, or look up the name
>>>>> again in the future when removing it?
>>>> fl structure is specific to a process using fastrpc driver. The reason to save
>>>> this debugfs file is to delete is when the process releases fastrpc device.
>>>> If the file is not deleted, it might flood multiple files in debugfs directory.
>>>>
>>>> As part of this change, only the file that is getting created by a process is
>>>> getting removed when process is releasing device and I don't think we
>>>> can clean up the whole directory at this point.
>>> My 2c: it might be better to create a single file that conains
>>> information for all the processes instead of that. Or use fdinfo data to
>>> export process / FD information to userspace.
>> Thanks for your review. The reason of not having single file for all processes is that
>> I can run 100s of iteration for any process(say calculator) and every time the properties
>> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
>> deleting the debugfs files for every process run.
>>
>> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
>> the information(like in debugfs) here.
> Which information is actually useful / interesting for application
> developers? If not for the fdinfo, I might still vote for a single file
> rather than a pile of per-process data.
I have tried to capture all the information that could be useful.

I can try changes to maintain single file for all active processes. Having this file specific
to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
carry information of all processes using that remoteproc.

--ekansh
>
>> --ekansh
>>>

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>
>
>
> On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> >>
> >> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> >>>> On 11/18/2024 7:32 PM, Greg KH wrote:
> >>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> >>>>>> Add changes to support debugfs. The fastrpc directory will be
> >>>>>> created which will carry debugfs files for all fastrpc processes.
> >>>>>> The information of fastrpc user and channel contexts are getting
> >>>>>> captured as part of this change.
> >>>>>>
> >>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >>>>>> ---
> >>>>>>  drivers/misc/fastrpc/Makefile        |   3 +-
> >>>>>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> >>>>>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> >>>>>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> >>>>>>  4 files changed, 205 insertions(+), 3 deletions(-)
> >>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> >>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> >>>>>>
> >>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> >>>>>> index 020d30789a80..4ff6b64166ae 100644
> >>>>>> --- a/drivers/misc/fastrpc/Makefile
> >>>>>> +++ b/drivers/misc/fastrpc/Makefile
> >>>>>> @@ -1,3 +1,4 @@
> >>>>>>  # SPDX-License-Identifier: GPL-2.0
> >>>>>>  obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> >>>>>> -fastrpc-objs    := fastrpc_main.o
> >>>>>> \ No newline at end of file
> >>>>>> +fastrpc-objs    := fastrpc_main.o \
> >>>>>> +                fastrpc_debug.o
> >>>>> Only build this file if debugfs is enabled.
> >>>>>
> >>>>> And again, "debug.c"?
> >>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> >>>> few more debug specific changes, maybe then I'll need to change the build rules again.
> >>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..cdb4fc6845a8
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>>>> @@ -0,0 +1,156 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
> >>>>>> +
> >>>>>> +#include <linux/debugfs.h>
> >>>>>> +#include <linux/seq_file.h>
> >>>>>> +#include "fastrpc_shared.h"
> >>>>>> +#include "fastrpc_debug.h"
> >>>>>> +
> >>>>>> +#ifdef CONFIG_DEBUG_FS
> >>>>> Please put the #ifdef in the .h file, not in the .c file.
> >>>> Ack
> >>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> >>>>>> +{
> >>>>>> +        char cur_comm[TASK_COMM_LEN];
> >>>>>> +        int domain_id, size;
> >>>>>> +        char *debugfs_buf;
> >>>>>> +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> >>>>>> +
> >>>>>> +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> >>>>>> +        cur_comm[TASK_COMM_LEN-1] = '\0';
> >>>>>> +        if (debugfs_dir != NULL) {
> >>>>>> +                domain_id = fl->cctx->domain_id;
> >>>>>> +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> >>>>>> +                                current->pid, fl->tgid, domain_id) + 1;
> >>>>>> +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> >>>>>> +                if (debugfs_buf == NULL)
> >>>>>> +                        return;
> >>>>>> +                /*
> >>>>>> +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> >>>>>> +                 * domain_id in debugfs filename to create unique file name
> >>>>>> +                 */
> >>>>>> +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> >>>>>> +                        cur_comm, current->pid, fl->tgid, domain_id);
> >>>>>> +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> >>>>>> +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> >>>>> Why are you saving the debugfs file?  What do you need to do with it
> >>>>> that you can't just delete the whole directory, or look up the name
> >>>>> again in the future when removing it?
> >>>> fl structure is specific to a process using fastrpc driver. The reason to save
> >>>> this debugfs file is to delete is when the process releases fastrpc device.
> >>>> If the file is not deleted, it might flood multiple files in debugfs directory.
> >>>>
> >>>> As part of this change, only the file that is getting created by a process is
> >>>> getting removed when process is releasing device and I don't think we
> >>>> can clean up the whole directory at this point.
> >>> My 2c: it might be better to create a single file that conains
> >>> information for all the processes instead of that. Or use fdinfo data to
> >>> export process / FD information to userspace.
> >> Thanks for your review. The reason of not having single file for all processes is that
> >> I can run 100s of iteration for any process(say calculator) and every time the properties
> >> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> >> deleting the debugfs files for every process run.
> >>
> >> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> >> the information(like in debugfs) here.
> > Which information is actually useful / interesting for application
> > developers? If not for the fdinfo, I might still vote for a single file
> > rather than a pile of per-process data.
> I have tried to capture all the information that could be useful.
>
> I can try changes to maintain single file for all active processes. Having this file specific
> to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
> carry information of all processes using that remoteproc.

I think it's a better idea, yes.

>
> --ekansh
> >
> >> --ekansh
> >>>
>


-- 
With best wishes
Dmitry

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Ekansh Gupta]

On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>>
>>
>> On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
>>>> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
>>>>>> On 11/18/2024 7:32 PM, Greg KH wrote:
>>>>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>>>>>>>> Add changes to support debugfs. The fastrpc directory will be
>>>>>>>> created which will carry debugfs files for all fastrpc processes.
>>>>>>>> The information of fastrpc user and channel contexts are getting
>>>>>>>> captured as part of this change.
>>>>>>>>
>>>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>>>> ---
>>>>>>>>  drivers/misc/fastrpc/Makefile        |   3 +-
>>>>>>>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>>>>>>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>>>>>>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>>>>>>>  4 files changed, 205 insertions(+), 3 deletions(-)
>>>>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>>>>>>>
>>>>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>>>>>>>> index 020d30789a80..4ff6b64166ae 100644
>>>>>>>> --- a/drivers/misc/fastrpc/Makefile
>>>>>>>> +++ b/drivers/misc/fastrpc/Makefile
>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>>>>>  obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
>>>>>>>> -fastrpc-objs    := fastrpc_main.o
>>>>>>>> \ No newline at end of file
>>>>>>>> +fastrpc-objs    := fastrpc_main.o \
>>>>>>>> +                fastrpc_debug.o
>>>>>>> Only build this file if debugfs is enabled.
>>>>>>>
>>>>>>> And again, "debug.c"?
>>>>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
>>>>>> few more debug specific changes, maybe then I'll need to change the build rules again.
>>>>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..cdb4fc6845a8
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>> @@ -0,0 +1,156 @@
>>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>>>>>>>> +
>>>>>>>> +#include <linux/debugfs.h>
>>>>>>>> +#include <linux/seq_file.h>
>>>>>>>> +#include "fastrpc_shared.h"
>>>>>>>> +#include "fastrpc_debug.h"
>>>>>>>> +
>>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>> Please put the #ifdef in the .h file, not in the .c file.
>>>>>> Ack
>>>>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>>>>>>>> +{
>>>>>>>> +        char cur_comm[TASK_COMM_LEN];
>>>>>>>> +        int domain_id, size;
>>>>>>>> +        char *debugfs_buf;
>>>>>>>> +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>>>>>>>> +
>>>>>>>> +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>>>>>>>> +        cur_comm[TASK_COMM_LEN-1] = '\0';
>>>>>>>> +        if (debugfs_dir != NULL) {
>>>>>>>> +                domain_id = fl->cctx->domain_id;
>>>>>>>> +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>>>>>>>> +                                current->pid, fl->tgid, domain_id) + 1;
>>>>>>>> +                debugfs_buf = kzalloc(size, GFP_KERNEL);
>>>>>>>> +                if (debugfs_buf == NULL)
>>>>>>>> +                        return;
>>>>>>>> +                /*
>>>>>>>> +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>>>>>>>> +                 * domain_id in debugfs filename to create unique file name
>>>>>>>> +                 */
>>>>>>>> +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>>>>>>>> +                        cur_comm, current->pid, fl->tgid, domain_id);
>>>>>>>> +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>>>>>>>> +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
>>>>>>> Why are you saving the debugfs file?  What do you need to do with it
>>>>>>> that you can't just delete the whole directory, or look up the name
>>>>>>> again in the future when removing it?
>>>>>> fl structure is specific to a process using fastrpc driver. The reason to save
>>>>>> this debugfs file is to delete is when the process releases fastrpc device.
>>>>>> If the file is not deleted, it might flood multiple files in debugfs directory.
>>>>>>
>>>>>> As part of this change, only the file that is getting created by a process is
>>>>>> getting removed when process is releasing device and I don't think we
>>>>>> can clean up the whole directory at this point.
>>>>> My 2c: it might be better to create a single file that conains
>>>>> information for all the processes instead of that. Or use fdinfo data to
>>>>> export process / FD information to userspace.
>>>> Thanks for your review. The reason of not having single file for all processes is that
>>>> I can run 100s of iteration for any process(say calculator) and every time the properties
>>>> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
>>>> deleting the debugfs files for every process run.
>>>>
>>>> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
>>>> the information(like in debugfs) here.
>>> Which information is actually useful / interesting for application
>>> developers? If not for the fdinfo, I might still vote for a single file
>>> rather than a pile of per-process data.
>> I have tried to capture all the information that could be useful.
>>
>> I can try changes to maintain single file for all active processes. Having this file specific
>> to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
>> carry information of all processes using that remoteproc.
> I think it's a better idea, yes.

Hi all,

I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
on this patch series.

//Ekansh

>> --ekansh
>>>> --ekansh
>

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Deepika Singh]

On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> 
> 
> On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
>> On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>>>
>>>
>>> On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
>>>> On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
>>>>> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
>>>>>>> On 11/18/2024 7:32 PM, Greg KH wrote:
>>>>>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>>>>>>>>> Add changes to support debugfs. The fastrpc directory will be
>>>>>>>>> created which will carry debugfs files for all fastrpc processes.
>>>>>>>>> The information of fastrpc user and channel contexts are getting
>>>>>>>>> captured as part of this change.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/misc/fastrpc/Makefile        |   3 +-
>>>>>>>>>   drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>>>>>>>>   drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>>>>>>>>   drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>>>>>>>>   4 files changed, 205 insertions(+), 3 deletions(-)
>>>>>>>>>   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>>>>>>>>> index 020d30789a80..4ff6b64166ae 100644
>>>>>>>>> --- a/drivers/misc/fastrpc/Makefile
>>>>>>>>> +++ b/drivers/misc/fastrpc/Makefile
>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>   # SPDX-License-Identifier: GPL-2.0
>>>>>>>>>   obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
>>>>>>>>> -fastrpc-objs    := fastrpc_main.o
>>>>>>>>> \ No newline at end of file
>>>>>>>>> +fastrpc-objs    := fastrpc_main.o \
>>>>>>>>> +                fastrpc_debug.o
>>>>>>>> Only build this file if debugfs is enabled.
>>>>>>>>
>>>>>>>> And again, "debug.c"?
>>>>>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
>>>>>>> few more debug specific changes, maybe then I'll need to change the build rules again.
>>>>>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..cdb4fc6845a8
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>> @@ -0,0 +1,156 @@
>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>>>>>>>>> +
>>>>>>>>> +#include <linux/debugfs.h>
>>>>>>>>> +#include <linux/seq_file.h>
>>>>>>>>> +#include "fastrpc_shared.h"
>>>>>>>>> +#include "fastrpc_debug.h"
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>>> Please put the #ifdef in the .h file, not in the .c file.
>>>>>>> Ack
>>>>>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>>>>>>>>> +{
>>>>>>>>> +        char cur_comm[TASK_COMM_LEN];
>>>>>>>>> +        int domain_id, size;
>>>>>>>>> +        char *debugfs_buf;
>>>>>>>>> +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>>>>>>>>> +
>>>>>>>>> +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>>>>>>>>> +        cur_comm[TASK_COMM_LEN-1] = '\0';
>>>>>>>>> +        if (debugfs_dir != NULL) {
>>>>>>>>> +                domain_id = fl->cctx->domain_id;
>>>>>>>>> +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>>>>>>>>> +                                current->pid, fl->tgid, domain_id) + 1;
>>>>>>>>> +                debugfs_buf = kzalloc(size, GFP_KERNEL);
>>>>>>>>> +                if (debugfs_buf == NULL)
>>>>>>>>> +                        return;
>>>>>>>>> +                /*
>>>>>>>>> +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>>>>>>>>> +                 * domain_id in debugfs filename to create unique file name
>>>>>>>>> +                 */
>>>>>>>>> +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>>>>>>>>> +                        cur_comm, current->pid, fl->tgid, domain_id);
>>>>>>>>> +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>>>>>>>>> +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
>>>>>>>> Why are you saving the debugfs file?  What do you need to do with it
>>>>>>>> that you can't just delete the whole directory, or look up the name
>>>>>>>> again in the future when removing it?
>>>>>>> fl structure is specific to a process using fastrpc driver. The reason to save
>>>>>>> this debugfs file is to delete is when the process releases fastrpc device.
>>>>>>> If the file is not deleted, it might flood multiple files in debugfs directory.
>>>>>>>
>>>>>>> As part of this change, only the file that is getting created by a process is
>>>>>>> getting removed when process is releasing device and I don't think we
>>>>>>> can clean up the whole directory at this point.
>>>>>> My 2c: it might be better to create a single file that conains
>>>>>> information for all the processes instead of that. Or use fdinfo data to
>>>>>> export process / FD information to userspace.
>>>>> Thanks for your review. The reason of not having single file for all processes is that
>>>>> I can run 100s of iteration for any process(say calculator) and every time the properties
>>>>> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
>>>>> deleting the debugfs files for every process run.
>>>>>
>>>>> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
>>>>> the information(like in debugfs) here.
>>>> Which information is actually useful / interesting for application
>>>> developers? If not for the fdinfo, I might still vote for a single file
>>>> rather than a pile of per-process data.
Let’s say I am trying to do debugfs read when 10+ or more sessions are 
active per channel, then for pushing data of nth process in a single 
file, I would have to wait for n-1 processes, by that time process data 
might get changed. How do you suggest handling this?
>>> I have tried to capture all the information that could be useful.
>>>
>>> I can try changes to maintain single file for all active processes. Having this file specific
>>> to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
>>> carry information of all processes using that remoteproc.
>> I think it's a better idea, yes.
> 
> Hi all,
> 
> I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
> on this patch series.
> 
> //Ekansh
> 
>>> --ekansh
>>>>> --ekansh
>>
> 

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
> 
> 
> On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> > 
> > 
> > On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> > > > 
> > > > 
> > > > On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > > > > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> > > > > > On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> > > > > > > > On 11/18/2024 7:32 PM, Greg KH wrote:
> > > > > > > > > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > Add changes to support debugfs. The fastrpc directory will be
> > > > > > > > > > created which will carry debugfs files for all fastrpc processes.
> > > > > > > > > > The information of fastrpc user and channel contexts are getting
> > > > > > > > > > captured as part of this change.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > > > > > > > > ---
> > > > > > > > > >   drivers/misc/fastrpc/Makefile        |   3 +-
> > > > > > > > > >   drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> > > > > > > > > >   drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> > > > > > > > > >   drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> > > > > > > > > >   4 files changed, 205 insertions(+), 3 deletions(-)
> > > > > > > > > >   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > >   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > index 020d30789a80..4ff6b64166ae 100644
> > > > > > > > > > --- a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > +++ b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > @@ -1,3 +1,4 @@
> > > > > > > > > >   # SPDX-License-Identifier: GPL-2.0
> > > > > > > > > >   obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> > > > > > > > > > -fastrpc-objs    := fastrpc_main.o
> > > > > > > > > > \ No newline at end of file
> > > > > > > > > > +fastrpc-objs    := fastrpc_main.o \
> > > > > > > > > > +                fastrpc_debug.o
> > > > > > > > > Only build this file if debugfs is enabled.
> > > > > > > > > 
> > > > > > > > > And again, "debug.c"?
> > > > > > > > I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> > > > > > > > few more debug specific changes, maybe then I'll need to change the build rules again.
> > > > > > > > > > diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000000..cdb4fc6845a8
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > @@ -0,0 +1,156 @@
> > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > +// Copyright (c) 2024 Qualcomm Innovation Center.
> > > > > > > > > > +
> > > > > > > > > > +#include <linux/debugfs.h>
> > > > > > > > > > +#include <linux/seq_file.h>
> > > > > > > > > > +#include "fastrpc_shared.h"
> > > > > > > > > > +#include "fastrpc_debug.h"
> > > > > > > > > > +
> > > > > > > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > > > > > Please put the #ifdef in the .h file, not in the .c file.
> > > > > > > > Ack
> > > > > > > > > > +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> > > > > > > > > > +{
> > > > > > > > > > +        char cur_comm[TASK_COMM_LEN];
> > > > > > > > > > +        int domain_id, size;
> > > > > > > > > > +        char *debugfs_buf;
> > > > > > > > > > +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> > > > > > > > > > +
> > > > > > > > > > +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> > > > > > > > > > +        cur_comm[TASK_COMM_LEN-1] = '\0';
> > > > > > > > > > +        if (debugfs_dir != NULL) {
> > > > > > > > > > +                domain_id = fl->cctx->domain_id;
> > > > > > > > > > +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> > > > > > > > > > +                                current->pid, fl->tgid, domain_id) + 1;
> > > > > > > > > > +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> > > > > > > > > > +                if (debugfs_buf == NULL)
> > > > > > > > > > +                        return;
> > > > > > > > > > +                /*
> > > > > > > > > > +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> > > > > > > > > > +                 * domain_id in debugfs filename to create unique file name
> > > > > > > > > > +                 */
> > > > > > > > > > +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> > > > > > > > > > +                        cur_comm, current->pid, fl->tgid, domain_id);
> > > > > > > > > > +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> > > > > > > > > > +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> > > > > > > > > Why are you saving the debugfs file?  What do you need to do with it
> > > > > > > > > that you can't just delete the whole directory, or look up the name
> > > > > > > > > again in the future when removing it?
> > > > > > > > fl structure is specific to a process using fastrpc driver. The reason to save
> > > > > > > > this debugfs file is to delete is when the process releases fastrpc device.
> > > > > > > > If the file is not deleted, it might flood multiple files in debugfs directory.
> > > > > > > > 
> > > > > > > > As part of this change, only the file that is getting created by a process is
> > > > > > > > getting removed when process is releasing device and I don't think we
> > > > > > > > can clean up the whole directory at this point.
> > > > > > > My 2c: it might be better to create a single file that conains
> > > > > > > information for all the processes instead of that. Or use fdinfo data to
> > > > > > > export process / FD information to userspace.
> > > > > > Thanks for your review. The reason of not having single file for all processes is that
> > > > > > I can run 100s of iteration for any process(say calculator) and every time the properties
> > > > > > of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> > > > > > deleting the debugfs files for every process run.
> > > > > > 
> > > > > > Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> > > > > > the information(like in debugfs) here.
> > > > > Which information is actually useful / interesting for application
> > > > > developers? If not for the fdinfo, I might still vote for a single file
> > > > > rather than a pile of per-process data.
> Let’s say I am trying to do debugfs read when 10+ or more sessions are
> active per channel, then for pushing data of nth process in a single file, I
> would have to wait for n-1 processes, by that time process data might get
> changed. How do you suggest handling this?

I'm yet to see the response to my question, what kind of information are
you outputting? What is actually relevant? Could you please provide an
example from the running system, so that we don't have to guess?

> > > > I have tried to capture all the information that could be useful.
> > > > 
> > > > I can try changes to maintain single file for all active processes. Having this file specific
> > > > to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
> > > > carry information of all processes using that remoteproc.
> > > I think it's a better idea, yes.
> > 
> > Hi all,
> > 
> > I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
> > on this patch series.
> > 
> > //Ekansh
> > 
> > > > --ekansh
> > > > > > --ekansh
> > > 
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Mon, Apr 14, 2025 at 10:48:49AM +0300, Dmitry Baryshkov wrote:
> On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
> > 
> > 
> > On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> > > 
> > > 
> > > On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > > > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> > > > > 
> > > > > 
> > > > > On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > > > > > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> > > > > > > On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > > > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> > > > > > > > > On 11/18/2024 7:32 PM, Greg KH wrote:
> > > > > > > > > > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > > Add changes to support debugfs. The fastrpc directory will be
> > > > > > > > > > > created which will carry debugfs files for all fastrpc processes.
> > > > > > > > > > > The information of fastrpc user and channel contexts are getting
> > > > > > > > > > > captured as part of this change.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > > > > > > > > > ---
> > > > > > > > > > >   drivers/misc/fastrpc/Makefile        |   3 +-
> > > > > > > > > > >   drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> > > > > > > > > > >   drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> > > > > > > > > > >   drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> > > > > > > > > > >   4 files changed, 205 insertions(+), 3 deletions(-)
> > > > > > > > > > >   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > >   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > index 020d30789a80..4ff6b64166ae 100644
> > > > > > > > > > > --- a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > +++ b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > @@ -1,3 +1,4 @@
> > > > > > > > > > >   # SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > >   obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> > > > > > > > > > > -fastrpc-objs    := fastrpc_main.o
> > > > > > > > > > > \ No newline at end of file
> > > > > > > > > > > +fastrpc-objs    := fastrpc_main.o \
> > > > > > > > > > > +                fastrpc_debug.o
> > > > > > > > > > Only build this file if debugfs is enabled.
> > > > > > > > > > 
> > > > > > > > > > And again, "debug.c"?
> > > > > > > > > I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> > > > > > > > > few more debug specific changes, maybe then I'll need to change the build rules again.
> > > > > > > > > > > diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 000000000000..cdb4fc6845a8
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > @@ -0,0 +1,156 @@
> > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > +// Copyright (c) 2024 Qualcomm Innovation Center.
> > > > > > > > > > > +
> > > > > > > > > > > +#include <linux/debugfs.h>
> > > > > > > > > > > +#include <linux/seq_file.h>
> > > > > > > > > > > +#include "fastrpc_shared.h"
> > > > > > > > > > > +#include "fastrpc_debug.h"
> > > > > > > > > > > +
> > > > > > > > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > > > > > > Please put the #ifdef in the .h file, not in the .c file.
> > > > > > > > > Ack
> > > > > > > > > > > +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> > > > > > > > > > > +{
> > > > > > > > > > > +        char cur_comm[TASK_COMM_LEN];
> > > > > > > > > > > +        int domain_id, size;
> > > > > > > > > > > +        char *debugfs_buf;
> > > > > > > > > > > +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> > > > > > > > > > > +
> > > > > > > > > > > +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> > > > > > > > > > > +        cur_comm[TASK_COMM_LEN-1] = '\0';
> > > > > > > > > > > +        if (debugfs_dir != NULL) {
> > > > > > > > > > > +                domain_id = fl->cctx->domain_id;
> > > > > > > > > > > +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> > > > > > > > > > > +                                current->pid, fl->tgid, domain_id) + 1;
> > > > > > > > > > > +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> > > > > > > > > > > +                if (debugfs_buf == NULL)
> > > > > > > > > > > +                        return;
> > > > > > > > > > > +                /*
> > > > > > > > > > > +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> > > > > > > > > > > +                 * domain_id in debugfs filename to create unique file name
> > > > > > > > > > > +                 */
> > > > > > > > > > > +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> > > > > > > > > > > +                        cur_comm, current->pid, fl->tgid, domain_id);
> > > > > > > > > > > +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> > > > > > > > > > > +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> > > > > > > > > > Why are you saving the debugfs file?  What do you need to do with it
> > > > > > > > > > that you can't just delete the whole directory, or look up the name
> > > > > > > > > > again in the future when removing it?
> > > > > > > > > fl structure is specific to a process using fastrpc driver. The reason to save
> > > > > > > > > this debugfs file is to delete is when the process releases fastrpc device.
> > > > > > > > > If the file is not deleted, it might flood multiple files in debugfs directory.
> > > > > > > > > 
> > > > > > > > > As part of this change, only the file that is getting created by a process is
> > > > > > > > > getting removed when process is releasing device and I don't think we
> > > > > > > > > can clean up the whole directory at this point.
> > > > > > > > My 2c: it might be better to create a single file that conains
> > > > > > > > information for all the processes instead of that. Or use fdinfo data to
> > > > > > > > export process / FD information to userspace.
> > > > > > > Thanks for your review. The reason of not having single file for all processes is that
> > > > > > > I can run 100s of iteration for any process(say calculator) and every time the properties
> > > > > > > of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> > > > > > > deleting the debugfs files for every process run.
> > > > > > > 
> > > > > > > Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> > > > > > > the information(like in debugfs) here.
> > > > > > Which information is actually useful / interesting for application
> > > > > > developers? If not for the fdinfo, I might still vote for a single file
> > > > > > rather than a pile of per-process data.
> > Let’s say I am trying to do debugfs read when 10+ or more sessions are
> > active per channel, then for pushing data of nth process in a single file, I
> > would have to wait for n-1 processes, by that time process data might get
> > changed. How do you suggest handling this?
> 
> I'm yet to see the response to my question, what kind of information are
> you outputting? What is actually relevant? Could you please provide an
> example from the running system, so that we don't have to guess?

And meanwhile could you please take a look at existing implementations?
E.g /sys/kernel/debug/dri/*/state, /sys/kernel/debug/dri/*/gem and the
fdinfo files for the process actually using drm/msm driver (you will see
memory consumption data there).

> 
> > > > > I have tried to capture all the information that could be useful.
> > > > > 
> > > > > I can try changes to maintain single file for all active processes. Having this file specific
> > > > > to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
> > > > > carry information of all processes using that remoteproc.
> > > > I think it's a better idea, yes.
> > > 
> > > Hi all,
> > > 
> > > I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
> > > on this patch series.
> > > 
> > > //Ekansh
> > > 
> > > > > --ekansh
> > > > > > > --ekansh
> > > > 
> > > 
> > 
> 
> -- 
> With best wishes
> Dmitry

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Deepika Singh]

On 4/14/2025 1:18 PM, Dmitry Baryshkov wrote:
> On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
>>
>>
>> On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
>>>
>>>
>>> On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
>>>> On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>>>>>
>>>>>
>>>>> On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
>>>>>> On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
>>>>>>> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
>>>>>>>>> On 11/18/2024 7:32 PM, Greg KH wrote:
>>>>>>>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>>>>>>>>>>> Add changes to support debugfs. The fastrpc directory will be
>>>>>>>>>>> created which will carry debugfs files for all fastrpc processes.
>>>>>>>>>>> The information of fastrpc user and channel contexts are getting
>>>>>>>>>>> captured as part of this change.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>>    drivers/misc/fastrpc/Makefile        |   3 +-
>>>>>>>>>>>    drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>>>>>>>>>>    drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>>>>>>>>>>    drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>>>>>>>>>>    4 files changed, 205 insertions(+), 3 deletions(-)
>>>>>>>>>>>    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>>>    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>>>>>>>>>>> index 020d30789a80..4ff6b64166ae 100644
>>>>>>>>>>> --- a/drivers/misc/fastrpc/Makefile
>>>>>>>>>>> +++ b/drivers/misc/fastrpc/Makefile
>>>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>>>    # SPDX-License-Identifier: GPL-2.0
>>>>>>>>>>>    obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
>>>>>>>>>>> -fastrpc-objs    := fastrpc_main.o
>>>>>>>>>>> \ No newline at end of file
>>>>>>>>>>> +fastrpc-objs    := fastrpc_main.o \
>>>>>>>>>>> +                fastrpc_debug.o
>>>>>>>>>> Only build this file if debugfs is enabled.
>>>>>>>>>>
>>>>>>>>>> And again, "debug.c"?
>>>>>>>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
>>>>>>>>> few more debug specific changes, maybe then I'll need to change the build rules again.
>>>>>>>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 000000000000..cdb4fc6845a8
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>>> @@ -0,0 +1,156 @@
>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>>>>>>>>>>> +
>>>>>>>>>>> +#include <linux/debugfs.h>
>>>>>>>>>>> +#include <linux/seq_file.h>
>>>>>>>>>>> +#include "fastrpc_shared.h"
>>>>>>>>>>> +#include "fastrpc_debug.h"
>>>>>>>>>>> +
>>>>>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>>>>> Please put the #ifdef in the .h file, not in the .c file.
>>>>>>>>> Ack
>>>>>>>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>>>>>>>>>>> +{
>>>>>>>>>>> +        char cur_comm[TASK_COMM_LEN];
>>>>>>>>>>> +        int domain_id, size;
>>>>>>>>>>> +        char *debugfs_buf;
>>>>>>>>>>> +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>>>>>>>>>>> +
>>>>>>>>>>> +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>>>>>>>>>>> +        cur_comm[TASK_COMM_LEN-1] = '\0';
>>>>>>>>>>> +        if (debugfs_dir != NULL) {
>>>>>>>>>>> +                domain_id = fl->cctx->domain_id;
>>>>>>>>>>> +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>>>>>>>>>>> +                                current->pid, fl->tgid, domain_id) + 1;
>>>>>>>>>>> +                debugfs_buf = kzalloc(size, GFP_KERNEL);
>>>>>>>>>>> +                if (debugfs_buf == NULL)
>>>>>>>>>>> +                        return;
>>>>>>>>>>> +                /*
>>>>>>>>>>> +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>>>>>>>>>>> +                 * domain_id in debugfs filename to create unique file name
>>>>>>>>>>> +                 */
>>>>>>>>>>> +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>>>>>>>>>>> +                        cur_comm, current->pid, fl->tgid, domain_id);
>>>>>>>>>>> +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>>>>>>>>>>> +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
>>>>>>>>>> Why are you saving the debugfs file?  What do you need to do with it
>>>>>>>>>> that you can't just delete the whole directory, or look up the name
>>>>>>>>>> again in the future when removing it?
>>>>>>>>> fl structure is specific to a process using fastrpc driver. The reason to save
>>>>>>>>> this debugfs file is to delete is when the process releases fastrpc device.
>>>>>>>>> If the file is not deleted, it might flood multiple files in debugfs directory.
>>>>>>>>>
>>>>>>>>> As part of this change, only the file that is getting created by a process is
>>>>>>>>> getting removed when process is releasing device and I don't think we
>>>>>>>>> can clean up the whole directory at this point.
>>>>>>>> My 2c: it might be better to create a single file that conains
>>>>>>>> information for all the processes instead of that. Or use fdinfo data to
>>>>>>>> export process / FD information to userspace.
>>>>>>> Thanks for your review. The reason of not having single file for all processes is that
>>>>>>> I can run 100s of iteration for any process(say calculator) and every time the properties
>>>>>>> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
>>>>>>> deleting the debugfs files for every process run.
>>>>>>>
>>>>>>> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
>>>>>>> the information(like in debugfs) here.
>>>>>> Which information is actually useful / interesting for application
>>>>>> developers? If not for the fdinfo, I might still vote for a single file
>>>>>> rather than a pile of per-process data.
>> Let’s say I am trying to do debugfs read when 10+ or more sessions are
>> active per channel, then for pushing data of nth process in a single file, I
>> would have to wait for n-1 processes, by that time process data might get
>> changed. How do you suggest handling this?
> 
> I'm yet to see the response to my question, what kind of information are
> you outputting? What is actually relevant? Could you please provide an
> example from the running system, so that we don't have to guess?
> 
Debugfs would include information like userspace maps, kernel allocated 
buffers, information regarding memory donated to DSP for session 
creation etc. Other relevant information can be added in debugfs on need 
basis.
>>>>> I have tried to capture all the information that could be useful.
>>>>>
>>>>> I can try changes to maintain single file for all active processes. Having this file specific
>>>>> to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
>>>>> carry information of all processes using that remoteproc.
>>>> I think it's a better idea, yes.
>>>
>>> Hi all,
>>>
>>> I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
>>> on this patch series.
>>>
>>> //Ekansh
>>>
>>>>> --ekansh
>>>>>>> --ekansh
>>>>
>>>
>>
> 

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Mon, Apr 21, 2025 at 11:41:15AM +0530, Deepika Singh wrote:
> 
> 
> On 4/14/2025 1:18 PM, Dmitry Baryshkov wrote:
> > On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
> > > 
> > > 
> > > On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> > > > 
> > > > 
> > > > On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > > > > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > > > > > > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> > > > > > > > On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > > > > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > On 11/18/2024 7:32 PM, Greg KH wrote:
> > > > > > > > > > > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > > > Add changes to support debugfs. The fastrpc directory will be
> > > > > > > > > > > > created which will carry debugfs files for all fastrpc processes.
> > > > > > > > > > > > The information of fastrpc user and channel contexts are getting
> > > > > > > > > > > > captured as part of this change.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >    drivers/misc/fastrpc/Makefile        |   3 +-
> > > > > > > > > > > >    drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> > > > > > > > > > > >    drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> > > > > > > > > > > >    drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> > > > > > > > > > > >    4 files changed, 205 insertions(+), 3 deletions(-)
> > > > > > > > > > > >    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > >    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > > index 020d30789a80..4ff6b64166ae 100644
> > > > > > > > > > > > --- a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > > +++ b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > > @@ -1,3 +1,4 @@
> > > > > > > > > > > >    # SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > >    obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> > > > > > > > > > > > -fastrpc-objs    := fastrpc_main.o
> > > > > > > > > > > > \ No newline at end of file
> > > > > > > > > > > > +fastrpc-objs    := fastrpc_main.o \
> > > > > > > > > > > > +                fastrpc_debug.o
> > > > > > > > > > > Only build this file if debugfs is enabled.
> > > > > > > > > > > 
> > > > > > > > > > > And again, "debug.c"?
> > > > > > > > > > I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> > > > > > > > > > few more debug specific changes, maybe then I'll need to change the build rules again.
> > > > > > > > > > > > diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > > new file mode 100644
> > > > > > > > > > > > index 000000000000..cdb4fc6845a8
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > > @@ -0,0 +1,156 @@
> > > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > > +// Copyright (c) 2024 Qualcomm Innovation Center.
> > > > > > > > > > > > +
> > > > > > > > > > > > +#include <linux/debugfs.h>
> > > > > > > > > > > > +#include <linux/seq_file.h>
> > > > > > > > > > > > +#include "fastrpc_shared.h"
> > > > > > > > > > > > +#include "fastrpc_debug.h"
> > > > > > > > > > > > +
> > > > > > > > > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > > > > > > > Please put the #ifdef in the .h file, not in the .c file.
> > > > > > > > > > Ack
> > > > > > > > > > > > +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +        char cur_comm[TASK_COMM_LEN];
> > > > > > > > > > > > +        int domain_id, size;
> > > > > > > > > > > > +        char *debugfs_buf;
> > > > > > > > > > > > +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> > > > > > > > > > > > +
> > > > > > > > > > > > +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> > > > > > > > > > > > +        cur_comm[TASK_COMM_LEN-1] = '\0';
> > > > > > > > > > > > +        if (debugfs_dir != NULL) {
> > > > > > > > > > > > +                domain_id = fl->cctx->domain_id;
> > > > > > > > > > > > +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> > > > > > > > > > > > +                                current->pid, fl->tgid, domain_id) + 1;
> > > > > > > > > > > > +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> > > > > > > > > > > > +                if (debugfs_buf == NULL)
> > > > > > > > > > > > +                        return;
> > > > > > > > > > > > +                /*
> > > > > > > > > > > > +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> > > > > > > > > > > > +                 * domain_id in debugfs filename to create unique file name
> > > > > > > > > > > > +                 */
> > > > > > > > > > > > +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> > > > > > > > > > > > +                        cur_comm, current->pid, fl->tgid, domain_id);
> > > > > > > > > > > > +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> > > > > > > > > > > > +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> > > > > > > > > > > Why are you saving the debugfs file?  What do you need to do with it
> > > > > > > > > > > that you can't just delete the whole directory, or look up the name
> > > > > > > > > > > again in the future when removing it?
> > > > > > > > > > fl structure is specific to a process using fastrpc driver. The reason to save
> > > > > > > > > > this debugfs file is to delete is when the process releases fastrpc device.
> > > > > > > > > > If the file is not deleted, it might flood multiple files in debugfs directory.
> > > > > > > > > > 
> > > > > > > > > > As part of this change, only the file that is getting created by a process is
> > > > > > > > > > getting removed when process is releasing device and I don't think we
> > > > > > > > > > can clean up the whole directory at this point.
> > > > > > > > > My 2c: it might be better to create a single file that conains
> > > > > > > > > information for all the processes instead of that. Or use fdinfo data to
> > > > > > > > > export process / FD information to userspace.
> > > > > > > > Thanks for your review. The reason of not having single file for all processes is that
> > > > > > > > I can run 100s of iteration for any process(say calculator) and every time the properties
> > > > > > > > of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> > > > > > > > deleting the debugfs files for every process run.
> > > > > > > > 
> > > > > > > > Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> > > > > > > > the information(like in debugfs) here.
> > > > > > > Which information is actually useful / interesting for application
> > > > > > > developers? If not for the fdinfo, I might still vote for a single file
> > > > > > > rather than a pile of per-process data.
> > > Let’s say I am trying to do debugfs read when 10+ or more sessions are
> > > active per channel, then for pushing data of nth process in a single file, I
> > > would have to wait for n-1 processes, by that time process data might get
> > > changed. How do you suggest handling this?
> > 
> > I'm yet to see the response to my question, what kind of information are
> > you outputting? What is actually relevant? Could you please provide an
> > example from the running system, so that we don't have to guess?
> > 
> Debugfs would include information like userspace maps, kernel allocated
> buffers, information regarding memory donated to DSP for session creation
> etc. Other relevant information can be added in debugfs on need basis.

Please look at debugfs/dri/0/gem, this file provides semantically more
or less the same kind of information.

Also, please be carefull with the "kernel allocated buffers". I'd be
careful to not to overexpose kernel addresses to userspace.

> > > > > > I have tried to capture all the information that could be useful.
> > > > > > 
> > > > > > I can try changes to maintain single file for all active processes. Having this file specific
> > > > > > to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
> > > > > > carry information of all processes using that remoteproc.
> > > > > I think it's a better idea, yes.
> > > > 
> > > > Hi all,
> > > > 
> > > > I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
> > > > on this patch series.
> > > > 
> > > > //Ekansh
> > > > 
> > > > > > --ekansh
> > > > > > > > --ekansh
> > > > > 
> > > > 
> > > 
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Greg KH]

On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
> 
> 
> On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> > 
> > 
> > On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> > > > 
> > > > 
> > > > On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > > > > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> > > > > > On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> > > > > > > > On 11/18/2024 7:32 PM, Greg KH wrote:
> > > > > > > > > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > Add changes to support debugfs. The fastrpc directory will be
> > > > > > > > > > created which will carry debugfs files for all fastrpc processes.
> > > > > > > > > > The information of fastrpc user and channel contexts are getting
> > > > > > > > > > captured as part of this change.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > > > > > > > > ---
> > > > > > > > > >   drivers/misc/fastrpc/Makefile        |   3 +-
> > > > > > > > > >   drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> > > > > > > > > >   drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> > > > > > > > > >   drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> > > > > > > > > >   4 files changed, 205 insertions(+), 3 deletions(-)
> > > > > > > > > >   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > >   create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > index 020d30789a80..4ff6b64166ae 100644
> > > > > > > > > > --- a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > +++ b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > @@ -1,3 +1,4 @@
> > > > > > > > > >   # SPDX-License-Identifier: GPL-2.0
> > > > > > > > > >   obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> > > > > > > > > > -fastrpc-objs    := fastrpc_main.o
> > > > > > > > > > \ No newline at end of file
> > > > > > > > > > +fastrpc-objs    := fastrpc_main.o \
> > > > > > > > > > +                fastrpc_debug.o
> > > > > > > > > Only build this file if debugfs is enabled.
> > > > > > > > > 
> > > > > > > > > And again, "debug.c"?
> > > > > > > > I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> > > > > > > > few more debug specific changes, maybe then I'll need to change the build rules again.
> > > > > > > > > > diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000000..cdb4fc6845a8
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > @@ -0,0 +1,156 @@
> > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > +// Copyright (c) 2024 Qualcomm Innovation Center.
> > > > > > > > > > +
> > > > > > > > > > +#include <linux/debugfs.h>
> > > > > > > > > > +#include <linux/seq_file.h>
> > > > > > > > > > +#include "fastrpc_shared.h"
> > > > > > > > > > +#include "fastrpc_debug.h"
> > > > > > > > > > +
> > > > > > > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > > > > > Please put the #ifdef in the .h file, not in the .c file.
> > > > > > > > Ack
> > > > > > > > > > +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> > > > > > > > > > +{
> > > > > > > > > > +        char cur_comm[TASK_COMM_LEN];
> > > > > > > > > > +        int domain_id, size;
> > > > > > > > > > +        char *debugfs_buf;
> > > > > > > > > > +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> > > > > > > > > > +
> > > > > > > > > > +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> > > > > > > > > > +        cur_comm[TASK_COMM_LEN-1] = '\0';
> > > > > > > > > > +        if (debugfs_dir != NULL) {
> > > > > > > > > > +                domain_id = fl->cctx->domain_id;
> > > > > > > > > > +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> > > > > > > > > > +                                current->pid, fl->tgid, domain_id) + 1;
> > > > > > > > > > +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> > > > > > > > > > +                if (debugfs_buf == NULL)
> > > > > > > > > > +                        return;
> > > > > > > > > > +                /*
> > > > > > > > > > +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> > > > > > > > > > +                 * domain_id in debugfs filename to create unique file name
> > > > > > > > > > +                 */
> > > > > > > > > > +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> > > > > > > > > > +                        cur_comm, current->pid, fl->tgid, domain_id);
> > > > > > > > > > +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> > > > > > > > > > +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> > > > > > > > > Why are you saving the debugfs file?  What do you need to do with it
> > > > > > > > > that you can't just delete the whole directory, or look up the name
> > > > > > > > > again in the future when removing it?
> > > > > > > > fl structure is specific to a process using fastrpc driver. The reason to save
> > > > > > > > this debugfs file is to delete is when the process releases fastrpc device.
> > > > > > > > If the file is not deleted, it might flood multiple files in debugfs directory.
> > > > > > > > 
> > > > > > > > As part of this change, only the file that is getting created by a process is
> > > > > > > > getting removed when process is releasing device and I don't think we
> > > > > > > > can clean up the whole directory at this point.
> > > > > > > My 2c: it might be better to create a single file that conains
> > > > > > > information for all the processes instead of that. Or use fdinfo data to
> > > > > > > export process / FD information to userspace.
> > > > > > Thanks for your review. The reason of not having single file for all processes is that
> > > > > > I can run 100s of iteration for any process(say calculator) and every time the properties
> > > > > > of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> > > > > > deleting the debugfs files for every process run.
> > > > > > 
> > > > > > Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> > > > > > the information(like in debugfs) here.
> > > > > Which information is actually useful / interesting for application
> > > > > developers? If not for the fdinfo, I might still vote for a single file
> > > > > rather than a pile of per-process data.
> Let’s say I am trying to do debugfs read when 10+ or more sessions are
> active per channel, then for pushing data of nth process in a single file, I
> would have to wait for n-1 processes, by that time process data might get
> changed. How do you suggest handling this?

I suggest you NEVER use debugfs for anything that you care about this
type of thing for.

debugfs is for debugging.  Don't expect to rely on it for anything
relating to performance, and many/most systems don't even have it
enabled.  It also can NOT be used for anything that actually is a real
functionality of the system, and MUST work properly if it is not enabled
or a failure happens with the creation of a debugfs file.

So why would this even be an issue, as surely you aren't expecting that
debugfs be the main api for your driver, right?  :)

thanks,

greg k-h

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Deepika Singh]

On 4/15/2025 6:47 PM, Greg KH wrote:
> On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
>>
>>
>> On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
>>>
>>>
>>> On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
>>>> On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
>>>>>
>>>>>
>>>>> On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
>>>>>> On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
>>>>>>> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
>>>>>>>>> On 11/18/2024 7:32 PM, Greg KH wrote:
>>>>>>>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
>>>>>>>>>>> Add changes to support debugfs. The fastrpc directory will be
>>>>>>>>>>> created which will carry debugfs files for all fastrpc processes.
>>>>>>>>>>> The information of fastrpc user and channel contexts are getting
>>>>>>>>>>> captured as part of this change.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>>    drivers/misc/fastrpc/Makefile        |   3 +-
>>>>>>>>>>>    drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
>>>>>>>>>>>    drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
>>>>>>>>>>>    drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
>>>>>>>>>>>    4 files changed, 205 insertions(+), 3 deletions(-)
>>>>>>>>>>>    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>>>    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
>>>>>>>>>>> index 020d30789a80..4ff6b64166ae 100644
>>>>>>>>>>> --- a/drivers/misc/fastrpc/Makefile
>>>>>>>>>>> +++ b/drivers/misc/fastrpc/Makefile
>>>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>>>    # SPDX-License-Identifier: GPL-2.0
>>>>>>>>>>>    obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
>>>>>>>>>>> -fastrpc-objs    := fastrpc_main.o
>>>>>>>>>>> \ No newline at end of file
>>>>>>>>>>> +fastrpc-objs    := fastrpc_main.o \
>>>>>>>>>>> +                fastrpc_debug.o
>>>>>>>>>> Only build this file if debugfs is enabled.
>>>>>>>>>>
>>>>>>>>>> And again, "debug.c"?
>>>>>>>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
>>>>>>>>> few more debug specific changes, maybe then I'll need to change the build rules again.
>>>>>>>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 000000000000..cdb4fc6845a8
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
>>>>>>>>>>> @@ -0,0 +1,156 @@
>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
>>>>>>>>>>> +
>>>>>>>>>>> +#include <linux/debugfs.h>
>>>>>>>>>>> +#include <linux/seq_file.h>
>>>>>>>>>>> +#include "fastrpc_shared.h"
>>>>>>>>>>> +#include "fastrpc_debug.h"
>>>>>>>>>>> +
>>>>>>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>>>>> Please put the #ifdef in the .h file, not in the .c file.
>>>>>>>>> Ack
>>>>>>>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
>>>>>>>>>>> +{
>>>>>>>>>>> +        char cur_comm[TASK_COMM_LEN];
>>>>>>>>>>> +        int domain_id, size;
>>>>>>>>>>> +        char *debugfs_buf;
>>>>>>>>>>> +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
>>>>>>>>>>> +
>>>>>>>>>>> +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
>>>>>>>>>>> +        cur_comm[TASK_COMM_LEN-1] = '\0';
>>>>>>>>>>> +        if (debugfs_dir != NULL) {
>>>>>>>>>>> +                domain_id = fl->cctx->domain_id;
>>>>>>>>>>> +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
>>>>>>>>>>> +                                current->pid, fl->tgid, domain_id) + 1;
>>>>>>>>>>> +                debugfs_buf = kzalloc(size, GFP_KERNEL);
>>>>>>>>>>> +                if (debugfs_buf == NULL)
>>>>>>>>>>> +                        return;
>>>>>>>>>>> +                /*
>>>>>>>>>>> +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
>>>>>>>>>>> +                 * domain_id in debugfs filename to create unique file name
>>>>>>>>>>> +                 */
>>>>>>>>>>> +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
>>>>>>>>>>> +                        cur_comm, current->pid, fl->tgid, domain_id);
>>>>>>>>>>> +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
>>>>>>>>>>> +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
>>>>>>>>>> Why are you saving the debugfs file?  What do you need to do with it
>>>>>>>>>> that you can't just delete the whole directory, or look up the name
>>>>>>>>>> again in the future when removing it?
>>>>>>>>> fl structure is specific to a process using fastrpc driver. The reason to save
>>>>>>>>> this debugfs file is to delete is when the process releases fastrpc device.
>>>>>>>>> If the file is not deleted, it might flood multiple files in debugfs directory.
>>>>>>>>>
>>>>>>>>> As part of this change, only the file that is getting created by a process is
>>>>>>>>> getting removed when process is releasing device and I don't think we
>>>>>>>>> can clean up the whole directory at this point.
>>>>>>>> My 2c: it might be better to create a single file that conains
>>>>>>>> information for all the processes instead of that. Or use fdinfo data to
>>>>>>>> export process / FD information to userspace.
>>>>>>> Thanks for your review. The reason of not having single file for all processes is that
>>>>>>> I can run 100s of iteration for any process(say calculator) and every time the properties
>>>>>>> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
>>>>>>> deleting the debugfs files for every process run.
>>>>>>>
>>>>>>> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
>>>>>>> the information(like in debugfs) here.
>>>>>> Which information is actually useful / interesting for application
>>>>>> developers? If not for the fdinfo, I might still vote for a single file
>>>>>> rather than a pile of per-process data.
>> Let’s say I am trying to do debugfs read when 10+ or more sessions are
>> active per channel, then for pushing data of nth process in a single file, I
>> would have to wait for n-1 processes, by that time process data might get
>> changed. How do you suggest handling this?
> 
> I suggest you NEVER use debugfs for anything that you care about this
> type of thing for.
> 
> debugfs is for debugging.  Don't expect to rely on it for anything
> relating to performance, and many/most systems don't even have it
> enabled.  It also can NOT be used for anything that actually is a real
> functionality of the system, and MUST work properly if it is not enabled
> or a failure happens with the creation of a debugfs file.
> 
> So why would this even be an issue, as surely you aren't expecting that
> debugfs be the main api for your driver, right?  :)
> 
> thanks,
> 
> greg k-h
I am not going to rely on debugfs for anything related to performance or 
real functionality. It would be used for debugging alone. Concern here 
is if i push all processes data to one file, then data might get updated 
by the time nth process's data is pushed and I might not get get correct 
data for nth process.

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Greg KH]

On Mon, Apr 21, 2025 at 01:51:09PM +0530, Deepika Singh wrote:
> 
> 
> On 4/15/2025 6:47 PM, Greg KH wrote:
> > On Mon, Apr 14, 2025 at 12:41:47PM +0530, Deepika Singh wrote:
> > > 
> > > 
> > > On 4/11/2025 1:55 PM, Ekansh Gupta wrote:
> > > > 
> > > > 
> > > > On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > > > > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> > > > > > > On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> > > > > > > > On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > > > > > > > > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > On 11/18/2024 7:32 PM, Greg KH wrote:
> > > > > > > > > > > On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> > > > > > > > > > > > Add changes to support debugfs. The fastrpc directory will be
> > > > > > > > > > > > created which will carry debugfs files for all fastrpc processes.
> > > > > > > > > > > > The information of fastrpc user and channel contexts are getting
> > > > > > > > > > > > captured as part of this change.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >    drivers/misc/fastrpc/Makefile        |   3 +-
> > > > > > > > > > > >    drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> > > > > > > > > > > >    drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> > > > > > > > > > > >    drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> > > > > > > > > > > >    4 files changed, 205 insertions(+), 3 deletions(-)
> > > > > > > > > > > >    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > >    create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > > index 020d30789a80..4ff6b64166ae 100644
> > > > > > > > > > > > --- a/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > > +++ b/drivers/misc/fastrpc/Makefile
> > > > > > > > > > > > @@ -1,3 +1,4 @@
> > > > > > > > > > > >    # SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > >    obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> > > > > > > > > > > > -fastrpc-objs    := fastrpc_main.o
> > > > > > > > > > > > \ No newline at end of file
> > > > > > > > > > > > +fastrpc-objs    := fastrpc_main.o \
> > > > > > > > > > > > +                fastrpc_debug.o
> > > > > > > > > > > Only build this file if debugfs is enabled.
> > > > > > > > > > > 
> > > > > > > > > > > And again, "debug.c"?
> > > > > > > > > > I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> > > > > > > > > > few more debug specific changes, maybe then I'll need to change the build rules again.
> > > > > > > > > > > > diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > > new file mode 100644
> > > > > > > > > > > > index 000000000000..cdb4fc6845a8
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> > > > > > > > > > > > @@ -0,0 +1,156 @@
> > > > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > > > +// Copyright (c) 2024 Qualcomm Innovation Center.
> > > > > > > > > > > > +
> > > > > > > > > > > > +#include <linux/debugfs.h>
> > > > > > > > > > > > +#include <linux/seq_file.h>
> > > > > > > > > > > > +#include "fastrpc_shared.h"
> > > > > > > > > > > > +#include "fastrpc_debug.h"
> > > > > > > > > > > > +
> > > > > > > > > > > > +#ifdef CONFIG_DEBUG_FS
> > > > > > > > > > > Please put the #ifdef in the .h file, not in the .c file.
> > > > > > > > > > Ack
> > > > > > > > > > > > +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +        char cur_comm[TASK_COMM_LEN];
> > > > > > > > > > > > +        int domain_id, size;
> > > > > > > > > > > > +        char *debugfs_buf;
> > > > > > > > > > > > +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> > > > > > > > > > > > +
> > > > > > > > > > > > +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> > > > > > > > > > > > +        cur_comm[TASK_COMM_LEN-1] = '\0';
> > > > > > > > > > > > +        if (debugfs_dir != NULL) {
> > > > > > > > > > > > +                domain_id = fl->cctx->domain_id;
> > > > > > > > > > > > +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> > > > > > > > > > > > +                                current->pid, fl->tgid, domain_id) + 1;
> > > > > > > > > > > > +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> > > > > > > > > > > > +                if (debugfs_buf == NULL)
> > > > > > > > > > > > +                        return;
> > > > > > > > > > > > +                /*
> > > > > > > > > > > > +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> > > > > > > > > > > > +                 * domain_id in debugfs filename to create unique file name
> > > > > > > > > > > > +                 */
> > > > > > > > > > > > +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> > > > > > > > > > > > +                        cur_comm, current->pid, fl->tgid, domain_id);
> > > > > > > > > > > > +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> > > > > > > > > > > > +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> > > > > > > > > > > Why are you saving the debugfs file?  What do you need to do with it
> > > > > > > > > > > that you can't just delete the whole directory, or look up the name
> > > > > > > > > > > again in the future when removing it?
> > > > > > > > > > fl structure is specific to a process using fastrpc driver. The reason to save
> > > > > > > > > > this debugfs file is to delete is when the process releases fastrpc device.
> > > > > > > > > > If the file is not deleted, it might flood multiple files in debugfs directory.
> > > > > > > > > > 
> > > > > > > > > > As part of this change, only the file that is getting created by a process is
> > > > > > > > > > getting removed when process is releasing device and I don't think we
> > > > > > > > > > can clean up the whole directory at this point.
> > > > > > > > > My 2c: it might be better to create a single file that conains
> > > > > > > > > information for all the processes instead of that. Or use fdinfo data to
> > > > > > > > > export process / FD information to userspace.
> > > > > > > > Thanks for your review. The reason of not having single file for all processes is that
> > > > > > > > I can run 100s of iteration for any process(say calculator) and every time the properties
> > > > > > > > of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> > > > > > > > deleting the debugfs files for every process run.
> > > > > > > > 
> > > > > > > > Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> > > > > > > > the information(like in debugfs) here.
> > > > > > > Which information is actually useful / interesting for application
> > > > > > > developers? If not for the fdinfo, I might still vote for a single file
> > > > > > > rather than a pile of per-process data.
> > > Let’s say I am trying to do debugfs read when 10+ or more sessions are
> > > active per channel, then for pushing data of nth process in a single file, I
> > > would have to wait for n-1 processes, by that time process data might get
> > > changed. How do you suggest handling this?
> > 
> > I suggest you NEVER use debugfs for anything that you care about this
> > type of thing for.
> > 
> > debugfs is for debugging.  Don't expect to rely on it for anything
> > relating to performance, and many/most systems don't even have it
> > enabled.  It also can NOT be used for anything that actually is a real
> > functionality of the system, and MUST work properly if it is not enabled
> > or a failure happens with the creation of a debugfs file.
> > 
> > So why would this even be an issue, as surely you aren't expecting that
> > debugfs be the main api for your driver, right?  :)
> > 
> > thanks,
> > 
> > greg k-h
> I am not going to rely on debugfs for anything related to performance or
> real functionality. It would be used for debugging alone. Concern here is if
> i push all processes data to one file, then data might get updated by the
> time nth process's data is pushed and I might not get get correct data for
> nth process.

But why would you care?  This is just debugging code, you should NEVER
make any action based on the output of a debugfs file.  This is all just
for your convience in working on the kernel code only.  If you have any
tool that relies on it, or requires performance issues with it, that is
flat out wrong.

Remember, the code can, and MUST, work just fine if debugfs is not
enabled in the kernel at all as that is how the majority of Linux
systems in the world are configured (for good reason).

thanks,

greg k-h

Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc

[Dmitry Baryshkov]

On Fri, Apr 11, 2025 at 01:55:12PM +0530, Ekansh Gupta wrote:
> 
> 
> On 12/3/2024 5:27 PM, Dmitry Baryshkov wrote:
> > On Tue, 3 Dec 2024 at 07:22, Ekansh Gupta <quic_ekangupt@quicinc.com> wrote:
> >>
> >>
> >> On 12/2/2024 6:18 PM, Dmitry Baryshkov wrote:
> >>> On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
> >>>> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> >>>>> On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> >>>>>> On 11/18/2024 7:32 PM, Greg KH wrote:
> >>>>>>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> >>>>>>>> Add changes to support debugfs. The fastrpc directory will be
> >>>>>>>> created which will carry debugfs files for all fastrpc processes.
> >>>>>>>> The information of fastrpc user and channel contexts are getting
> >>>>>>>> captured as part of this change.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/misc/fastrpc/Makefile        |   3 +-
> >>>>>>>>  drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> >>>>>>>>  drivers/misc/fastrpc/fastrpc_debug.h |  31 ++++++
> >>>>>>>>  drivers/misc/fastrpc/fastrpc_main.c  |  18 +++-
> >>>>>>>>  4 files changed, 205 insertions(+), 3 deletions(-)
> >>>>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> >>>>>>>>  create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> >>>>>>>> index 020d30789a80..4ff6b64166ae 100644
> >>>>>>>> --- a/drivers/misc/fastrpc/Makefile
> >>>>>>>> +++ b/drivers/misc/fastrpc/Makefile
> >>>>>>>> @@ -1,3 +1,4 @@
> >>>>>>>>  # SPDX-License-Identifier: GPL-2.0
> >>>>>>>>  obj-$(CONFIG_QCOM_FASTRPC)      += fastrpc.o
> >>>>>>>> -fastrpc-objs    := fastrpc_main.o
> >>>>>>>> \ No newline at end of file
> >>>>>>>> +fastrpc-objs    := fastrpc_main.o \
> >>>>>>>> +                fastrpc_debug.o
> >>>>>>> Only build this file if debugfs is enabled.
> >>>>>>>
> >>>>>>> And again, "debug.c"?
> >>>>>> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> >>>>>> few more debug specific changes, maybe then I'll need to change the build rules again.
> >>>>>>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>>>>>> new file mode 100644
> >>>>>>>> index 000000000000..cdb4fc6845a8
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>>>>>> @@ -0,0 +1,156 @@
> >>>>>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>>>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
> >>>>>>>> +
> >>>>>>>> +#include <linux/debugfs.h>
> >>>>>>>> +#include <linux/seq_file.h>
> >>>>>>>> +#include "fastrpc_shared.h"
> >>>>>>>> +#include "fastrpc_debug.h"
> >>>>>>>> +
> >>>>>>>> +#ifdef CONFIG_DEBUG_FS
> >>>>>>> Please put the #ifdef in the .h file, not in the .c file.
> >>>>>> Ack
> >>>>>>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> >>>>>>>> +{
> >>>>>>>> +        char cur_comm[TASK_COMM_LEN];
> >>>>>>>> +        int domain_id, size;
> >>>>>>>> +        char *debugfs_buf;
> >>>>>>>> +        struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> >>>>>>>> +
> >>>>>>>> +        memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> >>>>>>>> +        cur_comm[TASK_COMM_LEN-1] = '\0';
> >>>>>>>> +        if (debugfs_dir != NULL) {
> >>>>>>>> +                domain_id = fl->cctx->domain_id;
> >>>>>>>> +                size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> >>>>>>>> +                                current->pid, fl->tgid, domain_id) + 1;
> >>>>>>>> +                debugfs_buf = kzalloc(size, GFP_KERNEL);
> >>>>>>>> +                if (debugfs_buf == NULL)
> >>>>>>>> +                        return;
> >>>>>>>> +                /*
> >>>>>>>> +                 * Use HLOS process name, HLOS PID, fastrpc user TGID,
> >>>>>>>> +                 * domain_id in debugfs filename to create unique file name
> >>>>>>>> +                 */
> >>>>>>>> +                snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> >>>>>>>> +                        cur_comm, current->pid, fl->tgid, domain_id);
> >>>>>>>> +                fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> >>>>>>>> +                                debugfs_dir, fl, &fastrpc_debugfs_fops);
> >>>>>>> Why are you saving the debugfs file?  What do you need to do with it
> >>>>>>> that you can't just delete the whole directory, or look up the name
> >>>>>>> again in the future when removing it?
> >>>>>> fl structure is specific to a process using fastrpc driver. The reason to save
> >>>>>> this debugfs file is to delete is when the process releases fastrpc device.
> >>>>>> If the file is not deleted, it might flood multiple files in debugfs directory.
> >>>>>>
> >>>>>> As part of this change, only the file that is getting created by a process is
> >>>>>> getting removed when process is releasing device and I don't think we
> >>>>>> can clean up the whole directory at this point.
> >>>>> My 2c: it might be better to create a single file that conains
> >>>>> information for all the processes instead of that. Or use fdinfo data to
> >>>>> export process / FD information to userspace.
> >>>> Thanks for your review. The reason of not having single file for all processes is that
> >>>> I can run 100s of iteration for any process(say calculator) and every time the properties
> >>>> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> >>>> deleting the debugfs files for every process run.
> >>>>
> >>>> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> >>>> the information(like in debugfs) here.
> >>> Which information is actually useful / interesting for application
> >>> developers? If not for the fdinfo, I might still vote for a single file
> >>> rather than a pile of per-process data.
> >> I have tried to capture all the information that could be useful.
> >>
> >> I can try changes to maintain single file for all active processes. Having this file specific
> >> to a channel should be fine, right? like fastrpc_adsp, fastrpc_cdsp, etc.? Each file will
> >> carry information of all processes using that remoteproc.
> > I think it's a better idea, yes.
> 
> Hi all,
> 
> I'm adding Deepika <quic_dsi@quicinc.com> to this thread who is reworking
> on this patch series.

Please don't do this in future. Corresponding engineer can download the
mbox of the thread from lore.kernel.org, import it into the email client
and respond directly, without extra emails.

-- 
With best wishes
Dmitry