Bug 217062

Summary: Unnecessary battery is exported for Wacom panel
Product: Drivers Reporter: Mario Limonciello (AMD) (mario.limonciello)
Component: Input DevicesAssignee: drivers_input-devices
Status: RESOLVED CODE_FIX    
Severity: normal CC: bugzilla, killertofu, pingc
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 6.2 Subsystem:
Regression: No Bisected commit-id:

Description Mario Limonciello (AMD) 2023-02-20 15:38:08 UTC 
On a Lenovo Z13, I find that an extra battery device is exported by the kernel.

P: /devices/platform/AMDI0010:01/i2c-1/i2c-WACF2200:00/0018:056A:52E8.0002/power_supply/wacom_battery_0
L: 0
E: DEVPATH=/devices/platform/AMDI0010:01/i2c-1/i2c-WACF2200:00/0018:056A:52E8.0002/power_supply/wacom_battery_0
E: SUBSYSTEM=power_supply
E: POWER_SUPPLY_NAME=wacom_battery_0
E: POWER_SUPPLY_TYPE=Battery
E: POWER_SUPPLY_MODEL_NAME=Wacom HID 52E8
E: POWER_SUPPLY_PRESENT=0
E: POWER_SUPPLY_STATUS=Unknown
E: POWER_SUPPLY_SCOPE=Device
E: POWER_SUPPLY_CAPACITY=0

Presumably this is for an optional stylus?

Anyway userspace picks this up and shows it in the GNOME GUI.  I had proposed to hide it because the "present" attribute is "0", but they said that the wacom driver overloaded the present attribute to not mean "battery present" but instead "device present".  As the device isn't actually present, the proposal was to not register the battery until the device is present.

https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/1683#note_1678218

Can you please do this?
Comment 1 Jason Gerecke 2023-02-20 16:56:24 UTC 
> Presumably this is for an optional stylus?
Yes, almost certainly. This appears to be a sensor for a (battery-powered) AES pen.

> [...] they said that the wacom driver overloaded the present attribute to not
> mean "battery present" but instead "device present".
The driver /does/ use the "present" attribute to mean "battery present", but that attribute alone has no way to communicate an "unknown" value.

We can only know the state of a stylus battery if it is within proximity of the sensor (a couple cm at most). When the stylus is outside of this range, /anything/ could be happening with the battery (maybe its charging? maybe its not? maybe the user doesn't even /have/ a stylus, or just lent it to someone else for the day) and we have no way to know.

To communicate this lack of knowledge to userspace we set POWER_SUPPLY_STATUS=Unknown whenever the stylus is outside of proximity. When a pen is brought close enough for us to read its battery state we'll update the properties accordingly and set POWER_SUPPLY_STATUS=Discharging. Once the pen leaves proximity, we change POWER_SUPPLY_STATUS=Unknown again.

In my opinion, userspace should be ignoring (or maybe just greying out) the device if POWER_SUPPLY_STATUS=Unknown. But maybe we're misunderstanding what that flag is intended to mean?

> As the device isn't actually present, the proposal was to not register the
> battery until the device is present.
I don't really like this as a solution, though I do understand where its coming from. 'The "power supply" is in the stylus so why should there be a power_supply device registered when there's no stylus?' Makes sense. What doesn't make as much sense is what to do about the power_supply device after the pen leaves proximity. Do we just leave it in place until the next reboot? Do we set a timer in the kernel to remove it after an arbitrary amount of time? Do we remove it every time the pen leaves proximity? Is there a lighter-weight alternative to creating/removing power_supply devices to let userspace know that the device being powered isn't available right now (e.g. POWER_SUPPLY_STATUS=Unknown or similar)?
Comment 2 Mario Limonciello (AMD) 2023-02-20 17:01:48 UTC 
Thanks for the feedback!

> within proximity of the sensor (a couple cm at most)

Well so in practice you'll only be able to get useful data (at least from GNOME) if you open up the GNOME control center battery panel and then move it close to the screen.  I'd say 99% of the time this has no sense in being shown in GNOME.

> In my opinion, userspace should be ignoring (or maybe just greying out) the
> device if POWER_SUPPLY_STATUS=Unknown. But maybe we're misunderstanding what
> that flag is intended to mean?

I personally think this sounds reasonable.

> What doesn't make as much sense is what to do about the power_supply device
> after the pen leaves proximity. Do we just leave it in place until the next
> reboot? Do we set a timer in the kernel to remove it after an arbitrary
> amount of time? Do we remove it every time the pen leaves proximity? Is there
> a lighter-weight alternative to creating/removing power_supply devices to let
> userspace know that the device being powered isn't available right now (e.g.
> POWER_SUPPLY_STATUS=Unknown or similar)?

