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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4f496374-2d35-5d92-b7ec-cad256deb5a2@quicinc.com>
Date:   Mon, 17 Apr 2023 14:20:45 +0530
From:   Souradeep Chowdhury <quic_schowdhu@...cinc.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, Alex Elder <elder@...e.org>,
        Arnd Bergmann <arnd@...db.de>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, Sibi Sankar <quic_sibis@...cinc.com>,
        "Rajendra Nayak" <quic_rjendra@...cinc.com>
Subject: Re: [PATCH V1 2/3] drivers: misc: dcc: Add driver support for Data
 Capture and Compare unit(DCC)



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