Message-ID: <20e60deb-f1f3-4f96-a454-9cb9d8cc7c1d@quicinc.com>
Date: Mon, 21 Apr 2025 13:51:09 +0530
From: Deepika Singh <quic_dsi@...cinc.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: Ekansh Gupta <ekansh.gupta@....qualcomm.com>,
        Dmitry Baryshkov
	<dmitry.baryshkov@...aro.org>,
        <dmitry.baryshkov@....qualcomm.com>, <srinivas.kandagatla@...aro.org>,
        <linux-arm-msm@...r.kernel.org>, <quic_bkumar@...cinc.com>,
        <linux-kernel@...r.kernel.org>, <quic_chennak@...cinc.com>,
        <dri-devel@...ts.freedesktop.org>, <arnd@...db.de>
Subject: Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc



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@...cinc.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@...cinc.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.

Powered by blists - more mailing lists