[RFC] add standardized attributes for force_discharge and inhibit_charge

Commit Message

Nicolò Piazzalunga Sept. 28, 2021, 8:11 p.m. UTC

Hi Sebastian,

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.

Regards,
Nicolò

---
 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(+)

Comments

Hans de Goede Sept. 29, 2021, 9:38 a.m. UTC | #1

Hi,

On 9/28/21 10:11 PM, Nicolò Piazzalunga wrote:
> Hi Sebastian,
> 
> 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.

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.

> ---
>  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:	[e-mail]
> +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.

> +What:		/sys/class/power_supply/<supply_name>/inhibit_charge
> +Date:		September 2021
> +Contact:	[e-mail]
> +Description:
> +		Represents the presence of a manual override over the threshold
> +		attributes of the battery, thus inhibiting battery charge.
> +
> +		Access: Read, Write
> +
> +		Valid values:
> +			== ======================
> +			1: Stop charging
> +			0: Terminate the override
> +
>  What:		/sys/class/power_supply/<supply_name>/technology
>  Date:		May 2007
>  Contact:	[e-mail]
> 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

Nicolò Piazzalunga Sept. 29, 2021, 4:22 p.m. UTC | #2

Hi Hans,

you are right, I swapped 1 and 0 there of course, my mistake.

Nicolò

Sebastian Reichel Oct. 5, 2021, 4:23 p.m. UTC | #3

Hi,

On Wed, Sep 29, 2021 at 11:38:12AM +0200, Hans de Goede wrote:
> On 9/28/21 10:11 PM, Nicolò 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:	[e-mail]
> > +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:	[e-mail]
> > +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:	[e-mail]
> > 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
>

Hans de Goede Oct. 5, 2021, 6:01 p.m. UTC | #4

Hi,

On 10/5/21 6:23 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Sep 29, 2021 at 11:38:12AM +0200, Hans de Goede wrote:
>> On 9/28/21 10:11 PM, Nicolò 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.

Right, so then a userspace process can write "Discharging" to the status
property to enable "forced discharge" mode, I get that.

And from then on any reads of status will read "Discharging" but how
does (another) userspace process now differentiate this forced
"Discharging" from normal discharging ? It cannot differentiate the
2 unless we add a new "Forced Discharging" status string which
comes with the problem of current userspace not being prepared to
deal with this status string.

Which IMHO makes this idea of overloading / adding this new usage
to status a not so great API, this seems like something which
the status attribute was never intended for.

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

I agree that this is a weak argument, but when possible solutions
are otherwise mostly equal, then easy and especially also cleanliness
of implementation definitely comes into play. We don't have an
infinite amount of developer time (the opposite actually).

> 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:	[e-mail]
>>> +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:	[e-mail]
>>> +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.

Right, force-discharge automatically implies charging is
being inhibited, so putting this in one file makes sense.

Any suggestion for the name of the file ?  

Regards,

Hans


>>>  What:		/sys/class/power_supply/<supply_name>/technology
>>>  Date:		May 2007
>>>  Contact:	[e-mail]
>>> 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
>>

Sebastian Reichel Oct. 5, 2021, 10:06 p.m. UTC | #5

Hi,

On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
> Right, force-discharge automatically implies charging is
> being inhibited, so putting this in one file makes sense.
> 
> Any suggestion for the name of the file?

Maybe like this?

---------------------------------------------------------------------
What: /sys/class/power_supply/<supply_name>/charge_behaviour
Date: October 2021
Contact: [e-mail]
Description:
 Configure battery behaviour when a charger is being connected.

 Access: Read, Write

 Valid values:

 0: auto / no override
    When charger is connected battery should be charged
 1: force idle
    When charger is connected the battery should neither be charged
    nor discharged.
 2: force discharge
    When charger is connected the battery should be discharged
    anyways.
---------------------------------------------------------------------

-- Sebastian

Hans de Goede Oct. 6, 2021, 8:10 a.m. UTC | #6

Hi,

On 10/6/21 12:06 AM, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
>> Right, force-discharge automatically implies charging is
>> being inhibited, so putting this in one file makes sense.
>>
>> Any suggestion for the name of the file?
> 
> Maybe like this?
> 
> ---------------------------------------------------------------------
> What: /sys/class/power_supply/<supply_name>/charge_behaviour
> Date: October 2021
> Contact: [e-mail]
> Description:
>  Configure battery behaviour when a charger is being connected.
> 
>  Access: Read, Write
> 
>  Valid values:
> 
>  0: auto / no override
>     When charger is connected battery should be charged
>  1: force idle
>     When charger is connected the battery should neither be charged
>     nor discharged.
>  2: force discharge
>     When charger is connected the battery should be discharged
>     anyways.
> ---------------------------------------------------------------------

