mbox series

[V1,0/3] Add Data Capture and Compare(DCC) driver to new location

Message ID cover.1681480351.git.quic_schowdhu@quicinc.com
Headers show
Series Add Data Capture and Compare(DCC) driver to new location | expand

Message

Souradeep Chowdhury April 14, 2023, 1:59 p.m. UTC
DCC(Data Capture and Compare) is a DMA engine designed for debugging purposes.
In case of a system crash or manual software triggers by the user the DCC hardware
stores the value at the register addresses which can be used for debugging purposes.
The DCC driver provides the user with debugfs interface to configure the register
addresses. The options that the DCC hardware provides include reading from registers,
writing to registers, first reading and then writing to registers and looping
through the values of the same register.

This patch series is a continuation of the previous series

https://lore.kernel.org/linux-arm-kernel/20221228172825.r32vpphbdulaldvv@builder.lan/T/

The dcc driver is moved to a new location drivers/misc along with the binding
as per discussions.

Souradeep Chowdhury (3):
  dt-bindings: misc: qcom,dcc: Add the dtschema
  drivers: misc: dcc: Add driver support for Data Capture and Compare
    unit(DCC)
  MAINTAINERS: Add the entry for DCC(Data Capture and Compare) driver
    support

 .../devicetree/bindings/misc/qcom,dcc.yaml         |   44 +
 MAINTAINERS                                        |    8 +
 drivers/misc/Kconfig                               |    8 +
 drivers/misc/Makefile                              |    1 +
 drivers/misc/dcc.c                                 | 1305 ++++++++++++++++++++
 5 files changed, 1366 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/qcom,dcc.yaml
 create mode 100644 drivers/misc/dcc.c

--
2.7.4

Comments

Greg Kroah-Hartman April 15, 2023, 5:39 a.m. UTC | #1
On Fri, Apr 14, 2023 at 07:29:12PM +0530, Souradeep Chowdhury wrote:
> The DCC is a DMA Engine designed to capture and store data
> during system crash or software triggers. The DCC operates
> based on user inputs via the debugfs interface. The user gives
> addresses as inputs and these addresses are stored in the
> dcc sram. In case of a system crash or a manual software
> trigger by the user through the debugfs interface,
> the dcc captures and stores the values at these addresses.
> This patch contains the driver which has all the methods
> pertaining to the debugfs interface, auxiliary functions to
> support all the four fundamental operations of dcc namely
> read, write, read/modify/write and loop. The probe method
> here instantiates all the resources necessary for dcc to
> operate mainly the dedicated dcc sram where it stores the
> values. The DCC driver can be used for debugging purposes
> without going for a reboot since it can perform software
> triggers as well based on user inputs.

You have 72 columns, why not use them all please?

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.

It is now 2023 :)




> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#define STATUS_READY_TIMEOUT		5000  /* microseconds */
> +
> +#define DCC_SRAM_NODE "dcc_sram"

You only use this once, why is a #define needed?

> +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.

> +	if (IS_ERR(drvdata->dbg_dir)) {
> +		pr_err("can't create debugfs dir\n");

There is no need to ever check the return value of a debugfs call.

Nor do you really ever even need to save off the dentry here, just look
it up when you need to remove it.

> +		return;
> +	}
> +
> +	for (i = 0; i <= drvdata->nr_link_list; i++) {
> +		sprintf(list_num, "%d", i);
> +		list = debugfs_create_dir(list_num, drvdata->dbg_dir);
> +		debugfs_create_file("enable", 0600, list, drvdata, &enable_fops);
> +		debugfs_create_file("config", 0600, list, drvdata, &config_fops);
> +	}
> +
> +	debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops);
> +	debugfs_create_file("ready", 0400, drvdata->dbg_dir, drvdata, &ready_fops);
> +	debugfs_create_file("config_reset", 0200, drvdata->dbg_dir,
> +			    drvdata, &config_reset_fops);

This really looks like you are using debugfs to control the device, not
just for debugging information.  How are you going to be able to use the
device in a system that has debugfs disabled?

thanks,

greg k-h
Souradeep Chowdhury April 17, 2023, 6:01 a.m. UTC | #2
On 4/15/2023 11:09 AM, Greg Kroah-Hartman wrote:
> On Fri, Apr 14, 2023 at 07:29:12PM +0530, Souradeep Chowdhury wrote:
>> The DCC is a DMA Engine designed to capture and store data
>> during system crash or software triggers. The DCC operates
>> based on user inputs via the debugfs interface. The user gives
>> addresses as inputs and these addresses are stored in the
>> dcc sram. In case of a system crash or a manual software
>> trigger by the user through the debugfs interface,
>> the dcc captures and stores the values at these addresses.
>> This patch contains the driver which has all the methods
>> pertaining to the debugfs interface, auxiliary functions to
>> support all the four fundamental operations of dcc namely
>> read, write, read/modify/write and loop. The probe method
>> here instantiates all the resources necessary for dcc to
>> operate mainly the dedicated dcc sram where it stores the
>> values. The DCC driver can be used for debugging purposes
>> without going for a reboot since it can perform software
>> triggers as well based on user inputs.
> 
> You have 72 columns, why not use them all please?
> 
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> 
> It is now 2023 :)

