linux-kernel - Re: [RFC v01 2/3] PowerCap: Add class driver

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1375483407.2034.146.camel@joe-AO722>
Date:	Fri, 02 Aug 2013 15:43:27 -0700
From:	Joe Perches <joe@...ches.com>
To:	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:	linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org
Subject: Re: [RFC v01 2/3] PowerCap: Add class driver

On Fri, 2013-08-02 at 11:08 -0700, Srinivas Pandruvada wrote:
> Added power cap class driver, which provides an API for client drivers
> to use and provide a consistant sys-fs interface to user mode.

Just some stylistic notes.

> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
[]
> +/* Power zone show function */
> +#define define_device_show(_attr)		\
> +static ssize_t show_##_attr(struct device *dev, struct device_attribute *attr,\
> +			char *buf) \

Using _attr and another variable named attr is at least
visually confusing.

Maybe use name or type instead of _attr?

> +{ \
> +	u64 value; \
> +	ssize_t len = -EINVAL; \
> +	struct powercap_zone_device *pcd_dev = dev_get_drvdata(dev); \
> +	\
> +	if (pcd_dev && pcd_dev->ops && pcd_dev->ops->get_##_attr) { \
> +		mutex_lock(&pcd_dev->lock); \
> +		if (!pcd_dev->ops->get_##_attr(pcd_dev, &value)) \
> +			len = sprintf(buf, "%lld\n", value); \
> +		mutex_unlock(&pcd_dev->lock); \
> +	} \
> +	\
> +	return len; \
> +}
> +
> +/* Power zone store function; only reset is possible */
> +#define define_device_store(_attr)		\
> +static ssize_t store_##_attr(struct device *dev,\
> +				struct device_attribute *attr, \
> +				const char *buf, size_t count) \

here too

[]

> +/* Power zone constraint show function */
> +#define define_device_constraint_show(_attr) \
> +static ssize_t show_constraint_##_attr(struct device *dev, \
> +				struct device_attribute *attr,\
> +				char *buf) \

and here

[]

> +/* Power zone constraint store function */
> +#define define_device_constraint_store(_attr) \
> +static ssize_t store_constraint_##_attr(struct device *dev,\
> +				struct device_attribute *attr, \
> +				const char *buf, size_t count) \

etc...

> +struct powercap_zone_constraint *powercap_zone_add_constraint(
> +				struct powercap_zone_device *pcd_dev,
> +				struct powercap_zone_constraint_ops *ops)
> +{
[]
> +	dev_dbg(&pcd_dev->device, "powercap_zone_add_constraint\n");

These logging messages aren't useful.
function tracing works very well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/