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

Thomas Weißschuh
2024-09-08T17:28:53Z

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