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

[Souradeep Chowdhury]

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@xxxxxxxxxxx/

"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@xxxxxxxx/

"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@xxxxxxxxxxx/

> thanks,
>
> greg k-h