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

[Hans de Goede <hdegoede@redhat.com>]

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