Re: [PATCH V1 2/3] drivers: misc: dcc: Add driver support for Data Capture and Compare unit(DCC)

Souradeep Chowdhury
2023-04-17T08:51:44Z

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