| Message ID | 20240908144414.82887-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [6.11,regression,fix] power: supply: sysfs: Revert use power_supply_property_is_writeable() | expand |
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 >
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 >> >
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 ...
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
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;
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(-)