Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4f496374-2d35-5d92-b7ec-cad256deb5a2@quicinc.com>
Date:   Mon, 17 Apr 2023 14:20:45 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH V1 2/3] drivers: misc: dcc: Add driver support for Data
 Capture and Compare unit(DCC)
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Rob Herring <robh+dt@kernel.org>, Alex Elder <elder@ieee.org>,
        Arnd Bergmann <arnd@arndb.de>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, Sibi Sankar <quic_sibis@quicinc.com>,
        "Rajendra Nayak" <quic_rjendra@quicinc.com>
References: <cover.1681480351.git.quic_schowdhu@quicinc.com>
 <b1a9cbbcfefe133cc9047a71a2acdaa74239df29.1681480351.git.quic_schowdhu@quicinc.com>
 <ZDo4jIIV7cfPD2qW@kroah.com>
 <f3196d7a-50f0-9bfb-71a6-47ddb9686039@quicinc.com>
 <ZDzkkNQP5eO2vcxA@kroah.com>
 <7c46fe45-70b4-1f20-5ab4-cd51917d04a8@quicinc.com>
 <ZDz4RcQ1sFpynxop@kroah.com>
From:   Souradeep Chowdhury <quic_schowdhu@quicinc.com>
In-Reply-To: <ZDz4RcQ1sFpynxop@kroah.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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