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