Given it's only available when the device is super close to the screen I tend to agree it seems expensive to create/remove devices constantly.


@Bastien what do you think?
Comment 3 Jason Gerecke 2023-02-20 17:45:33 UTC 
>> within proximity of the sensor (a couple cm at most)
>
> Well so in practice you'll only be able to get useful data (at least from
> GNOME) if you open up the GNOME control center battery panel and then move it
> close to the screen.  I'd say 99% of the time this has no sense in being
> shown in GNOME.

This might actually be a good argument for a larger change to how the driver reports the battery status from AES pens. I can imagine users wanting to see the battery level for the pen they have "recently" used, even if it isn't in prox at that exact moment. Having the driver report non-Unknown (but potentially stale) data for a short time (few minutes?) would achieve this. It would also probably somewhat-improve the situation for users who have complained that GNOME pops up a fresh low-battery warning every single time their pen enters proximity... (I'm not sure if that's still an issue or if GNOME did something about it...)
Comment 4 Mario Limonciello (AMD) 2023-02-20 17:48:09 UTC 
If you make such a change, presumably you can also put off reporting the existence of a battery until the first time a pen connects, right?

That would completely avoid the GNOME issue as I reproduced it.
Comment 5 Bastien Nocera 2023-02-21 09:16:32 UTC 
(In reply to Mario Limonciello (AMD) from comment #2)
> Thanks for the feedback!
> 
> > within proximity of the sensor (a couple cm at most)
> 
> Well so in practice you'll only be able to get useful data (at least from
> GNOME) if you open up the GNOME control center battery panel and then move
> it close to the screen.  I'd say 99% of the time this has no sense in being
> shown in GNOME.
> 
> > In my opinion, userspace should be ignoring (or maybe just greying out) the
> > device if POWER_SUPPLY_STATUS=Unknown. But maybe we're misunderstanding
> what
> > that flag is intended to mean?
> 
> I personally think this sounds reasonable.
> 
> > What doesn't make as much sense is what to do about the power_supply device
> > after the pen leaves proximity. Do we just leave it in place until the next
> > reboot? Do we set a timer in the kernel to remove it after an arbitrary
> > amount of time? Do we remove it every time the pen leaves proximity? Is
> there
> > a lighter-weight alternative to creating/removing power_supply devices to
> let
> > userspace know that the device being powered isn't available right now
> (e.g.
> > POWER_SUPPLY_STATUS=Unknown or similar)?
> 
> Given it's only available when the device is super close to the screen I
> tend to agree it seems expensive to create/remove devices constantly.
> 
> 
> @Bastien what do you think?

I don't really mind how you encode it, but it needs to be documented in the power_supply ABI docs before it's used in that way. POWER_SUPPLY_STATUS=Unknown isn't good enough though, as that might be the status of a device with, say, AA batteries where the battery voltage takes a while to settle. A quick grep POWER_SUPPLY_STATUS_UNKNOWN confirms that it would be a bad idea to use that to mean "device isn't around".

As I've mentioned before, you're probably looking at a new attribute that will need to be documented before it's used, so everyone is on the same page.
Comment 6 Mario Limonciello (AMD) 2023-02-21 14:45:57 UTC 
What about the combination of POWER_SUPPLY_STATUS=Unknown and POWER_SUPPLY_PRESENT=0 both?

Could that cover this case well enough for now or do you still think a new property is needed?
Comment 7 Bastien Nocera 2023-02-23 09:48:20 UTC 
I don't think it's good enough. It probably happens in some corner cases that don't match this one, it's undocumented and really, it's not related to the power_supply/battery itself.

Thinking about it, it would be better for the parent device (the input device) to have a sysfs attribute for the proximity that user-space could consume without having to listen to input events themselves.

Do you want to send a mail to linux-input to discuss it?
Comment 8 Mario Limonciello (AMD) 2023-02-23 15:32:10 UTC 
> Do you want to send a mail to linux-input to discuss it?

https://lore.kernel.org/linux-input/MN0PR12MB6101F4408960BDB9CF63DD53E2AB9@MN0PR12MB6101.namprd12.prod.outlook.com/T/#u