Messages in this thread | | | Date | Mon, 17 Apr 2023 14:20:45 +0530 | Subject | Re: [PATCH V1 2/3] drivers: misc: dcc: Add driver support for Data Capture and Compare unit(DCC) | From | Souradeep Chowdhury <> |
| |
On 4/17/2023 1:11 PM, Greg Kroah-Hartman wrote: > On Mon, Apr 17, 2023 at 12:26:23PM +0530, Souradeep Chowdhury wrote: >> On 4/17/2023 11:47 AM, Greg Kroah-Hartman wrote: >>> On Mon, Apr 17, 2023 at 11:31:46AM +0530, Souradeep Chowdhury wrote: >>>> On 4/15/2023 11:09 AM, Greg Kroah-Hartman wrote: >>>>>> +static void dcc_create_debug_dir(struct dcc_drvdata *drvdata) >>>>>> +{ >>>>>> + int i; >>>>>> + char list_num[10]; >>>>>> + struct dentry *list; >>>>>> + struct device *dev = drvdata->dev; >>>>>> + >>>>>> + drvdata->dbg_dir = debugfs_create_dir(dev_name(dev), NULL); >>>>> >>>>> You are creating a directory at the root of debugfs with just your >>>>> device name? While that will work, that feels very odd. Please use a >>>>> subdirectory. >>>> >>>> This is as per the comment on v17 of the patch series on DCC >>>> >>>> https://lore.kernel.org/linux-arm-kernel/6693993c-bd81-c974-a903-52a62bfec606@ieee.org/ >>>> >>>> "You never remove this dcc_dbg directory. Why not? >>>> >>>> And since you don't, dcc_dbg could just be a local >>>> variable here rather than being a global. But it >>>> seems to me this is the root directory you want to >>>> remove when you're done." >>> >>> But that's not the issue. The issue is that you are putting into >>> /sys/kernel/debug/ a flat namespace of all of your devices. Is that >>> really what you want? If so, why do you think this will never conflict >>> with any other device in the system? >> >> Since we are going by the dev_name here which also contains the address >> as per the example in the yaml binding, this will not conflict with other >> dev_names in the system. > > That is not true at all. dev_names are only unique per bus type. There > is nothing preventing any other bus from using the same name for their > device as yours. > > So please, for the sake of your own sanity, just create a directory and > dump all of your devices into it. And for the sake of all of ours, as > making the root of debugfs full of random device names is a mess, don't > you think? What would happen if all drivers did that?
Ack
> >>>> As per upstream discussions it was decided that debugfs will be a suitable >>>> interface for DCC. More on this in the following link:- >>>> >>>> https://lore.kernel.org/linux-arm-kernel/20221228172825.r32vpphbdulaldvv@builder.lan/ >>> >>> debugfs is not a suitable interface for anything that is "real" as >>> that's not what it is there for. Most systems disable debugfs entirely >>> (i.e. Android), or prevent any normal user from accessing it, so this >>> api you are creating will not be able to be used by anyone. >>> >>> debugfs is for debugging, not for anything that the system functionality >>> relies on to work properly. >> >> DCC is a debugging hardware which stores the values of the configured >> register address on a system crash on it's dedicated sram. These register >> addresses are configured by the user through the debugfs interface. Also on >> the platforms where debugfs is disabled there is an alternative of using >> bootconfig suggested to configure the register values during boot-time. >> There is an ongoing patch series for that as follows:- >> >> https://lore.kernel.org/linux-arm-kernel/cover.1675054375.git.quic_schowdhu@quicinc.com/T/ > > But again, debugfs is not even mounted in most systems, so how are they > going to interact with your hardware? Restricting it to debugfs feels > very odd to me, but hey, it's your code, I guess you don't want anyone > to use it :) > > good luck! > > greg k-h
| |