That looks good to me. Although I just realized that some hw may
only support 1. or 2. maybe explicitly document this and that
EOPNOTSUPP will be reported when the value is not supported
(vs EINVAL for plain invalid values) ?

Regards,

Hans

Sebastian Reichel Oct. 6, 2021, 2:39 p.m. UTC | #7

Hi Hans,

On Wed, Oct 06, 2021 at 10:10:36AM +0200, Hans de Goede wrote:
> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
> > On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
> >> Right, force-discharge automatically implies charging is
> >> being inhibited, so putting this in one file makes sense.
> >>
> >> Any suggestion for the name of the file?
> > 
> > Maybe like this?
> > 
> > ---------------------------------------------------------------------
> > What: /sys/class/power_supply/<supply_name>/charge_behaviour
> > Date: October 2021
> > Contact: [e-mail]
> > Description:
> >  Configure battery behaviour when a charger is being connected.
> > 
> >  Access: Read, Write
> > 
> >  Valid values:
> > 
> >  0: auto / no override
> >     When charger is connected battery should be charged
> >  1: force idle
> >     When charger is connected the battery should neither be charged
> >     nor discharged.
> >  2: force discharge
> >     When charger is connected the battery should be discharged
> >     anyways.
> > ---------------------------------------------------------------------
> 
> That looks good to me. Although I just realized that some hw may
> only support 1. or 2. maybe explicitly document this and that
> EOPNOTSUPP will be reported when the value is not supported
> (vs EINVAL for plain invalid values)?

Sounds good to me.

-- Sebastian

Thomas Weißschuh Oct. 6, 2021, 2:49 p.m. UTC | #8

On 2021-10-06T10:10+0200, Hans de Goede wrote:
> Hi,
> 
> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
> >> Right, force-discharge automatically implies charging is
> >> being inhibited, so putting this in one file makes sense.
> >>
> >> Any suggestion for the name of the file?
> > 
> > Maybe like this?
> > 
> > ---------------------------------------------------------------------
> > What: /sys/class/power_supply/<supply_name>/charge_behaviour
> > Date: October 2021
> > Contact: [e-mail]
> > Description:
> >  Configure battery behaviour when a charger is being connected.
> > 
> >  Access: Read, Write
> > 
> >  Valid values:
> > 
> >  0: auto / no override
> >     When charger is connected battery should be charged
> >  1: force idle
> >     When charger is connected the battery should neither be charged
> >     nor discharged.
> >  2: force discharge
> >     When charger is connected the battery should be discharged
> >     anyways.
> > ---------------------------------------------------------------------
> 
> That looks good to me. Although I just realized that some hw may
> only support 1. or 2. maybe explicitly document this and that
> EOPNOTSUPP will be reported when the value is not supported
> (vs EINVAL for plain invalid values) ?

Would that not force a userspace applications to offer all possibilities to
the user only to tell them that it's not supported?
If the driver knows what is supported and what not it should make this
discoverable without actually performing the operation.

Maybe something along the lines of /sys/power/mem_sleep.

Thomas

Hans de Goede Oct. 6, 2021, 3:27 p.m. UTC | #9

Hi,

On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
> On 2021-10-06T10:10+0200, Hans de Goede wrote:
>> Hi,
>>
>> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
>>> Hi,
>>>
>>> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
>>>> Right, force-discharge automatically implies charging is
>>>> being inhibited, so putting this in one file makes sense.
>>>>
>>>> Any suggestion for the name of the file?
>>>
>>> Maybe like this?
>>>
>>> ---------------------------------------------------------------------
>>> What: /sys/class/power_supply/<supply_name>/charge_behaviour
>>> Date: October 2021
>>> Contact: [e-mail]
>>> Description:
>>>  Configure battery behaviour when a charger is being connected.
>>>
>>>  Access: Read, Write
>>>
>>>  Valid values:
>>>
>>>  0: auto / no override
>>>     When charger is connected battery should be charged
>>>  1: force idle
>>>     When charger is connected the battery should neither be charged
>>>     nor discharged.
>>>  2: force discharge
>>>     When charger is connected the battery should be discharged
>>>     anyways.
>>> ---------------------------------------------------------------------
>>
>> That looks good to me. Although I just realized that some hw may
>> only support 1. or 2. maybe explicitly document this and that
>> EOPNOTSUPP will be reported when the value is not supported
>> (vs EINVAL for plain invalid values) ?
> 
> Would that not force a userspace applications to offer all possibilities to
> the user only to tell them that it's not supported?
> If the driver knows what is supported and what not it should make this
> discoverable without actually performing the operation.
> 
> Maybe something along the lines of /sys/power/mem_sleep.

