[PATCH 6.11 regression fix] power: supply: sysfs: Revert use power_supply_property_is_writeable()

[PATCH 6.11 regression fix] power: supply: sysfs: Revert use power_supply_property_is_writeable()

[Hans de Goede]

power_supply_property_is_writeable() contains the following check:

        if (atomic_read(&psy->use_cnt) <= 0 ||
                        !psy->desc->property_is_writeable)
                return -ENODEV;

psy->use_cnt gets initialized to 0 and is incremented by
__power_supply_register() after it calls device_add(); and thus after
the driver core calls power_supply_attr_is_visible() to get the attr's
permissions.

So when power_supply_attr_is_visible() runs psy->use_cnt is 0 failing
the above check. This is causing all the attributes to have permissions
of 444 even those which should be writable.

Move back to manually calling psy->desc->property_is_writeable() without
the psy->use_cnt check to fix this.

Fixes: be6299c6e55e ("power: supply: sysfs: use power_supply_property_is_writeable()")
Cc: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note this is a straight-forward revert of be6299c6e55e
---
 drivers/power/supply/power_supply_sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 6cd3fac1891b..fb9b67b5a9aa 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -387,7 +387,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 		int property = psy->desc->properties[i];
 
 		if (property == attrno) {
-			if (power_supply_property_is_writeable(psy, property) > 0)
+			if (psy->desc->property_is_writeable &&
+			    psy->desc->property_is_writeable(psy, property) > 0)
 				mode |= S_IWUSR;
 
 			return mode;
-- 
2.46.0

Re: [PATCH 6.11 regression fix] power: supply: sysfs: Revert use power_supply_property_is_writeable()

[Thomas Weißschuh]

Hi Hans,

On 2024-09-08 16:44:14+0000, Hans de Goede wrote:
> power_supply_property_is_writeable() contains the following check:
> 
>         if (atomic_read(&psy->use_cnt) <= 0 ||
>                         !psy->desc->property_is_writeable)
>                 return -ENODEV;
> 
> psy->use_cnt gets initialized to 0 and is incremented by
> __power_supply_register() after it calls device_add(); and thus after
> the driver core calls power_supply_attr_is_visible() to get the attr's
> permissions.
> 
> So when power_supply_attr_is_visible() runs psy->use_cnt is 0 failing
> the above check. This is causing all the attributes to have permissions
> of 444 even those which should be writable.
> 
> Move back to manually calling psy->desc->property_is_writeable() without
> the psy->use_cnt check to fix this.

Thanks for the fix!

OTOH the whole power_supply_attr_is_visible() is completely unused
outisde of the psy core. So instead we could unexport it, drop the use_cnt
check and use it again.
(All of this as part of the psy extension series, for now your revert
should be used)

What do you think?

> 
> Fixes: be6299c6e55e ("power: supply: sysfs: use power_supply_property_is_writeable()")
> Cc: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note this is a straight-forward revert of be6299c6e55e
> ---
>  drivers/power/supply/power_supply_sysfs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 6cd3fac1891b..fb9b67b5a9aa 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -387,7 +387,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>  		int property = psy->desc->properties[i];
>  
>  		if (property == attrno) {
> -			if (power_supply_property_is_writeable(psy, property) > 0)
> +			if (psy->desc->property_is_writeable &&
> +			    psy->desc->property_is_writeable(psy, property) > 0)
>  				mode |= S_IWUSR;
>  
>  			return mode;
> -- 
> 2.46.0
> 

Re: [PATCH 6.11 regression fix] power: supply: sysfs: Revert use power_supply_property_is_writeable()

[Hans de Goede]

Hi,

On 9/8/24 4:52 PM, Thomas Weißschuh wrote:
> Hi Hans,
> 
> On 2024-09-08 16:44:14+0000, Hans de Goede wrote:
>> power_supply_property_is_writeable() contains the following check:
>>
>>         if (atomic_read(&psy->use_cnt) <= 0 ||
>>                         !psy->desc->property_is_writeable)
>>                 return -ENODEV;
>>
>> psy->use_cnt gets initialized to 0 and is incremented by
>> __power_supply_register() after it calls device_add(); and thus after
>> the driver core calls power_supply_attr_is_visible() to get the attr's
>> permissions.
>>
>> So when power_supply_attr_is_visible() runs psy->use_cnt is 0 failing
>> the above check. This is causing all the attributes to have permissions
>> of 444 even those which should be writable.
>>
>> Move back to manually calling psy->desc->property_is_writeable() without
>> the psy->use_cnt check to fix this.
> 
> Thanks for the fix!
> 
> OTOH the whole power_supply_attr_is_visible() is completely unused
> outisde of the psy core. So instead we could unexport it, drop the use_cnt
> check and use it again.
> (All of this as part of the psy extension series, for now your revert
> should be used)
> 
> What do you think?

So I took a look at other users of power_supply_attr_is_visible() and
the only other user is power_supply_hwmon_is_visible() which suffers
from the exact same problem.

NACK.

So self-nack for this fix. It is better to drop the use_cnt check
from power_supply_property_is_writeable() altogether since currently
all hwmon attributes are always 0444 too. I checked with a max170xx_battery
where /sys/class/hwmon/hwmon5/temp1_min_alarm should be 0644, but
until I fix power_supply_property_is_writeable() it is 0444.

Also temp1_max_alarm is missing from the hwmon, I also have a fix
for that one ...

Regards,

Hans


>> Fixes: be6299c6e55e ("power: supply: sysfs: use power_supply_property_is_writeable()")
>> Cc: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note this is a straight-forward revert of be6299c6e55e
>> ---
>>  drivers/power/supply/power_supply_sysfs.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> index 6cd3fac1891b..fb9b67b5a9aa 100644
>> --- a/drivers/power/supply/power_supply_sysfs.c
>> +++ b/drivers/power/supply/power_supply_sysfs.c
>> @@ -387,7 +387,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>>  		int property = psy->desc->properties[i];
>>  
>>  		if (property == attrno) {
>> -			if (power_supply_property_is_writeable(psy, property) > 0)
>> +			if (psy->desc->property_is_writeable &&
>> +			    psy->desc->property_is_writeable(psy, property) > 0)
>>  				mode |= S_IWUSR;
>>  
>>  			return mode;
>> -- 
>> 2.46.0
>>
> 

Re: [PATCH 6.11 regression fix] power: supply: sysfs: Revert use power_supply_property_is_writeable()

[Thomas Weißschuh]

On 2024-09-08 18:38:21+0000, Hans de Goede wrote:
> Hi,
> 
> On 9/8/24 4:52 PM, Thomas Weißschuh wrote:
> > Hi Hans,
> > 
> > On 2024-09-08 16:44:14+0000, Hans de Goede wrote:
> >> power_supply_property_is_writeable() contains the following check:
> >>
> >>         if (atomic_read(&psy->use_cnt) <= 0 ||
> >>                         !psy->desc->property_is_writeable)
> >>                 return -ENODEV;
> >>
> >> psy->use_cnt gets initialized to 0 and is incremented by
> >> __power_supply_register() after it calls device_add(); and thus after
> >> the driver core calls power_supply_attr_is_visible() to get the attr's
> >> permissions.
> >>
> >> So when power_supply_attr_is_visible() runs psy->use_cnt is 0 failing
> >> the above check. This is causing all the attributes to have permissions
> >> of 444 even those which should be writable.
> >>
> >> Move back to manually calling psy->desc->property_is_writeable() without
> >> the psy->use_cnt check to fix this.
> > 
> > Thanks for the fix!
> > 
> > OTOH the whole power_supply_attr_is_visible() is completely unused
> > outisde of the psy core. So instead we could unexport it, drop the use_cnt
> > check and use it again.
> > (All of this as part of the psy extension series, for now your revert
> > should be used)
> > 
> > What do you think?
> 
> So I took a look at other users of power_supply_attr_is_visible() and
> the only other user is power_supply_hwmon_is_visible() which suffers
> from the exact same problem.
> 
> NACK.
> 
> So self-nack for this fix. It is better to drop the use_cnt check
> from power_supply_property_is_writeable() altogether since currently
> all hwmon attributes are always 0444 too. I checked with a max170xx_battery
> where /sys/class/hwmon/hwmon5/temp1_min_alarm should be 0644, but
> until I fix power_supply_property_is_writeable() it is 0444.

IMO it should also be unexported and renamed to
psy_property_is_writeable() to signify that it's internal to the psy
core.

> Also temp1_max_alarm is missing from the hwmon, I also have a fix
> for that one ...

Re: [PATCH 6.11 regression fix] power: supply: sysfs: Revert use power_supply_property_is_writeable()

[Hans de Goede]

Hi,

On 9/8/24 7:28 PM, Thomas Weißschuh wrote:
> On 2024-09-08 18:38:21+0000, Hans de Goede wrote:
>> Hi,
>>
>> On 9/8/24 4:52 PM, Thomas Weißschuh wrote:
>>> Hi Hans,
>>>
>>> On 2024-09-08 16:44:14+0000, Hans de Goede wrote:
>>>> power_supply_property_is_writeable() contains the following check:
>>>>
>>>>         if (atomic_read(&psy->use_cnt) <= 0 ||
>>>>                         !psy->desc->property_is_writeable)
>>>>                 return -ENODEV;
>>>>
>>>> psy->use_cnt gets initialized to 0 and is incremented by
>>>> __power_supply_register() after it calls device_add(); and thus after
>>>> the driver core calls power_supply_attr_is_visible() to get the attr's
>>>> permissions.
>>>>
>>>> So when power_supply_attr_is_visible() runs psy->use_cnt is 0 failing
>>>> the above check. This is causing all the attributes to have permissions
>>>> of 444 even those which should be writable.
>>>>
>>>> Move back to manually calling psy->desc->property_is_writeable() without
>>>> the psy->use_cnt check to fix this.
>>>
>>> Thanks for the fix!
>>>
>>> OTOH the whole power_supply_attr_is_visible() is completely unused
>>> outisde of the psy core. So instead we could unexport it, drop the use_cnt
>>> check and use it again.
>>> (All of this as part of the psy extension series, for now your revert
>>> should be used)
>>>
>>> What do you think?
>>
>> So I took a look at other users of power_supply_attr_is_visible() and
>> the only other user is power_supply_hwmon_is_visible() which suffers
>> from the exact same problem.
>>
>> NACK.
>>
>> So self-nack for this fix. It is better to drop the use_cnt check
>> from power_supply_property_is_writeable() altogether since currently
>> all hwmon attributes are always 0444 too. I checked with a max170xx_battery
>> where /sys/class/hwmon/hwmon5/temp1_min_alarm should be 0644, but
>> until I fix power_supply_property_is_writeable() it is 0444.
> 
> IMO it should also be unexported and renamed to
> psy_property_is_writeable() to signify that it's internal to the psy
> core.

Agreed, but that is something for a follow-up patch.

Feel free to add a patch for that to the next version of
your psy extension series :)

Regards,

Hans