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

Hans de Goede
2024-09-08T16:38:28Z

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
>>
>