Good point, but something like /sys/power/mem_sleep works
very differently then how all the other power_supply properties work.

In general if something is supported or not on a psy class
device is communicated by the presence / absence of attributes.

So I think we should move back to having 2 separate attributes
for this after all; and group the 2 together in the doc and
document that enabling (setting to 1) one of force_charge /
inhibit_charge automatically clears the setting of the other.

Then the availability of the features can simply be probed
by checking for the presence of the property files.

Regards,

Hans

Sebastian Reichel Oct. 6, 2021, 4:28 p.m. UTC | #10

Hi,

On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
> On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
> > On 2021-10-06T10:10+0200, Hans de Goede wrote:
> >> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
> >>> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
> >>>> Right, force-discharge automatically implies charging is
> >>>> being inhibited, so putting this in one file makes sense.
> >>>>
> >>>> Any suggestion for the name of the file?
> >>>
> >>> Maybe like this?
> >>>
> >>> ---------------------------------------------------------------------
> >>> What: /sys/class/power_supply/<supply_name>/charge_behaviour
> >>> Date: October 2021
> >>> Contact: [e-mail]
> >>> Description:
> >>>  Configure battery behaviour when a charger is being connected.
> >>>
> >>>  Access: Read, Write
> >>>
> >>>  Valid values:
> >>>
> >>>  0: auto / no override
> >>>     When charger is connected battery should be charged
> >>>  1: force idle
> >>>     When charger is connected the battery should neither be charged
> >>>     nor discharged.
> >>>  2: force discharge
> >>>     When charger is connected the battery should be discharged
> >>>     anyways.
> >>> ---------------------------------------------------------------------
> >>
> >> That looks good to me. Although I just realized that some hw may
> >> only support 1. or 2. maybe explicitly document this and that
> >> EOPNOTSUPP will be reported when the value is not supported
> >> (vs EINVAL for plain invalid values) ?
> > 
> > Would that not force a userspace applications to offer all possibilities to
> > the user only to tell them that it's not supported?
> > If the driver knows what is supported and what not it should make this
> > discoverable without actually performing the operation.
> > 
> > Maybe something along the lines of /sys/power/mem_sleep.
> 
> Good point, but something like /sys/power/mem_sleep works
> very differently then how all the other power_supply properties work.

Actually we already use this format in power-supply for USB
types, implemented in power_supply_show_usb_type().

> In general if something is supported or not on a psy class
> device is communicated by the presence / absence of attributes.
>
> So I think we should move back to having 2 separate attributes
> for this after all; and group the 2 together in the doc and
> document that enabling (setting to 1) one of force_charge /
> inhibit_charge automatically clears the setting of the other.
> 
> Then the availability of the features can simply be probed
> by checking for the presence of the property files.

If it's two files, then somebody needs to come up with proper 
names. Things like 'force_discharge' look sensible in this context,
but on a system with two batteries (like some Thinkpads have) it
is easy to confuse with "I want to discharge this battery before
the other one (while no AC is connected)".

-- Sebastian

Hans de Goede Oct. 6, 2021, 5:47 p.m. UTC | #11

Hi,