Ack

> 
> 
> 
> 
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/delay.h>
>> +#include <linux/fs.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +
>> +#define STATUS_READY_TIMEOUT		5000  /* microseconds */
>> +
>> +#define DCC_SRAM_NODE "dcc_sram"
> 
> You only use this once, why is a #define needed?

This is as per the comment on version 1 of the patch series on DCC

https://lore.kernel.org/linux-arm-kernel/YElUCaBUOx7hEuIh@builder.lan/

"kzalloc + strlcpy can be replaced by kstrdup(), but that said...all 
this does seems to be to copy a const string to the heap and lugging it
around. Use a define instead."


> 
>> +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."


> 
>> +	if (IS_ERR(drvdata->dbg_dir)) {
>> +		pr_err("can't create debugfs dir\n");
> 
> There is no need to ever check the return value of a debugfs call.
> 
> Nor do you really ever even need to save off the dentry here, just look
> it up when you need to remove it.

Ack

> 
>> +		return;
>> +	}
>> +
>> +	for (i = 0; i <= drvdata->nr_link_list; i++) {
>> +		sprintf(list_num, "%d", i);
>> +		list = debugfs_create_dir(list_num, drvdata->dbg_dir);
>> +		debugfs_create_file("enable", 0600, list, drvdata, &enable_fops);
>> +		debugfs_create_file("config", 0600, list, drvdata, &config_fops);
>> +	}
>> +
>> +	debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops);
>> +	debugfs_create_file("ready", 0400, drvdata->dbg_dir, drvdata, &ready_fops);
>> +	debugfs_create_file("config_reset", 0200, drvdata->dbg_dir,
>> +			    drvdata, &config_reset_fops);
> 
> This really looks like you are using debugfs to control the device, not
> just for debugging information.  How are you going to be able to use the
> device in a system that has debugfs disabled?

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/

> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman April 17, 2023, 6:17 a.m. UTC | #3
On Mon, Apr 17, 2023 at 11:31:46AM +0530, Souradeep Chowdhury wrote:
> 
> 
> On 4/15/2023 11:09 AM, Greg Kroah-Hartman wrote:
> > On Fri, Apr 14, 2023 at 07:29:12PM +0530, Souradeep Chowdhury wrote:
> > > The DCC is a DMA Engine designed to capture and store data
> > > during system crash or software triggers. The DCC operates
> > > based on user inputs via the debugfs interface. The user gives
> > > addresses as inputs and these addresses are stored in the
> > > dcc sram. In case of a system crash or a manual software
> > > trigger by the user through the debugfs interface,
> > > the dcc captures and stores the values at these addresses.
> > > This patch contains the driver which has all the methods
> > > pertaining to the debugfs interface, auxiliary functions to
> > > support all the four fundamental operations of dcc namely
> > > read, write, read/modify/write and loop. The probe method
> > > here instantiates all the resources necessary for dcc to
> > > operate mainly the dedicated dcc sram where it stores the
> > > values. The DCC driver can be used for debugging purposes
> > > without going for a reboot since it can perform software
> > > triggers as well based on user inputs.
> > 
> > You have 72 columns, why not use them all please?
> > 
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> > > + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> > 
> > It is now 2023 :)
> 
> Ack
> 
> > 
> > 
> > 
> > 
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/debugfs.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/fs.h>
> > > +#include <linux/io.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/miscdevice.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/uaccess.h>
> > > +
> > > +#define STATUS_READY_TIMEOUT		5000  /* microseconds */
> > > +
> > > +#define DCC_SRAM_NODE "dcc_sram"
> > 
> > You only use this once, why is a #define needed?
> 
> This is as per the comment on version 1 of the patch series on DCC
> 
> https://lore.kernel.org/linux-arm-kernel/YElUCaBUOx7hEuIh@builder.lan/
> 
> "kzalloc + strlcpy can be replaced by kstrdup(), but that said...all this
> does seems to be to copy a const string to the heap and lugging it
> around. Use a define instead."

But as you are not doing any of that here, just using the string in the
one place it is needed is the same thing :)

> > > +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?

