Hi,

On Wed, Sep 29, 2021 at 11:38:12AM +0200, Hans de Goede wrote:
> On 9/28/21 10:11 PM, Nicoḷ Piazzalunga wrote:
> > this is a proposal to introduce separate and standardized attributes
> > for force_discharge and inhibit_charge of a battery.
> > These are simpler than using status from a user-space perspective,
> > as discussed on the platform-driver-x86 mail list.
> 
> To clarify things a bit here, the reasons for not using status for
> this are:
> 
> 1. This would require adding new status-es which so far have never
> been seen by userspace, which will likely cause confusion of e.g.
> upower. IOW I believe that adding new status-es for this would
> be a userspace ABI break.

Some embedded devices have status writable and use existing status
and reset when adapter is replugged. That's why there are no extra
status already.

> 2. The devices where we currently want to use this functionality
> use the ACPI battery interface, which is standardized between
> vendors and dealt with by drivers/apci/battery.c  but this kinda
> extra functionality is vendor specific. The drivers/apci/battery.c
> has code allowing vendor drivers to "hook" ACPI batteries and get
> add/remove calls for them. Then in these calls currently the
> vendor drivers do:
> 
> 	device_add_groups(&battery->dev, my_prop_group))
> 
> Which allows them to register extra sysfs_attributes for
> for example charge_control_start_threshold and
> charge_control_end_threshold.
> 
> This works well, but having vendor drivers somehow intercept /
> muck with the status handling in drivers/apci/battery.c is a
> non trival problem. Where as with new separate attributes
> this is already a solved problem.

The second argument is a very weak one. We do not accept bad
userspace API to avoid restructuring a driver. FWIW adding
attributes that way is already racy and a bug:

http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

The power-supply subsystem offers registering custom sysfs
files since a few kernel releases by setting up the following:

/* Run-time specific power supply configuration */
struct power_supply_config {
    ...
   	/* Device specific sysfs attributes */
	const struct attribute_group **attr_grp;
    ...
}

All drivers in drivers/power/supply/ have been converted and
the ACPI is one of the last drivers not using this.

> > ---
> >  Documentation/ABI/testing/sysfs-class-power | 27 +++++++++++++++++++++
> >  drivers/power/supply/power_supply_sysfs.c   |  2 ++
> >  include/linux/power_supply.h                |  2 ++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> > index ca830c6cd809..2c5f48f49273 100644
> > --- a/Documentation/ABI/testing/sysfs-class-power
> > +++ b/Documentation/ABI/testing/sysfs-class-power
> > @@ -455,6 +455,33 @@ Description:
> >  			      "Unknown", "Charging", "Discharging",
> >  			      "Not charging", "Full"
> >  
> > +What:		/sys/class/power_supply/<supply_name>/force_discharge
> > +Date:		September 2021
> > +Contact:	linux-pm@vger.kernel.org
> > +Description:
> > +		Represents the forced discharging status of the battery.
> > +
> > +		Access: Read, Write
> > +
> > +		Valid values:
> > +			== ====================================
> > +			0: Force discharge while AC is attached
> > +			1: Terminate forced discharging
> > +
> 
> I think you have 0 and 1 swapped here? I would expect 1 to be enable forced
> discharging and 0 be normal operation, iow only discharge when not on AC.

Considering some Thinkpads have two batteries the naming and
description of this is quite bad. Only the valid values part
suggests that this is just about AC.

> > +What:		/sys/class/power_supply/<supply_name>/inhibit_charge
> > +Date:		September 2021
> > +Contact:	linux-pm@vger.kernel.org
> > +Description:
> > +		Represents the presence of a manual override over the threshold
> > +		attributes of the battery, thus inhibiting battery charge.

The first part is specific to thinkpad implementation.

> > +
> > +		Access: Read, Write
> > +
> > +		Valid values:
> > +			== ======================
> > +			1: Stop charging
> > +			0: Terminate the override
> > +

I wonder if its better to put both into the same file, considering
that the only logical options are

 * 0 = no override (= charge when possible)
 * 1 = force idle
 * 2 = force discharge

I.e. forced discharge implies battery not being charged.

-- Sebastian

> >  What:		/sys/class/power_supply/<supply_name>/technology
> >  Date:		May 2007
> >  Contact:	linux-pm@vger.kernel.org
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index c3d7cbcd4fad..6e7303935810 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -136,6 +136,8 @@ static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
> >  static struct power_supply_attr power_supply_attrs[] = {
> >  	/* Properties of type `int' */
> >  	POWER_SUPPLY_ENUM_ATTR(STATUS),
> > +	POWER_SUPPLY_ENUM_ATTR(FORCE_DISCHARGE),
> > +	POWER_SUPPLY_ENUM_ATTR(INHIBIT_CHARGE),
> >  	POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPE),
> >  	POWER_SUPPLY_ENUM_ATTR(HEALTH),
> >  	POWER_SUPPLY_ATTR(PRESENT),
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 9ca1f120a211..4340fe65df4d 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -96,6 +96,8 @@ enum {
> >  enum power_supply_property {
> >  	/* Properties of type `int' */
> >  	POWER_SUPPLY_PROP_STATUS = 0,
> > +	POWER_SUPPLY_PROP_FORCE_DISCHARGE,
> > +	POWER_SUPPLY_PROP_INHIBIT_CHARGE,
> >  	POWER_SUPPLY_PROP_CHARGE_TYPE,
> >  	POWER_SUPPLY_PROP_HEALTH,
> >  	POWER_SUPPLY_PROP_PRESENT,
> > 
> 
> Regards,
> 
> Hans
>