On 10/6/21 6:28 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
>> On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
>>> On 2021-10-06T10:10+0200, Hans de Goede wrote:
>>>> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
>>>>> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
>>>>>> Right, force-discharge automatically implies charging is
>>>>>> being inhibited, so putting this in one file makes sense.
>>>>>>
>>>>>> Any suggestion for the name of the file?
>>>>>
>>>>> Maybe like this?
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> What: /sys/class/power_supply/<supply_name>/charge_behaviour
>>>>> Date: October 2021
>>>>> Contact: [e-mail]
>>>>> Description:
>>>>>  Configure battery behaviour when a charger is being connected.
>>>>>
>>>>>  Access: Read, Write
>>>>>
>>>>>  Valid values:
>>>>>
>>>>>  0: auto / no override
>>>>>     When charger is connected battery should be charged
>>>>>  1: force idle
>>>>>     When charger is connected the battery should neither be charged
>>>>>     nor discharged.
>>>>>  2: force discharge
>>>>>     When charger is connected the battery should be discharged
>>>>>     anyways.
>>>>> ---------------------------------------------------------------------
>>>>
>>>> That looks good to me. Although I just realized that some hw may
>>>> only support 1. or 2. maybe explicitly document this and that
>>>> EOPNOTSUPP will be reported when the value is not supported
>>>> (vs EINVAL for plain invalid values) ?
>>>
>>> Would that not force a userspace applications to offer all possibilities to
>>> the user only to tell them that it's not supported?
>>> If the driver knows what is supported and what not it should make this
>>> discoverable without actually performing the operation.
>>>
>>> Maybe something along the lines of /sys/power/mem_sleep.
>>
>> Good point, but something like /sys/power/mem_sleep works
>> very differently then how all the other power_supply properties work.
> 
> Actually we already use this format in power-supply for USB
> types, implemented in power_supply_show_usb_type().
> 
>> In general if something is supported or not on a psy class
>> device is communicated by the presence / absence of attributes.
>>
>> So I think we should move back to having 2 separate attributes
>> for this after all; and group the 2 together in the doc and
>> document that enabling (setting to 1) one of force_charge /
>> inhibit_charge automatically clears the setting of the other.
>>
>> Then the availability of the features can simply be probed
>> by checking for the presence of the property files.
> 
> If it's two files, then somebody needs to come up with proper 
> names. Things like 'force_discharge' look sensible in this context,
> but on a system with two batteries (like some Thinkpads have) it
> is easy to confuse with "I want to discharge this battery before
> the other one (while no AC is connected)".

Ah I did not realize there was already some (read-only) precedence
for this in the psy subsystem.

Since there is precedence for this using
/sys/class/power_supply/<supply_name>/charge_behaviour

with an example contents of say:

[auto] inhibit-charge force-discharge

Works for me and having 1 file instead of 2 is better then
because this clearly encapsulates that inhibit-charge and
force-discharge are mutually exclusive.

Regards,

Hans

Thomas Koch Oct. 6, 2021, 7:24 p.m. UTC | #12

Hi,

On 06.10.21 19:47, Hans de Goede wrote:
> Hi,
>
> On 10/6/21 6:28 PM, Sebastian Reichel wrote:
>> Hi,
>>
>> On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
>>> On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
>>>> On 2021-10-06T10:10+0200, Hans de Goede wrote:
>>>>> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
>>>>>> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
>>>>>>> Right, force-discharge automatically implies charging is
>>>>>>> being inhibited, so putting this in one file makes sense.
>>>>>>>
>>>>>>> Any suggestion for the name of the file?
>>>>>>
>>>>>> Maybe like this?
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> What: /sys/class/power_supply/<supply_name>/charge_behaviour
>>>>>> Date: October 2021
>>>>>> Contact: [e-mail]
>>>>>> Description:
>>>>>>   Configure battery behaviour when a charger is being connected.
>>>>>>
>>>>>>   Access: Read, Write
>>>>>>
>>>>>>   Valid values:
>>>>>>
>>>>>>   0: auto / no override
>>>>>>      When charger is connected battery should be charged
>>>>>>   1: force idle
>>>>>>      When charger is connected the battery should neither be charged
>>>>>>      nor discharged.
>>>>>>   2: force discharge
>>>>>>      When charger is connected the battery should be discharged
>>>>>>      anyways.
>>>>>> ---------------------------------------------------------------------
>>>>>
>>>>> That looks good to me. Although I just realized that some hw may
>>>>> only support 1. or 2. maybe explicitly document this and that
>>>>> EOPNOTSUPP will be reported when the value is not supported
>>>>> (vs EINVAL for plain invalid values) ?
>>>>
>>>> Would that not force a userspace applications to offer all possibilities to
>>>> the user only to tell them that it's not supported?
>>>> If the driver knows what is supported and what not it should make this
>>>> discoverable without actually performing the operation.
>>>>
>>>> Maybe something along the lines of /sys/power/mem_sleep.
>>>
>>> Good point, but something like /sys/power/mem_sleep works
>>> very differently then how all the other power_supply properties work.
>>
>> Actually we already use this format in power-supply for USB
>> types, implemented in power_supply_show_usb_type().
>>
>>> In general if something is supported or not on a psy class
>>> device is communicated by the presence / absence of attributes.
>>>
>>> So I think we should move back to having 2 separate attributes
>>> for this after all; and group the 2 together in the doc and
>>> document that enabling (setting to 1) one of force_charge /
>>> inhibit_charge automatically clears the setting of the other.
>>>
>>> Then the availability of the features can simply be probed
>>> by checking for the presence of the property files.
>>
>> If it's two files, then somebody needs to come up with proper
>> names. Things like 'force_discharge' look sensible in this context,
>> but on a system with two batteries (like some Thinkpads have) it
>> is easy to confuse with "I want to discharge this battery before
>> the other one (while no AC is connected)". > Ah I did not realize there was already some (read-only) precedence
> for this in the psy subsystem.
>
> Since there is precedence for this using
> /sys/class/power_supply/<supply_name>/charge_behaviour
>
> with an example contents of say:
>
> [auto] inhibit-charge force-discharge
>
> Works for me and having 1 file instead of 2 is better then
> because this clearly encapsulates that inhibit-charge and
> force-discharge are mutually exclusive.
In fact they do not reset each other on ThinkPads. It's possible to

