POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are
used to charge the machine itself (so a "system" scope), like the
single USB port on a phone, rather than devices.
The wacom_sys driver is the only driver that sets its device battery as
being a USB type, which doesn't seem correct based on its usage, so
switch it to be a battery type like all the other USB-connected devices.
Signed-off-by: Bastien Nocera <[email protected]>
---
drivers/hid/wacom_sys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 066c567dbaa2..620fe74f5676 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom,
bat_desc->get_property = wacom_battery_get_property;
sprintf(battery->bat_name, "wacom_battery_%ld", n);
bat_desc->name = battery->bat_name;
- bat_desc->type = POWER_SUPPLY_TYPE_USB;
+ bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
bat_desc->use_for_apm = 0;
ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
--
2.35.1
It seems that the USB type was chosen to fit into a upower heuristic
that still exists (see [1], [2]). Looking at the upower code I suspect
that swapping to the Battery type will at least cause
"UP_DEVICE_KIND_TABLET" to no longer be used for our dongle-based
wireless devices (Bluetooth-based might still be fine though). We
haven't sold dongle-based devices in a while, but they're definitely
still out there. If the batteries in those devices are seen as system
batteries that could cause a problem -- e.g. triggering hibernation
when the tablet battery gets low.
I think it would be wise to test this first to see if there's any
obvious real-world fallout from the change...
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96983296281507f049425b84f0d244c40d506eba
[2]: https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
On Thu, Apr 7, 2022 at 1:52 PM Bastien Nocera <[email protected]> wrote:
>
> POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are
> used to charge the machine itself (so a "system" scope), like the
> single USB port on a phone, rather than devices.
>
> The wacom_sys driver is the only driver that sets its device battery as
> being a USB type, which doesn't seem correct based on its usage, so
> switch it to be a battery type like all the other USB-connected devices.
>
> Signed-off-by: Bastien Nocera <[email protected]>
> ---
> drivers/hid/wacom_sys.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 066c567dbaa2..620fe74f5676 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom,
> bat_desc->get_property = wacom_battery_get_property;
> sprintf(battery->bat_name, "wacom_battery_%ld", n);
> bat_desc->name = battery->bat_name;
> - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> bat_desc->use_for_apm = 0;
>
> ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
> --
> 2.35.1
>
On Thu, 7 Apr 2022, Bastien Nocera wrote:
> POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are
> used to charge the machine itself (so a "system" scope), like the
> single USB port on a phone, rather than devices.
>
> The wacom_sys driver is the only driver that sets its device battery as
> being a USB type, which doesn't seem correct based on its usage, so
> switch it to be a battery type like all the other USB-connected devices.
>
> Signed-off-by: Bastien Nocera <[email protected]>
> ---
> drivers/hid/wacom_sys.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 066c567dbaa2..620fe74f5676 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom,
> bat_desc->get_property = wacom_battery_get_property;
> sprintf(battery->bat_name, "wacom_battery_%ld", n);
> bat_desc->name = battery->bat_name;
> - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> bat_desc->use_for_apm = 0;
>
> ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver
maintainers) to get their Ack.
--
Jiri Kosina
SUSE Labs
Following up on my previous comment. I've been able to test this patch
with both flavors of wireless interface. Both Bluetooth (Intuos Pro)
and dongle-based (Intuos5) appear to have mostly-correct behavior
while charging and discharging, even when the battery level gradually
drops to zero. The misbehaviors I see appear to be limited to upower
mis-categorizing the devices as an e.g. keyboard or generic battery
rather than as a tablet. This leads to some slightly confusing UI
issues (e.g. GNOME and KDE referring to the device incorrectly), but
nothing too annoying. If upower is taught to recognize tablets under
more circumstances those issues should disappear.
Ping tells me you may have an Intuos4 Wireless, Bastien? Any
additional testing you can do with that device would be appreciated,
though even without it I'm personally comfortable enough to provide an
ack:
Acked-by: Jason Gerecke <[email protected]>
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
On Tue, Apr 12, 2022 at 1:53 AM Jiri Kosina <[email protected]> wrote:
>
> On Thu, 7 Apr 2022, Bastien Nocera wrote:
>
> > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that are
> > used to charge the machine itself (so a "system" scope), like the
> > single USB port on a phone, rather than devices.
> >
> > The wacom_sys driver is the only driver that sets its device battery as
> > being a USB type, which doesn't seem correct based on its usage, so
> > switch it to be a battery type like all the other USB-connected devices.
> >
> > Signed-off-by: Bastien Nocera <[email protected]>
> > ---
> > drivers/hid/wacom_sys.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 066c567dbaa2..620fe74f5676 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct wacom *wacom,
> > bat_desc->get_property = wacom_battery_get_property;
> > sprintf(battery->bat_name, "wacom_battery_%ld", n);
> > bat_desc->name = battery->bat_name;
> > - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > bat_desc->use_for_apm = 0;
> >
> > ps_bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
>
> Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver
> maintainers) to get their Ack.
>
> --
> Jiri Kosina
> SUSE Labs
>
On Mon, 2022-04-11 at 17:08 +0200, Jiri Kosina wrote:
> On Thu, 7 Apr 2022, Bastien Nocera wrote:
>
> > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that
> > are
> > used to charge the machine itself (so a "system" scope), like the
> > single USB port on a phone, rather than devices.
> >
> > The wacom_sys driver is the only driver that sets its device
> > battery as
> > being a USB type, which doesn't seem correct based on its usage, so
> > switch it to be a battery type like all the other USB-connected
> > devices.
> >
> > Signed-off-by: Bastien Nocera <[email protected]>
> > ---
> > drivers/hid/wacom_sys.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 066c567dbaa2..620fe74f5676 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct
> > wacom *wacom,
> > bat_desc->get_property = wacom_battery_get_property;
> > sprintf(battery->bat_name, "wacom_battery_%ld", n);
> > bat_desc->name = battery->bat_name;
> > - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > bat_desc->use_for_apm = 0;
> >
> > ps_bat = devm_power_supply_register(dev, bat_desc,
> > &psy_cfg);
>
> Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver
> maintainers) to get their Ack.
Hey,
I know both Jason and Ping, but their name didn't show up when running
get_maintainers.pl and neither of their names are in the MAINTAINERS
file.
This probably needs fixing.
Cheers
On Wed, Apr 20, 2022 at 1:52 AM Bastien Nocera <[email protected]> wrote:
>
> On Wed, 2022-04-13 at 07:59 -0700, Jason Gerecke wrote:
> > Following up on my previous comment. I've been able to test this
> > patch
> > with both flavors of wireless interface. Both Bluetooth (Intuos Pro)
> > and dongle-based (Intuos5) appear to have mostly-correct behavior
> > while charging and discharging, even when the battery level gradually
> > drops to zero. The misbehaviors I see appear to be limited to upower
> > mis-categorizing the devices as an e.g. keyboard or generic battery
> > rather than as a tablet. This leads to some slightly confusing UI
> > issues (e.g. GNOME and KDE referring to the device incorrectly), but
> > nothing too annoying. If upower is taught to recognize tablets under
> > more circumstances those issues should disappear.
> >
> > Ping tells me you may have an Intuos4 Wireless, Bastien? Any
> > additional testing you can do with that device would be appreciated,
> > though even without it I'm personally comfortable enough to provide
> > an
> > ack:
> >
> > Acked-by: Jason Gerecke <[email protected]>
>
> Only devices I have is the original Wacom Graphire Bluetooth, and a
> wired Intuos4 (PTK-640) which I think changed names not long after.
>
> This might be enough:
> https://gitlab.freedesktop.org/upower/upower/-/merge_requests/127
>
> Please file an issue with the info discussed in the previous mail if it
> isn't.
>
Thanks Bastien, it seems to do the trick for Bluetooth devices but
needs a little more work for the dongle-based Intuos5. I've filed a
new issue with some suggestions as requested:
https://gitlab.freedesktop.org/upower/upower/-/issues/182
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
> Cheers
>
> >
> > Jason
> > ---
> > Now instead of four in the eights place /
> > you’ve got three, ‘Cause you added one /
> > (That is to say, eight) to the two, /
> > But you can’t take seven from three, /
> > So you look at the sixty-fours....
> >
> > On Tue, Apr 12, 2022 at 1:53 AM Jiri Kosina <[email protected]> wrote:
> > >
> > > On Thu, 7 Apr 2022, Bastien Nocera wrote:
> > >
> > > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports
> > > > that are
> > > > used to charge the machine itself (so a "system" scope), like the
> > > > single USB port on a phone, rather than devices.
> > > >
> > > > The wacom_sys driver is the only driver that sets its device
> > > > battery as
> > > > being a USB type, which doesn't seem correct based on its usage,
> > > > so
> > > > switch it to be a battery type like all the other USB-connected
> > > > devices.
> > > >
> > > > Signed-off-by: Bastien Nocera <[email protected]>
> > > > ---
> > > > drivers/hid/wacom_sys.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > > > index 066c567dbaa2..620fe74f5676 100644
> > > > --- a/drivers/hid/wacom_sys.c
> > > > +++ b/drivers/hid/wacom_sys.c
> > > > @@ -1777,7 +1777,7 @@ static int
> > > > __wacom_initialize_battery(struct wacom *wacom,
> > > > bat_desc->get_property = wacom_battery_get_property;
> > > > sprintf(battery->bat_name, "wacom_battery_%ld", n);
> > > > bat_desc->name = battery->bat_name;
> > > > - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> > > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > > > bat_desc->use_for_apm = 0;
> > > >
> > > > ps_bat = devm_power_supply_register(dev, bat_desc,
> > > > &psy_cfg);
> > >
> > > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver
> > > maintainers) to get their Ack.
> > >
> > > --
> > > Jiri Kosina
> > > SUSE Labs
> > >
>
On Wed, 2022-04-13 at 07:59 -0700, Jason Gerecke wrote:
> Following up on my previous comment. I've been able to test this
> patch
> with both flavors of wireless interface. Both Bluetooth (Intuos Pro)
> and dongle-based (Intuos5) appear to have mostly-correct behavior
> while charging and discharging, even when the battery level gradually
> drops to zero. The misbehaviors I see appear to be limited to upower
> mis-categorizing the devices as an e.g. keyboard or generic battery
> rather than as a tablet. This leads to some slightly confusing UI
> issues (e.g. GNOME and KDE referring to the device incorrectly), but
> nothing too annoying. If upower is taught to recognize tablets under
> more circumstances those issues should disappear.
>
> Ping tells me you may have an Intuos4 Wireless, Bastien? Any
> additional testing you can do with that device would be appreciated,
> though even without it I'm personally comfortable enough to provide
> an
> ack:
>
> Acked-by: Jason Gerecke <[email protected]>
Only devices I have is the original Wacom Graphire Bluetooth, and a
wired Intuos4 (PTK-640) which I think changed names not long after.
This might be enough:
https://gitlab.freedesktop.org/upower/upower/-/merge_requests/127
Please file an issue with the info discussed in the previous mail if it
isn't.
Cheers
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
> On Tue, Apr 12, 2022 at 1:53 AM Jiri Kosina <[email protected]> wrote:
> >
> > On Thu, 7 Apr 2022, Bastien Nocera wrote:
> >
> > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports
> > > that are
> > > used to charge the machine itself (so a "system" scope), like the
> > > single USB port on a phone, rather than devices.
> > >
> > > The wacom_sys driver is the only driver that sets its device
> > > battery as
> > > being a USB type, which doesn't seem correct based on its usage,
> > > so
> > > switch it to be a battery type like all the other USB-connected
> > > devices.
> > >
> > > Signed-off-by: Bastien Nocera <[email protected]>
> > > ---
> > > drivers/hid/wacom_sys.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > > index 066c567dbaa2..620fe74f5676 100644
> > > --- a/drivers/hid/wacom_sys.c
> > > +++ b/drivers/hid/wacom_sys.c
> > > @@ -1777,7 +1777,7 @@ static int
> > > __wacom_initialize_battery(struct wacom *wacom,
> > > bat_desc->get_property = wacom_battery_get_property;
> > > sprintf(battery->bat_name, "wacom_battery_%ld", n);
> > > bat_desc->name = battery->bat_name;
> > > - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > > bat_desc->use_for_apm = 0;
> > >
> > > ps_bat = devm_power_supply_register(dev, bat_desc,
> > > &psy_cfg);
> >
> > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver
> > maintainers) to get their Ack.
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >
On Wed, 20 Apr 2022, Bastien Nocera wrote:
> > > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that
> > > are
> > > used to charge the machine itself (so a "system" scope), like the
> > > single USB port on a phone, rather than devices.
> > >
> > > The wacom_sys driver is the only driver that sets its device
> > > battery as
> > > being a USB type, which doesn't seem correct based on its usage, so
> > > switch it to be a battery type like all the other USB-connected
> > > devices.
> > >
> > > Signed-off-by: Bastien Nocera <[email protected]>
> > > ---
> > > drivers/hid/wacom_sys.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > > index 066c567dbaa2..620fe74f5676 100644
> > > --- a/drivers/hid/wacom_sys.c
> > > +++ b/drivers/hid/wacom_sys.c
> > > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct
> > > wacom *wacom,
> > > bat_desc->get_property = wacom_battery_get_property;
> > > sprintf(battery->bat_name, "wacom_battery_%ld", n);
> > > bat_desc->name = battery->bat_name;
> > > - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> > > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > > bat_desc->use_for_apm = 0;
> > >
> > > ps_bat = devm_power_supply_register(dev, bat_desc,
> > > &psy_cfg);
> >
> > Thanks Bastien, makes sense. CCing Jason and Ping (the Wacom driver
> > maintainers) to get their Ack.
>
> Hey,
>
> I know both Jason and Ping, but their name didn't show up when running
> get_maintainers.pl and neither of their names are in the MAINTAINERS
> file.
>
> This probably needs fixing.
Agreed. Wacom driver is by itself substantial enough in order to have
MAINTAINERS entry.
Jason, Ping, could you please send me a patch adding yourself as
maintainers?
Thanks,
--
Jiri Kosina
SUSE Labs
On Wed, 13 Apr 2022, Jason Gerecke wrote:
> Following up on my previous comment. I've been able to test this patch
> with both flavors of wireless interface. Both Bluetooth (Intuos Pro)
> and dongle-based (Intuos5) appear to have mostly-correct behavior
> while charging and discharging, even when the battery level gradually
> drops to zero. The misbehaviors I see appear to be limited to upower
> mis-categorizing the devices as an e.g. keyboard or generic battery
> rather than as a tablet. This leads to some slightly confusing UI
> issues (e.g. GNOME and KDE referring to the device incorrectly), but
> nothing too annoying. If upower is taught to recognize tablets under
> more circumstances those issues should disappear.
>
> Ping tells me you may have an Intuos4 Wireless, Bastien? Any
> additional testing you can do with that device would be appreciated,
> though even without it I'm personally comfortable enough to provide an
> ack:
>
> Acked-by: Jason Gerecke <[email protected]>
Thanks Jason, now applied.
--
Jiri Kosina
SUSE Labs
On Mon, 2022-04-11 at 11:51 -0700, Jason Gerecke wrote:
> It seems that the USB type was chosen to fit into a upower heuristic
> that still exists (see [1], [2]).
This heuristic was introduced in a commit that reads:
"
Hardcode wacom battery devices as not power-supply devices
We'll switch to a kernel property when the power_supply interface is
fixed.
"
Might be time :)
> Looking at the upower code I suspect
> that swapping to the Battery type will at least cause
> "UP_DEVICE_KIND_TABLET" to no longer be used for our dongle-based
> wireless devices (Bluetooth-based might still be fine though). We
> haven't sold dongle-based devices in a while, but they're definitely
> still out there. If the batteries in those devices are seen as system
> batteries that could cause a problem -- e.g. triggering hibernation
> when the tablet battery gets low.
Whatever the type of "power_supply", the device will never be detected
as supplying the system, as the "scope" is correct.
> I think it would be wise to test this first to see if there's any
> obvious real-world fallout from the change...
Worse case scenario, the tablet is detected as something other than a
tablet. If that happens, please file a bug against upower and attach
the output of "udevadm info --export-db" and CC: me on the issue, and
I'll fix the detection.
Cheers
>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=96983296281507f049425b84f0d244c40d506eba
> [2]:
> https://cgit.freedesktop.org/upower/tree/src/linux/up-device-supply.c
>
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
>
>
>
> On Thu, Apr 7, 2022 at 1:52 PM Bastien Nocera <[email protected]>
> wrote:
> >
> > POWER_SUPPLY_TYPE_USB seems to only ever be used by USB ports that
> > are
> > used to charge the machine itself (so a "system" scope), like the
> > single USB port on a phone, rather than devices.
> >
> > The wacom_sys driver is the only driver that sets its device
> > battery as
> > being a USB type, which doesn't seem correct based on its usage, so
> > switch it to be a battery type like all the other USB-connected
> > devices.
> >
> > Signed-off-by: Bastien Nocera <[email protected]>
> > ---
> > drivers/hid/wacom_sys.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 066c567dbaa2..620fe74f5676 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -1777,7 +1777,7 @@ static int __wacom_initialize_battery(struct
> > wacom *wacom,
> > bat_desc->get_property = wacom_battery_get_property;
> > sprintf(battery->bat_name, "wacom_battery_%ld", n);
> > bat_desc->name = battery->bat_name;
> > - bat_desc->type = POWER_SUPPLY_TYPE_USB;
> > + bat_desc->type = POWER_SUPPLY_TYPE_BATTERY;
> > bat_desc->use_for_apm = 0;
> >
> > ps_bat = devm_power_supply_register(dev, bat_desc,
> > &psy_cfg);
> > --
> > 2.35.1
> >