Messages in this thread | | | Date | Tue, 11 May 2010 19:58:12 +0200 | From | Daniel Mack <> | Subject | Re: [PATCH 1/3] pda_power: add support for writeable properties |
| |
On Tue, May 11, 2010 at 09:47:08PM +0400, Anton Vorontsov wrote: > On Tue, May 11, 2010 at 06:38:44PM +0200, Daniel Mack wrote: > > 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'.
Me neither, but I looked it up, and it's in fact both allowed ;)
> > +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. :-)
Hmm, no. The code defaults to (S_IRUSR | S_IRGRP | S_IROTH) which is 0444, just like it was before. So there shouldn't be any regression. The mode is only changed if the psy defines a property_is_writeable() callback which returns 1. Or do I miss your point?
> 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().
Ugh, really? I would _much_ prefer to actually _see_ which property is writeable, just from looking at the file attributes in sysfs.
Thanks, Daniel
| |