1. set force_discharge=1 -- discharging commences
2. set inhibit_charge=1 -- discharging continues, force_discharge remains 1
3. set force_discharge=0 -- battery does not charge, inhibit_charge
remains 1

>
> Regards,
>
> Hans
>
>

Freundliche Grüße / Kind regards,
Thomas Koch

Mail : [e-mail]
Web  : https://linrunner.de/tlp

Thomas Weißschuh Oct. 6, 2021, 9:39 p.m. UTC | #13

On 2021-10-06T21:24+0200, Thomas Koch wrote:
> Date: Wed, 6 Oct 2021 21:24:14 +0200
> From: Thomas Koch <[e-mail]>
> To: Hans de Goede <[e-mail]>, Sebastian Reichel
>  <[e-mail]>
> Cc: Thomas Weißschuh <[e-mail]>, Nicolò Piazzalunga
>  <[e-mail]>, [e-mail],
>  "[e-mail]"
>  <[e-mail]>, "[e-mail]"
>  <[e-mail]>
> Subject: Re: [RFC] add standardized attributes for force_discharge and
>  inhibit_charge
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
>  Thunderbird/78.13.0
> 
> Hi,
> 
> On 06.10.21 19:47, Hans de Goede wrote:
> > Hi,
> > 
> > On 10/6/21 6:28 PM, Sebastian Reichel wrote:
> > > Hi,
> > > 
> > > On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
> > > > On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
> > > > > On 2021-10-06T10:10+0200, Hans de Goede wrote:
> > > > > > On 10/6/21 12:06 AM, Sebastian Reichel wrote:
> > > > > > > On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
> > > > > > > > Right, force-discharge automatically implies charging is
> > > > > > > > being inhibited, so putting this in one file makes sense.
> > > > > > > > 
> > > > > > > > Any suggestion for the name of the file?
> > > > > > > 
> > > > > > > Maybe like this?
> > > > > > > 
> > > > > > > ---------------------------------------------------------------------
> > > > > > > What: /sys/class/power_supply/<supply_name>/charge_behaviour
> > > > > > > Date: October 2021
> > > > > > > Contact: [e-mail]
> > > > > > > Description:
> > > > > > >   Configure battery behaviour when a charger is being connected.
> > > > > > > 
> > > > > > >   Access: Read, Write
> > > > > > > 
> > > > > > >   Valid values:
> > > > > > > 
> > > > > > >   0: auto / no override
> > > > > > >      When charger is connected battery should be charged
> > > > > > >   1: force idle
> > > > > > >      When charger is connected the battery should neither be charged
> > > > > > >      nor discharged.
> > > > > > >   2: force discharge
> > > > > > >      When charger is connected the battery should be discharged
> > > > > > >      anyways.
> > > > > > > ---------------------------------------------------------------------
> > > > > > 
> > > > > > That looks good to me. Although I just realized that some hw may
> > > > > > only support 1. or 2. maybe explicitly document this and that
> > > > > > EOPNOTSUPP will be reported when the value is not supported
> > > > > > (vs EINVAL for plain invalid values) ?
> > > > > 
> > > > > Would that not force a userspace applications to offer all possibilities to
> > > > > the user only to tell them that it's not supported?
> > > > > If the driver knows what is supported and what not it should make this
> > > > > discoverable without actually performing the operation.
> > > > > 
> > > > > Maybe something along the lines of /sys/power/mem_sleep.
> > > > 
> > > > Good point, but something like /sys/power/mem_sleep works
> > > > very differently then how all the other power_supply properties work.
> > > 
> > > Actually we already use this format in power-supply for USB
> > > types, implemented in power_supply_show_usb_type().
> > > 
> > > > In general if something is supported or not on a psy class
> > > > device is communicated by the presence / absence of attributes.
> > > > 
> > > > So I think we should move back to having 2 separate attributes
> > > > for this after all; and group the 2 together in the doc and
> > > > document that enabling (setting to 1) one of force_charge /
> > > > inhibit_charge automatically clears the setting of the other.
> > > > 
> > > > Then the availability of the features can simply be probed
> > > > by checking for the presence of the property files.
> > > 
> > > If it's two files, then somebody needs to come up with proper
> > > names. Things like 'force_discharge' look sensible in this context,
> > > but on a system with two batteries (like some Thinkpads have) it
> > > is easy to confuse with "I want to discharge this battery before
> > > the other one (while no AC is connected)". > Ah I did not realize there was already some (read-only) precedence
> > for this in the psy subsystem.
> > 
> > Since there is precedence for this using
> > /sys/class/power_supply/<supply_name>/charge_behaviour
> > 
> > with an example contents of say:
> > 
> > [auto] inhibit-charge force-discharge
> > 
> > Works for me and having 1 file instead of 2 is better then
> > because this clearly encapsulates that inhibit-charge and
> > force-discharge are mutually exclusive.
> In fact they do not reset each other on ThinkPads. It's possible to
> 
> 1. set force_discharge=1 -- discharging commences
> 2. set inhibit_charge=1 -- discharging continues, force_discharge remains 1
> 3. set force_discharge=0 -- battery does not charge, inhibit_charge
> remains 1

