| Message ID | 20241118084046.3201290-5-quic_ekangupt@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
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
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.
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 > >
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 > > > > >
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 > >>> >
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);
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