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