But in the end there are only three states the user cares about, or?
(inhibit, force_discharge and normal)

So when selecting inhibit or force_discharge the driver itself can reset the
other option so the users do not have to care about the internal state of the
EC.

Thomas

Thomas Koch Oct. 7, 2021, 5:56 a.m. UTC | #14

Hi,

On 06.10.21 23:39, Thomas Weißschuh wrote:
> On 2021-10-06T21:24+0200, Thomas Koch wrote:
>> Date: Wed, 6 Oct 2021 21:24:14 +0200
>> From: Thomas Koch <[e-mail]>
>> To: Hans de Goede <[e-mail]>, Sebastian Reichel
>>   <[e-mail]>
>> Cc: Thomas Weißschuh <[e-mail]>, Nicolò Piazzalunga
>>   <[e-mail]>, [e-mail],
>>   "[e-mail]"
>>   <[e-mail]>, "[e-mail]"
>>   <[e-mail]>
>> Subject: Re: [RFC] add standardized attributes for force_discharge and
>>   inhibit_charge
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
>>   Thunderbird/78.13.0
>>
>> Hi,
>>
>> On 06.10.21 19:47, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 10/6/21 6:28 PM, Sebastian Reichel wrote:
>>>> Hi,
>>>>
>>>> On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
>>>>> On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
>>>>>> On 2021-10-06T10:10+0200, Hans de Goede wrote:
>>>>>>> On 10/6/21 12:06 AM, Sebastian Reichel wrote:
>>>>>>>> On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
>>>>>>>>> Right, force-discharge automatically implies charging is
>>>>>>>>> being inhibited, so putting this in one file makes sense.
>>>>>>>>>
>>>>>>>>> Any suggestion for the name of the file?
>>>>>>>>
>>>>>>>> Maybe like this?
>>>>>>>>
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>> What: /sys/class/power_supply/<supply_name>/charge_behaviour
>>>>>>>> Date: October 2021
>>>>>>>> Contact: [e-mail]
>>>>>>>> Description:
>>>>>>>>    Configure battery behaviour when a charger is being connected.
>>>>>>>>
>>>>>>>>    Access: Read, Write
>>>>>>>>
>>>>>>>>    Valid values:
>>>>>>>>
>>>>>>>>    0: auto / no override
>>>>>>>>       When charger is connected battery should be charged
>>>>>>>>    1: force idle
>>>>>>>>       When charger is connected the battery should neither be charged
>>>>>>>>       nor discharged.
>>>>>>>>    2: force discharge
>>>>>>>>       When charger is connected the battery should be discharged
>>>>>>>>       anyways.
>>>>>>>> ---------------------------------------------------------------------
>>>>>>>
>>>>>>> That looks good to me. Although I just realized that some hw may
>>>>>>> only support 1. or 2. maybe explicitly document this and that
>>>>>>> EOPNOTSUPP will be reported when the value is not supported
>>>>>>> (vs EINVAL for plain invalid values) ?
>>>>>>
>>>>>> Would that not force a userspace applications to offer all possibilities to
>>>>>> the user only to tell them that it's not supported?
>>>>>> If the driver knows what is supported and what not it should make this
>>>>>> discoverable without actually performing the operation.
>>>>>>
>>>>>> Maybe something along the lines of /sys/power/mem_sleep.
>>>>>
>>>>> Good point, but something like /sys/power/mem_sleep works
>>>>> very differently then how all the other power_supply properties work.
>>>>
>>>> Actually we already use this format in power-supply for USB
>>>> types, implemented in power_supply_show_usb_type().
>>>>
>>>>> In general if something is supported or not on a psy class
>>>>> device is communicated by the presence / absence of attributes.
>>>>>
>>>>> So I think we should move back to having 2 separate attributes
>>>>> for this after all; and group the 2 together in the doc and
>>>>> document that enabling (setting to 1) one of force_charge /
>>>>> inhibit_charge automatically clears the setting of the other.
>>>>>
>>>>> Then the availability of the features can simply be probed
>>>>> by checking for the presence of the property files.
>>>>
>>>> If it's two files, then somebody needs to come up with proper
>>>> names. Things like 'force_discharge' look sensible in this context,
>>>> but on a system with two batteries (like some Thinkpads have) it
>>>> is easy to confuse with "I want to discharge this battery before
>>>> the other one (while no AC is connected)". > Ah I did not realize there was already some (read-only) precedence
>>> for this in the psy subsystem.
>>>
>>> Since there is precedence for this using
>>> /sys/class/power_supply/<supply_name>/charge_behaviour
>>>
>>> with an example contents of say:
>>>
>>> [auto] inhibit-charge force-discharge
>>>
>>> Works for me and having 1 file instead of 2 is better then
>>> because this clearly encapsulates that inhibit-charge and
>>> force-discharge are mutually exclusive.
>> In fact they do not reset each other on ThinkPads. It's possible to
>>
>> 1. set force_discharge=1 -- discharging commences
>> 2. set inhibit_charge=1 -- discharging continues, force_discharge remains 1
>> 3. set force_discharge=0 -- battery does not charge, inhibit_charge
>> remains 1
>
> But in the end there are only three states the user cares about, or?
> (inhibit, force_discharge and normal)
>
> So when selecting inhibit or force_discharge the driver itself can reset the
> other option so the users do not have to care about the internal state of the
> EC.
Correct. It works with the three states Sebastian suggested because
force_discharge overrules inhibit_charge.

