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

[Deepika Singh <quic_dsi@quicinc.com>]

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