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

Chen, Mike Ximing
2021-12-21T23:18:29Z

> -----Original Message-----
> From: Joe Perches <joe@perches.com>
> Sent: Tuesday, December 21, 2021 2:20 AM
> To: Chen, Mike Ximing <mike.ximing.chen@intel.com>; linux-kernel@vger.kernel.org
> Cc: arnd@arndb.de; gregkh@linuxfoundation.org; Williams, Dan J <dan.j.williams@intel.com>; 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:
> > 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.
> []
> > diff --git a/drivers/misc/dlb/dlb_args.h b/drivers/misc/dlb/dlb_args.h
> []
> > @@ -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

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

> > diff --git a/drivers/misc/dlb/dlb_pf_ops.c
> > b/drivers/misc/dlb/dlb_pf_ops.c
> []
> > @@ -102,3 +102,198 @@ void dlb_pf_init_hardware(struct dlb *dlb)
> []
> > +#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.

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

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