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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SJ0PR11MB51810AC3BFBB39F7D0833F82D97C9@SJ0PR11MB5181.namprd11.prod.outlook.com>
Date:   Tue, 21 Dec 2021 23:18:22 +0000
From:   "Chen, Mike Ximing" <mike.ximing.chen@...el.com>
To:     Joe Perches <joe@...ches.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "arnd@...db.de" <arnd@...db.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "pierre-louis.bossart@...ux.intel.com" 
        <pierre-louis.bossart@...ux.intel.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: RE: [RFC PATCH v12 17/17] dlb: add basic sysfs interfaces


> -----Original Message-----
> From: Joe Perches <joe@...ches.com>
> Sent: Tuesday, December 21, 2021 2:20 AM
> To: Chen, Mike Ximing <mike.ximing.chen@...el.com>; linux-kernel@...r.kernel.org
> Cc: arnd@...db.de; gregkh@...uxfoundation.org; Williams, Dan J <dan.j.williams@...el.com>; pierre-
> louis.bossart@...ux.intel.com; netdev@...r.kernel.org; davem@...emloft.net; kuba@...nel.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...
>