Re: [PATCH 1/3] pda_power: add support for writeable properties

[Anton Vorontsov <cbouatmailru@gmail.com>]

On Tue, May 11, 2010 at 06:38:44PM +0200, Daniel Mack wrote:
> This patch adds support for writeable power supply properties and
> exposes them as writeable to sysfs.

A long-awaited feature! Thanks!

> 
> A power supply implementation must implement two new function calls in
> order to use that feature:
> 
>   int set_property(struct power_supply *psy,
>                    enum power_supply_property psp,
>                    const union power_supply_propval *val);
> 
>   int property_is_writeable(struct power_supply *psy,

I'm not a native English speaker, but I think this should be
'writable'.

>                             enum power_supply_property psp);
[...]
>  #define POWER_SUPPLY_ATTR(_name)					\
>  {									\
> -	.attr = { .name = #_name, .mode = 0444 },	\
> +	.attr = { .name = #_name },					\
>  	.show = power_supply_show_property,				\
> -	.store = NULL,							\
> +	.store = power_supply_store_property,				\
>  }
>  
>  static struct device_attribute power_supply_attrs[];
> @@ -91,6 +91,25 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	return sprintf(buf, "%d\n", value.intval);
>  }
>  
> +static ssize_t power_supply_store_property(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf, size_t count) {
> +	ssize_t ret;
> +	struct power_supply *psy = dev_get_drvdata(dev);
> +	const ptrdiff_t off = attr - power_supply_attrs;
> +	union power_supply_propval value;
> +	long long_val;
> +
> +	/* TODO: support other types than int */
> +	ret = strict_strtol(buf, 10, &long_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	value.intval = long_val;
> +
> +	return psy->set_property(psy, off, &value);
> +}
> +
>  /* Must be in the same order as POWER_SUPPLY_PROP_* */
>  static struct device_attribute power_supply_attrs[] = {
>  	/* Properties of type `int' */
> @@ -164,6 +183,14 @@ int power_supply_create_attrs(struct power_supply *psy)
>  	}
>  
>  	for (j = 0; j < psy->num_properties; j++) {
> +		mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> +
> +		if (psy->property_is_writeable &&
> +		    psy->property_is_writeable(psy, psy->properties[j]) > 0)
> +			mode |= S_IWUSR;
> +
> +		power_supply_attrs[psy->properties[j]].attr.mode = mode;

This is dangerous. You're changing the attr mode for all power
supplies, including already registered. I have no idea how attr
handling core will cope with that, but we'd better not check
this. :-)

Instead, change the mode to '0644' unconditionally,
and in power_supply_store_property() do something like this:
{
	if (!psy->set_property)
		return -EINVAL; (or EPERM, not sure which is better).
	....
	return psy->set_property(psy, off, &value);
	/* ^^^here set_property() should -EPERM if some property
	 * is read-only.
	 */
}

Plus, that way you don't need is_writable().

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2