Prev: [PATCH 02/25] lmb: No reason to include asm/lmb.h late
Next: perf: core, remove hw_perf_event_init
From: Mark Brown on 10 May 2010 05:50
On Fri, May 07, 2010 at 07:52:11PM +0200, Daniel Mack wrote:

> + /* TODO: support other types than int */
> + ret = strict_strtol(buf, 10, &long_val);
> + if (ret < 0)
> + return ret;

It'd be nicer if we could error out if this is used on properties with
an inappropriate type but we don't seem to have type information
anywhere convenient for implementing that, which is a bit unfortunate.

Otherwise this looks reasonable in itself.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Daniel Mack on 10 May 2010 09:40
On Mon, May 10, 2010 at 10:48:54AM +0100, Mark Brown wrote:
> On Fri, May 07, 2010 at 07:52:11PM +0200, Daniel Mack wrote:
>
> > + /* TODO: support other types than int */
> > + ret = strict_strtol(buf, 10, &long_val);
> > + if (ret < 0)
> > + return ret;
>
> It'd be nicer if we could error out if this is used on properties with
> an inappropriate type but we don't seem to have type information
> anywhere convenient for implementing that, which is a bit unfortunate.

Yep, hence the 'TODO' :)
So currently the verdict is: if we can't convert the input to an
integer, we bail.

> Otherwise this looks reasonable in itself.

Ok, thanks.

Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 11 May 2010 13:50
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(a)gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Daniel Mack on 11 May 2010 14:00
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 11 May 2010 14:30
On Tue, May 11, 2010 at 07:58:12PM +0200, Daniel Mack wrote:
[...]
> 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?

Yes, power_supply_attrs is a global array, and you shouldn't change
it between power_supply_register() calls.

If you don't see why it's a bad idea in general, think about it
other way, a race:

....someone registers psy0 with attr X marked as read-only...
....code flow stops before device_create_file(psy0, global->mode)..
[preempt]
....someone registers psy1 with attr X marked as writable...
....you set up global->mode to 0644...
[preempt again]
....we end up calling device_create_file(psy0, 0644)...

--
Anton Vorontsov
email: cbouatmailru(a)gmail.com
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH 02/25] lmb: No reason to include asm/lmb.h late
Next: perf: core, remove hw_perf_event_init