lkml.org 
[lkml]   [2010]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2010-05-12 10:09    [from the cache]
©2003-2011 Jasper Spaans