RE: [RFC PATCH v12 17/17] dlb: add basic sysfs interfaces

-----Original Message-----
From: Joe Perches <joe@perches.com>
Sent: Tuesday, December 21, 2021 2:20 AM
To: Chen, Mike Ximing <redacted>; linux-kernel@vger.kernel.org
Cc: arnd@arndb.de; gregkh@linuxfoundation.org; Williams, Dan J <redacted>; pierre-
louis.bossart@linux.intel.com; netdev@vger.kernel.org; davem@davemloft.net; kuba@kernel.org
Subject: Re: [RFC PATCH v12 17/17] dlb: add basic sysfs interfaces

On Tue, 2021-12-21 at 00:50 -0600, Mike Ximing Chen wrote:

quoted

The dlb sysfs interfaces include files for reading the total and
available device resources, and reading the device ID and version. The
interfaces are used for device level configurations and resource
inquiries.

[]

quoted

diff --git a/drivers/misc/dlb/dlb_args.h b/drivers/misc/dlb/dlb_args.h

[]

quoted

@@ -58,6 +58,40 @@ struct dlb_create_sched_domain_args {
 	__u32 num_dir_credits;
 };

+/*
+ * dlb_get_num_resources_args: Used to get the number of available resources
+ *      (queues, ports, etc.) that this device owns.
+ *
+ * Output parameters:
+ * @response.status: Detailed error code. In certain cases, such as if the
+ *	request arg is invalid, the driver won't set status.
+ * @num_domains: Number of available scheduling domains.
+ * @num_ldb_queues: Number of available load-balanced queues.
+ * @num_ldb_ports: Total number of available load-balanced ports.
+ * @num_dir_ports: Number of available directed ports. There is one directed
+ *	queue for every directed port.
+ * @num_atomic_inflights: Amount of available temporary atomic QE storage.
+ * @num_hist_list_entries: Amount of history list storage.
+ * @max_contiguous_hist_list_entries: History list storage is allocated in
+ *	a contiguous chunk, and this return value is the longest available
+ *	contiguous range of history list entries.
+ * @num_ldb_credits: Amount of available load-balanced QE storage.
+ * @num_dir_credits: Amount of available directed QE storage.
+ */

Is this supposed to be kernel-doc format with /** as the comment initiator ?

Yes. Thanks

quoted

+struct dlb_get_num_resources_args {
+	/* Output parameters */
+	struct dlb_cmd_response response;
+	__u32 num_sched_domains;
+	__u32 num_ldb_queues;
+	__u32 num_ldb_ports;
+	__u32 num_dir_ports;
+	__u32 num_atomic_inflights;
+	__u32 num_hist_list_entries;
+	__u32 max_contiguous_hist_list_entries;
+	__u32 num_ldb_credits;
+	__u32 num_dir_credits;

__u32 is used when visible to user-space.
Do these really need to use __u32 and not u32 ?

Will change to u32.

quoted

diff --git a/drivers/misc/dlb/dlb_pf_ops.c

b/drivers/misc/dlb/dlb_pf_ops.c

[]

quoted

@@ -102,3 +102,198 @@ void dlb_pf_init_hardware(struct dlb *dlb)

[]

quoted

+#define DLB_TOTAL_SYSFS_SHOW(name, macro)		\
+static ssize_t total_##name##_show(			\
+	struct device *dev,				\
+	struct device_attribute *attr,			\
+	char *buf)					\
+{							\
+	int val = DLB_MAX_NUM_##macro;			\
+							\
+	return scnprintf(buf, PAGE_SIZE, "%d\n", val);	\

Use sysfs_emit rather than scnprintf

maybe:
	return sysfs_emit(buf, "%u\n", DLB_MAX_NUM_##macro);

Yes. Thanks for the suggestion.

quoted

+#define DLB_AVAIL_SYSFS_SHOW(name)			     \
+static ssize_t avail_##name##_show(			     \
+	struct device *dev,				     \
+	struct device_attribute *attr,			     \
+	char *buf)					     \
+{							     \
+	struct dlb *dlb = dev_get_drvdata(dev);		     \
+	struct dlb_get_num_resources_args arg;		     \
+	struct dlb_hw *hw = &dlb->hw;			     \
+	int val;					     \

u32 val?

It should be int. dlb_hw_get_num_resources(() returns int.

quoted

+							     \
+	mutex_lock(&dlb->resource_mutex);		     \
+							     \
+	val = dlb_hw_get_num_resources(hw, &arg);	     \
+							     \
+	mutex_unlock(&dlb->resource_mutex);		     \
+							     \
+	if (val)					     \
+		return -1;				     \
+							     \
+	val = arg.name;					     \
+							     \
+	return scnprintf(buf, PAGE_SIZE, "%d\n", val);	     \

sysfs_emit, etc...