> > > +	if (IS_ERR(drvdata->dbg_dir)) {
> > > +		pr_err("can't create debugfs dir\n");
> > 
> > There is no need to ever check the return value of a debugfs call.
> > 
> > Nor do you really ever even need to save off the dentry here, just look
> > it up when you need to remove it.
> 
> Ack
> 
> > 
> > > +		return;
> > > +	}
> > > +
> > > +	for (i = 0; i <= drvdata->nr_link_list; i++) {
> > > +		sprintf(list_num, "%d", i);
> > > +		list = debugfs_create_dir(list_num, drvdata->dbg_dir);
> > > +		debugfs_create_file("enable", 0600, list, drvdata, &enable_fops);
> > > +		debugfs_create_file("config", 0600, list, drvdata, &config_fops);
> > > +	}
> > > +
> > > +	debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops);
> > > +	debugfs_create_file("ready", 0400, drvdata->dbg_dir, drvdata, &ready_fops);
> > > +	debugfs_create_file("config_reset", 0200, drvdata->dbg_dir,
> > > +			    drvdata, &config_reset_fops);
> > 
> > This really looks like you are using debugfs to control the device, not
> > just for debugging information.  How are you going to be able to use the
> > device in a system that has debugfs disabled?
> 
> 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.

Also, that was a v21 of the patch series, why did the numbering start
over here at v1?

thanks,

greg k-h
Souradeep Chowdhury April 17, 2023, 6:56 a.m. UTC | #4
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:
>>> On Fri, Apr 14, 2023 at 07:29:12PM +0530, Souradeep Chowdhury wrote:
>>>> The DCC is a DMA Engine designed to capture and store data
>>>> during system crash or software triggers. The DCC operates
>>>> based on user inputs via the debugfs interface. The user gives
>>>> addresses as inputs and these addresses are stored in the
>>>> dcc sram. In case of a system crash or a manual software
>>>> trigger by the user through the debugfs interface,
>>>> the dcc captures and stores the values at these addresses.
>>>> This patch contains the driver which has all the methods
>>>> pertaining to the debugfs interface, auxiliary functions to
>>>> support all the four fundamental operations of dcc namely
>>>> read, write, read/modify/write and loop. The probe method
>>>> here instantiates all the resources necessary for dcc to
>>>> operate mainly the dedicated dcc sram where it stores the
>>>> values. The DCC driver can be used for debugging purposes
>>>> without going for a reboot since it can perform software
>>>> triggers as well based on user inputs.
>>>
>>> You have 72 columns, why not use them all please?
>>>
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
>>>
>>> It is now 2023 :)
>>
>> Ack
>>
>>>
>>>
>>>
>>>
>>>> + */
>>>> +
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/fs.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/miscdevice.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/uaccess.h>
>>>> +
>>>> +#define STATUS_READY_TIMEOUT		5000  /* microseconds */
>>>> +
>>>> +#define DCC_SRAM_NODE "dcc_sram"
>>>
>>> You only use this once, why is a #define needed?
>>
>> This is as per the comment on version 1 of the patch series on DCC
>>
>> https://lore.kernel.org/linux-arm-kernel/YElUCaBUOx7hEuIh@builder.lan/
>>
>> "kzalloc + strlcpy can be replaced by kstrdup(), but that said...all this
>> does seems to be to copy a const string to the heap and lugging it
>> around. Use a define instead."
> 
> But as you are not doing any of that here, just using the string in the
> one place it is needed is the same thing :)

Ack

> 
>>>> +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.

> 
>>>> +	if (IS_ERR(drvdata->dbg_dir)) {
>>>> +		pr_err("can't create debugfs dir\n");
>>>
>>> There is no need to ever check the return value of a debugfs call.
>>>
>>> Nor do you really ever even need to save off the dentry here, just look
>>> it up when you need to remove it.
>>
>> Ack
>>
>>>
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i <= drvdata->nr_link_list; i++) {
>>>> +		sprintf(list_num, "%d", i);
>>>> +		list = debugfs_create_dir(list_num, drvdata->dbg_dir);
>>>> +		debugfs_create_file("enable", 0600, list, drvdata, &enable_fops);
>>>> +		debugfs_create_file("config", 0600, list, drvdata, &config_fops);
>>>> +	}
>>>> +
>>>> +	debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops);
>>>> +	debugfs_create_file("ready", 0400, drvdata->dbg_dir, drvdata, &ready_fops);
>>>> +	debugfs_create_file("config_reset", 0200, drvdata->dbg_dir,
>>>> +			    drvdata, &config_reset_fops);
>>>
>>> This really looks like you are using debugfs to control the device, not
>>> just for debugging information.  How are you going to be able to use the
>>> device in a system that has debugfs disabled?
>>
>> 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/

> 
> Also, that was a v21 of the patch series, why did the numbering start
> over here at v1?

Ack. It was decided prior to merge this driver under drivers/soc/qcom 
but after discussions it was concluded that drivers/misc will be the 
suitable home for this driver. Will post subsequent versions as a 
continuation of the previous series for convenience.


> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman April 17, 2023, 7:41 a.m. UTC | #5
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?

> > > 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
Souradeep Chowdhury April 17, 2023, 8:50 a.m. UTC | #6
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