Whereby for user-friendliness I would prefer Hans' strings

 >>> [auto] inhibit-charge force-discharge


to Sebastian's numbered states.

>
> Thomas
>

Freundliche Grüße / Kind regards,
Thomas Koch

Mail : [e-mail]
Web  : https://linrunner.de/tlp

Sebastian Reichel Oct. 7, 2021, 11:28 a.m. UTC | #15

Hi,

On Thu, Oct 07, 2021 at 07:56:13AM +0200, Thomas Koch wrote:
> > > > > On Wed, Oct 06, 2021 at 05:27:22PM +0200, Hans de Goede wrote:
> > > > > > On 10/6/21 4:49 PM, Thomas Weißschuh wrote:
> > > > > > > On 2021-10-06T10:10+0200, Hans de Goede wrote:
> > > > > > > > On 10/6/21 12:06 AM, Sebastian Reichel wrote:Ack
> > > > > > > > > On Tue, Oct 05, 2021 at 08:01:12PM +0200, Hans de Goede wrote:
> > > > > > > > > > Right, force-discharge automatically implies charging is
> > > > > > > > > > being inhibited, so putting this in one file makes sense.
> > > > > > > > > > 
> > > > > > > > > > Any suggestion for the name of the file?
> > > > > > > > > 
> > > > > > > > > Maybe like this?
> > > > > > > > > 
> > > > > > > > > ---------------------------------------------------------------------
> > > > > > > > > What: /sys/class/power_supply/<supply_name>/charge_behaviour
> > > > > > > > > Date: October 2021
> > > > > > > > > Contact: [e-mail]
> > > > > > > > > Description:
> > > > > > > > >    Configure battery behaviour when a charger is being connected.
> > > > > > > > > 
> > > > > > > > >    Access: Read, Write
> > > > > > > > > 
> > > > > > > > >    Valid values:
> > > > > > > > > 
> > > > > > > > >    0: auto / no override
> > > > > > > > >       When charger is connected battery should be charged
> > > > > > > > >    1: force idle
> > > > > > > > >       When charger is connected the battery should neither be charged
> > > > > > > > >       nor discharged.
> > > > > > > > >    2: force discharge
> > > > > > > > >       When charger is connected the battery should be discharged
> > > > > > > > >       anyways.
> > > > > > > > > ---------------------------------------------------------------------
> > > > > > > > 
> > > > > > > > That looks good to me. Although I just realized that some hw may
> > > > > > > > only support 1. or 2. maybe explicitly document this and that
> > > > > > > > EOPNOTSUPP will be reported when the value is not supported
> > > > > > > > (vs EINVAL for plain invalid values) ?
> > > > > > > 
> > > > > > > Would that not force a userspace applications to offer all possibilities to
> > > > > > > the user only to tell them that it's not supported?
> > > > > > > If the driver knows what is supported and what not it should make this
> > > > > > > discoverable without actually performing the operation.
> > > > > > > 
> > > > > > > Maybe something along the lines of /sys/power/mem_sleep.
> > > > > > 
> > > > > > Good point, but something like /sys/power/mem_sleep works
> > > > > > very differently then how all the other power_supply properties work.
> > > > > 
> > > > > Actually we already use this format in power-supply for USB
> > > > > types, implemented in power_supply_show_usb_type().
> > > > > 
> > > > > > In general if something is supported or not on a psy class
> > > > > > device is communicated by the presence / absence of attributes.
> > > > > > 
> > > > > > So I think we should move back to having 2 separate attributes
> > > > > > for this after all; and group the 2 together in the doc and
> > > > > > document that enabling (setting to 1) one of force_charge /
> > > > > > inhibit_charge automatically clears the setting of the other.
> > > > > > 
> > > > > > Then the availability of the features can simply be probed
> > > > > > by checking for the presence of the property files.
> > > > > 
> > > > > If it's two files, then somebody needs to come up with proper
> > > > > names. Things like 'force_discharge' look sensible in this context,
> > > > > but on a system with two batteries (like some Thinkpads have) it
> > > > > is easy to confuse with "I want to discharge this battery before
> > > > > the other one (while no AC is connected)". > Ah I did not realize there was already some (read-only) precedence
> > > > for this in the psy subsystem.
> > > > 
> > > > Since there is precedence for this using
> > > > /sys/class/power_supply/<supply_name>/charge_behaviour
> > > > 
> > > > with an example contents of say:
> > > > 
> > > > [auto] inhibit-charge force-discharge
> > > > 
> > > > Works for me and having 1 file instead of 2 is better then
> > > > because this clearly encapsulates that inhibit-charge and
> > > > force-discharge are mutually exclusive.
> > > In fact they do not reset each other on ThinkPads. It's possible to
> > > 
> > > 1. set force_discharge=1 -- discharging commences
> > > 2. set inhibit_charge=1 -- discharging continues, force_discharge remains 1
> > > 3. set force_discharge=0 -- battery does not charge, inhibit_charge
> > > remains 1
> > 
> > But in the end there are only three states the user cares about, or?
> > (inhibit, force_discharge and normal)
> > 
> > So when selecting inhibit or force_discharge the driver itself can reset the
> > other option so the users do not have to care about the internal state of the
> > EC.
> Correct. It works with the three states Sebastian suggested because
> force_discharge overrules inhibit_charge.
> 
> Whereby for user-friendliness I would prefer Hans' strings
> 
> >>> [auto] inhibit-charge force-discharge
> 
> to Sebastian's numbered states.

LGTM.

-- Sebastian

Patch

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:	[e-mail]
+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
+
+What:		/sys/class/power_supply/<supply_name>/inhibit_charge
+Date:		September 2021
+Contact:	[e-mail]
+Description:
+		Represents the presence of a manual override over the threshold
+		attributes of the battery, thus inhibiting battery charge.
+
+		Access: Read, Write
+
+		Valid values:
+			== ======================
+			1: Stop charging
+			0: Terminate the override
+
 What:		/sys/class/power_supply/<supply_name>/technology
 Date:		May 2007
 Contact:	[e-mail]
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,