[Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Rafael J. Wysocki]

Hi Anton,

Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
access the sysfs battery attributes with this commit applied.

Reverting the commit along with commit 0011d2d4a5f7bb5666dcfb9f9b3dbdb084ab98f1
(power_supply: Add support for writeable properties) fixes the problem for me, but
reverting commit 0011d2d4a5f7bb5666dcfb9f9b3dbdb084ab98f1 alone doesn't fix it,
so it looks like your commit is the source of the problem.

Thanks,
Rafael

Re: [Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Daniel Mack]

On Tue, May 25, 2010 at 12:28:24AM +0200, Rafael J. Wysocki wrote:
> Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
> 11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
> access the sysfs battery attributes with this commit applied.

Can you still see the attributes in sysfs? What are their properties?

Thanks,
Daniel

Re: [Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Rafael J. Wysocki]

On Tuesday 25 May 2010, Daniel Mack wrote:
> On Tue, May 25, 2010 at 12:28:24AM +0200, Rafael J. Wysocki wrote:
> > Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> > attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
> > 11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
> > access the sysfs battery attributes with this commit applied.
> 
> Can you still see the attributes in sysfs?

Yes, I can.

> What are their properties?

They don't seem to be different from the properties without the commit.

Yet, the battery monitor evidently thinks the battery is not present (it also
doesn't seem to see the AC adapter).

Thanks,
Rafael

Re: [Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Rafael J. Wysocki]

On Tuesday 25 May 2010, Rafael J. Wysocki wrote:
> On Tuesday 25 May 2010, Daniel Mack wrote:
> > On Tue, May 25, 2010 at 12:28:24AM +0200, Rafael J. Wysocki wrote:
> > > Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> > > attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
> > > 11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
> > > access the sysfs battery attributes with this commit applied.
> > 
> > Can you still see the attributes in sysfs?
> 
> Yes, I can.

Ah, sorry.  The 'type' property is not present (I didn't notice that before).

Rafael

Re: [Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Daniel Mack]

On Tue, May 25, 2010 at 02:20:45AM +0200, Rafael J. Wysocki wrote:
> On Tuesday 25 May 2010, Rafael J. Wysocki wrote:
> > On Tuesday 25 May 2010, Daniel Mack wrote:
> > > On Tue, May 25, 2010 at 12:28:24AM +0200, Rafael J. Wysocki wrote:
> > > > Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> > > > attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
> > > > 11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
> > > > access the sysfs battery attributes with this commit applied.
> > > 
> > > Can you still see the attributes in sysfs?
> > 
> > Yes, I can.
> 
> Ah, sorry.  The 'type' property is not present (I didn't notice that before).

Ah. That I didn't see. Does the (untested) patch below help?

Daniel

>From ea5c518fb287d5e80e9a46ba1f9a17c45141187c Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Tue, 25 May 2010 02:39:45 +0200
Subject: [PATCH] power_supply: fix regression for 'type' property

Commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
attribute groups) causes a regression the power supply core does not
export the 'type' attribute anymore.

POWER_SUPPLY_PROP_TYPE is handled by the power supply core without the
low-level driver, so power_supply_attr_is_visible() must always return
the entry as readable.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/power/power_supply_sysfs.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 6a86cdf..9d30eeb 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -179,14 +179,16 @@ static mode_t power_supply_attr_is_visible(struct kobject *kobj,
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct power_supply *psy = dev_get_drvdata(dev);
+	mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
 	int i;
 
+	if (attrno == POWER_SUPPLY_PROP_TYPE)
+		return mode;
+
 	for (i = 0; i < psy->num_properties; i++) {
 		int property = psy->properties[i];
 
 		if (property == attrno) {
-			mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
-
 			if (psy->property_is_writeable &&
 			    psy->property_is_writeable(psy, property) > 0)
 				mode |= S_IWUSR;
-- 
1.7.1

Re: [Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Rafael J. Wysocki]

On Tuesday 25 May 2010, Daniel Mack wrote:
> On Tue, May 25, 2010 at 02:20:45AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday 25 May 2010, Rafael J. Wysocki wrote:
> > > On Tuesday 25 May 2010, Daniel Mack wrote:
> > > > On Tue, May 25, 2010 at 12:28:24AM +0200, Rafael J. Wysocki wrote:
> > > > > Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> > > > > attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
> > > > > 11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
> > > > > access the sysfs battery attributes with this commit applied.
> > > > 
> > > > Can you still see the attributes in sysfs?
> > > 
> > > Yes, I can.
> > 
> > Ah, sorry.  The 'type' property is not present (I didn't notice that before).
> 
> Ah. That I didn't see. Does the (untested) patch below help?
> 
> Daniel
> 
> From ea5c518fb287d5e80e9a46ba1f9a17c45141187c Mon Sep 17 00:00:00 2001
> From: Daniel Mack <daniel@caiaq.de>
> Date: Tue, 25 May 2010 02:39:45 +0200
> Subject: [PATCH] power_supply: fix regression for 'type' property
> 
> Commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> attribute groups) causes a regression the power supply core does not
> export the 'type' attribute anymore.
> 
> POWER_SUPPLY_PROP_TYPE is handled by the power supply core without the
> low-level driver, so power_supply_attr_is_visible() must always return
> the entry as readable.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Reported-by: Rafael J. Wysocki <rjw@sisk.pl>

Yes, that's it, thanks.

Rafael


> ---
>  drivers/power/power_supply_sysfs.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index 6a86cdf..9d30eeb 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -179,14 +179,16 @@ static mode_t power_supply_attr_is_visible(struct kobject *kobj,
>  {
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct power_supply *psy = dev_get_drvdata(dev);
> +	mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
>  	int i;
>  
> +	if (attrno == POWER_SUPPLY_PROP_TYPE)
> +		return mode;
> +
>  	for (i = 0; i < psy->num_properties; i++) {
>  		int property = psy->properties[i];
>  
>  		if (property == attrno) {
> -			mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> -
>  			if (psy->property_is_writeable &&
>  			    psy->property_is_writeable(psy, property) > 0)
>  				mode |= S_IWUSR;
> 

Re: [Regression] Commit "power_supply: Use attribute groups" breaks KDE battery monitor on openSUSE 11.3 M6

[Daniel Mack]

On Tue, May 25, 2010 at 02:55:11AM +0200, Rafael J. Wysocki wrote:
> On Tuesday 25 May 2010, Daniel Mack wrote:
> > On Tue, May 25, 2010 at 02:20:45AM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday 25 May 2010, Rafael J. Wysocki wrote:
> > > > On Tuesday 25 May 2010, Daniel Mack wrote:
> > > > > On Tue, May 25, 2010 at 12:28:24AM +0200, Rafael J. Wysocki wrote:
> > > > > > Your commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> > > > > > attribute groups) unfortunately breaks KDE 4.4's battery monitor from openSUSE
> > > > > > 11.3 Milestone 6 on my Acer Ferrari One.  Apparently, the battery monitor can't
> > > > > > access the sysfs battery attributes with this commit applied.
> > > > > 
> > > > > Can you still see the attributes in sysfs?
> > > > 
> > > > Yes, I can.
> > > 
> > > Ah, sorry.  The 'type' property is not present (I didn't notice that before).
> > 
> > Ah. That I didn't see. Does the (untested) patch below help?
> > 
> > Daniel
> > 
> > From ea5c518fb287d5e80e9a46ba1f9a17c45141187c Mon Sep 17 00:00:00 2001
> > From: Daniel Mack <daniel@caiaq.de>
> > Date: Tue, 25 May 2010 02:39:45 +0200
> > Subject: [PATCH] power_supply: fix regression for 'type' property
> > 
> > Commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
> > attribute groups) causes a regression the power supply core does not
> > export the 'type' attribute anymore.
> > 
> > POWER_SUPPLY_PROP_TYPE is handled by the power supply core without the
> > low-level driver, so power_supply_attr_is_visible() must always return
> > the entry as readable.
> > 
> > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Yes, that's it, thanks.

Good, thanks for testing.

I think Anton will pick this for battery-2.6.git.


Daniel


> >  drivers/power/power_supply_sysfs.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> > index 6a86cdf..9d30eeb 100644
> > --- a/drivers/power/power_supply_sysfs.c
> > +++ b/drivers/power/power_supply_sysfs.c
> > @@ -179,14 +179,16 @@ static mode_t power_supply_attr_is_visible(struct kobject *kobj,
> >  {
> >  	struct device *dev = container_of(kobj, struct device, kobj);
> >  	struct power_supply *psy = dev_get_drvdata(dev);
> > +	mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> >  	int i;
> >  
> > +	if (attrno == POWER_SUPPLY_PROP_TYPE)
> > +		return mode;
> > +
> >  	for (i = 0; i < psy->num_properties; i++) {
> >  		int property = psy->properties[i];
> >  
> >  		if (property == attrno) {
> > -			mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> > -
> >  			if (psy->property_is_writeable &&
> >  			    psy->property_is_writeable(psy, property) > 0)
> >  				mode |= S_IWUSR;
> > 
> 

[GIT PULL] battery-2.6.git

[Anton Vorontsov]

Hello Linus,

Please pull from

  git://git.infradead.org/battery-2.6.git master

to receive a fixup for regression reported by Rafael J. Wysocki
http://lkml.org/lkml/2010/5/24/273

Thanks!

commit bbabb158f0e9d41174ae5c2183a8e4f981daf6ce
Author: Daniel Mack <daniel@caiaq.de>
Date:   Tue May 25 02:39:45 2010 +0200

    power_supply: Fix regression for 'type' property
    
    Commit 5f487cd34f4337f9bc27ca19da72a39d1b0a0ab4 (power_supply: Use
    attribute groups) causes a regression the power supply core does not
    export the 'type' attribute anymore.
    
    POWER_SUPPLY_PROP_TYPE is handled by the power supply core without the
    low-level driver, so power_supply_attr_is_visible() must always return
    the entry as readable.
    
    Reported-by: Rafael J. Wysocki <rjw@sisk.pl>
    Signed-off-by: Daniel Mack <daniel@caiaq.de>
    Tested-by: Rafael J. Wysocki <rjw@sisk.pl>
    Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 6a86cdf..9d30eeb 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -179,14 +179,16 @@ static mode_t power_supply_attr_is_visible(struct kobject *kobj,
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct power_supply *psy = dev_get_drvdata(dev);
+	mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
 	int i;
 
+	if (attrno == POWER_SUPPLY_PROP_TYPE)
+		return mode;
+
 	for (i = 0; i < psy->num_properties; i++) {
 		int property = psy->properties[i];
 
 		if (property == attrno) {
-			mode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
-
 			if (psy->property_is_writeable &&
 			    psy->property_is_writeable(psy, property) > 0)
 				mode |= S_IWUSR;