* Re: [PATCH 4/5] USB: core: Add API to change the wireless_status
From: Bastien Nocera @ 2023-02-23 16:17 UTC (permalink / raw)
To: Alan Stern
Cc: linux-usb, linux-input, Greg Kroah-Hartman, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <Y/eJDDPXJfYgfdfI@rowland.harvard.edu>
On Thu, 2023-02-23 at 10:41 -0500, Alan Stern wrote:
> On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > Allow device specific drivers to change the wireless status of a
> > device.
> > This will allow user-space to know whether the device is available,
> > whether or not specific USB interfaces can detect it.
> >
> > This can be used by wireless headsets with USB receivers to
> > propagate to
> > user-space whether or not the headset is turned on, so as to
> > consider it
> > as unavailable, and not switch to it just because the receiver is
> > plugged in.
> >
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> > drivers/usb/core/message.c | 13 +++++++++++++
> > drivers/usb/core/usb.c | 24 ++++++++++++++++++++++++
> > include/linux/usb.h | 4 ++++
> > 3 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/usb/core/message.c
> > b/drivers/usb/core/message.c
> > index 127fac1af676..d5c7749d515e 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1908,6 +1908,18 @@ static void __usb_queue_reset_device(struct
> > work_struct *ws)
> > usb_put_intf(iface); /* Undo _get_ in
> > usb_queue_reset_device() */
> > }
> >
> > +/*
> > + * Internal function to set the wireless_status sysfs attribute
> > + * See usb_set_wireless_status() for more details
> > + */
> > +static void __usb_wireless_status_intf(struct work_struct *ws)
> > +{
> > + struct usb_interface *iface =
> > + container_of(ws, struct usb_interface,
> > wireless_status_work);
> > +
> > + usb_update_wireless_status_attr(iface);
> > + usb_put_intf(iface); /* Undo _get_ in
> > usb_set_wireless_status() */
> > +}
>
> Have you thought about what will happen if this routine ends up
> running
> after the interface has been deleted?
I believe that usb_release_interface() will only be called once the
last reference to the interface is dropped, so bar any refcounting
bugs, the interface should always exist when this function is called.
> > /*
> > * usb_set_configuration - Makes a particular device setting be
> > current
> > @@ -2100,6 +2112,7 @@ int usb_set_configuration(struct usb_device
> > *dev, int configuration)
> > intf->dev.type = &usb_if_device_type;
> > intf->dev.groups = usb_interface_groups;
> > INIT_WORK(&intf->reset_ws,
> > __usb_queue_reset_device);
> > + INIT_WORK(&intf->wireless_status_work,
> > __usb_wireless_status_intf);
> > intf->minor = -1;
> > device_initialize(&intf->dev);
> > pm_runtime_no_callbacks(&intf->dev);
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 11b15d7b357a..5f42c5b9d209 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct
> > usb_device *dev)
> > }
> > EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
> >
> > +/**
> > + * usb_set_wireless_status - sets the wireless_status struct
> > member
> > + * @dev: the device to modify
> > + * @status: the new wireless status
> > + *
> > + * Set the wireless_status struct member to the new value, and
> > emit
> > + * sysfs changes as necessary.
> > + *
> > + * Returns: 0 on success, -EALREADY if already set.
> > + */
> > +int usb_set_wireless_status(struct usb_interface *iface,
> > + enum usb_wireless_status status)
> > +{
> > + if (iface->wireless_status == status)
> > + return -EALREADY;
> > +
> > + usb_get_intf(iface);
> > + iface->wireless_status = status;
> > + schedule_work(&iface->wireless_status_work);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_set_wireless_status);
>
> This routine belongs in message.c, next to
> __usb_wireless_status_intf().
Sure, done locally.
^ permalink raw reply
* Re: [PATCH] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
From: Benjamin Tissoires @ 2023-02-23 16:08 UTC (permalink / raw)
To: jikos, Danny Kaehn; +Cc: linux-input, ethan.twardy
In-Reply-To: <20230210170044.11835-1-kaehndan@gmail.com>
On Fri, 10 Feb 2023 11:00:44 -0600, Danny Kaehn wrote:
> The CP2112 generates interrupts from a polling routine on a thread,
> and can only support threaded interrupts. This patch configures the
> gpiochip irq chip with this flag, disallowing consumers to request
> a hard IRQ from this driver, which resulted in a segfault previously.
>
>
Applied to hid/hid.git (for-6.3/upstream-fixes), thanks!
[1/1] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
https://git.kernel.org/hid/hid/c/37f5b858a665
Cheers,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply
* Re: [PATCH 4/5] USB: core: Add API to change the wireless_status
From: Alan Stern @ 2023-02-23 15:41 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Greg Kroah-Hartman, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-4-hadess@hadess.net>
On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> Allow device specific drivers to change the wireless status of a device.
> This will allow user-space to know whether the device is available,
> whether or not specific USB interfaces can detect it.
>
> This can be used by wireless headsets with USB receivers to propagate to
> user-space whether or not the headset is turned on, so as to consider it
> as unavailable, and not switch to it just because the receiver is
> plugged in.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> drivers/usb/core/message.c | 13 +++++++++++++
> drivers/usb/core/usb.c | 24 ++++++++++++++++++++++++
> include/linux/usb.h | 4 ++++
> 3 files changed, 41 insertions(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 127fac1af676..d5c7749d515e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1908,6 +1908,18 @@ static void __usb_queue_reset_device(struct work_struct *ws)
> usb_put_intf(iface); /* Undo _get_ in usb_queue_reset_device() */
> }
>
> +/*
> + * Internal function to set the wireless_status sysfs attribute
> + * See usb_set_wireless_status() for more details
> + */
> +static void __usb_wireless_status_intf(struct work_struct *ws)
> +{
> + struct usb_interface *iface =
> + container_of(ws, struct usb_interface, wireless_status_work);
> +
> + usb_update_wireless_status_attr(iface);
> + usb_put_intf(iface); /* Undo _get_ in usb_set_wireless_status() */
> +}
Have you thought about what will happen if this routine ends up running
after the interface has been deleted?
> /*
> * usb_set_configuration - Makes a particular device setting be current
> @@ -2100,6 +2112,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
> intf->dev.type = &usb_if_device_type;
> intf->dev.groups = usb_interface_groups;
> INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
> + INIT_WORK(&intf->wireless_status_work, __usb_wireless_status_intf);
> intf->minor = -1;
> device_initialize(&intf->dev);
> pm_runtime_no_callbacks(&intf->dev);
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 11b15d7b357a..5f42c5b9d209 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct usb_device *dev)
> }
> EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
>
> +/**
> + * usb_set_wireless_status - sets the wireless_status struct member
> + * @dev: the device to modify
> + * @status: the new wireless status
> + *
> + * Set the wireless_status struct member to the new value, and emit
> + * sysfs changes as necessary.
> + *
> + * Returns: 0 on success, -EALREADY if already set.
> + */
> +int usb_set_wireless_status(struct usb_interface *iface,
> + enum usb_wireless_status status)
> +{
> + if (iface->wireless_status == status)
> + return -EALREADY;
> +
> + usb_get_intf(iface);
> + iface->wireless_status = status;
> + schedule_work(&iface->wireless_status_work);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_set_wireless_status);
This routine belongs in message.c, next to __usb_wireless_status_intf().
Alan Stern
^ permalink raw reply
* "proximity" attribute for input devices
From: Limonciello, Mario @ 2023-02-23 15:29 UTC (permalink / raw)
To: linux-input@vger.kernel.org, Bastien Nocera, ping.cheng@wacom.com,
jason.gerecke@wacom.com
[Public]
Hello,
A problem was found that a Wacom device's stylus battery showed up in GNOME even though the stylus wasn't near by.
As part of discussing how to handle it Bastien proposes a new sysfs attribute "proximity" to let userspace know this.
He asked[1] to start the discussion on Linux-input ML.
[1] https://bugzilla.kernel.org/show_bug.cgi?id=217062#c7
Thanks,
^ permalink raw reply
* Re: [PATCH 4/5] USB: core: Add API to change the wireless_status
From: Bastien Nocera @ 2023-02-23 14:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <Y/dvsH0IXhdOKh3L@kroah.com>
On Thu, 2023-02-23 at 14:52 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> > Allow device specific drivers to change the wireless status of a
> > device.
> > This will allow user-space to know whether the device is available,
> > whether or not specific USB interfaces can detect it.
> >
> > This can be used by wireless headsets with USB receivers to
> > propagate to
> > user-space whether or not the headset is turned on, so as to
> > consider it
> > as unavailable, and not switch to it just because the receiver is
> > plugged in.
> >
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> > drivers/usb/core/message.c | 13 +++++++++++++
> > drivers/usb/core/usb.c | 24 ++++++++++++++++++++++++
> > include/linux/usb.h | 4 ++++
> > 3 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/usb/core/message.c
> > b/drivers/usb/core/message.c
> > index 127fac1af676..d5c7749d515e 100644
> > --- a/drivers/usb/core/message.c
> > +++ b/drivers/usb/core/message.c
> > @@ -1908,6 +1908,18 @@ static void __usb_queue_reset_device(struct
> > work_struct *ws)
> > usb_put_intf(iface); /* Undo _get_ in
> > usb_queue_reset_device() */
> > }
> >
> > +/*
> > + * Internal function to set the wireless_status sysfs attribute
> > + * See usb_set_wireless_status() for more details
> > + */
> > +static void __usb_wireless_status_intf(struct work_struct *ws)
> > +{
> > + struct usb_interface *iface =
> > + container_of(ws, struct usb_interface,
> > wireless_status_work);
> > +
> > + usb_update_wireless_status_attr(iface);
> > + usb_put_intf(iface); /* Undo _get_ in
> > usb_set_wireless_status() */
> > +}
>
> Why is this in a workqueue? Why can't it just happen when asked?
>
> I do not see any justification for that in your changelog or code :(
Because, in many cases, updates would be triggered from USB events,
which happen in a softirq. Is that explanation good enough for the
commit message, or do you prefer it in the code?
Looks something like this if you don't use a workqueue:
Call Trace:
<IRQ>
dump_stack_lvl+0x5b/0x77
mark_lock.cold+0x48/0xe1
? lock_is_held_type+0xe8/0x140
? lock_is_held_type+0xe8/0x140
__lock_acquire+0x800/0x1ef0
? update_load_avg+0x762/0x890
lock_acquire+0xce/0x2d0
? __kmem_cache_alloc_node+0x31/0x3b0
? lock_is_held_type+0xe8/0x140
? find_held_lock+0x32/0x90
fs_reclaim_acquire+0xa5/0xe0
? __kmem_cache_alloc_node+0x31/0x3b0
__kmem_cache_alloc_node+0x31/0x3b0
? usb_set_wireless_status+0x29/0x120
kmalloc_trace+0x26/0x60
usb_set_wireless_status+0x29/0x120
hidpp_raw_hidpp_event.cold+0x107/0x119 [hid_logitech_hidpp]
? lock_release+0x14f/0x460
hidpp_raw_event+0xf2/0x610 [hid_logitech_hidpp]
? _raw_spin_unlock_irqrestore+0x40/0x60
hid_input_report+0x142/0x160
hid_irq_in+0x177/0x1a0
__usb_hcd_giveback_urb+0x9a/0x110
usb_giveback_urb_bh+0x97/0x110
tasklet_action_common.constprop.0+0xe3/0x110
__do_softirq+0xec/0x590
__irq_exit_rcu+0xf9/0x170
irq_exit_rcu+0xa/0x30
common_interrupt+0xb9/0xd0
</IRQ>
<TASK>
asm_common_interrupt+0x22/0x40
RIP: 0010:cpuidle_enter_state+0xeb/0x320
Code: 4b 99 75 ff 45 84 ff 74 16 9c 58 0f 1f 40 00 f6 c4 02 0f 85 05 02
00 00 31 ff e8 80 b6 7d ff e8 1b 9c 85 ff fb 0f 1f 44 00 00 <45> 85 ed
0f 88 e2 00 00 00 49 63 cd 4c 89 f2 48 8d 04 49 48 8d 04
RSP: 0018:ffffaa07c017be98 EFLAGS: 00000206
RAX: 00000000000a8327 RBX: ffffca07be8036b8 RCX: 0000000000000001
RDX: 0000000000000000 RSI: ffffffffb46ba489 RDI: ffffffffb463dd36
RBP: 0000000000000008 R08: 0000000000000001 R09: 0000000000000001
R10: 0000000000000001 R11: 0000000000000000 R12: ffffffffb50d5e00
R13: 0000000000000008 R14: 0000001da3b692b2 R15: 0000000000000000
? cpuidle_enter_state+0xe5/0x320
cpuidle_enter+0x29/0x40
do_idle+0x21d/0x2b0
cpu_startup_entry+0x19/0x20
start_secondary+0x113/0x140
secondary_startup_64_no_verify+0xe5/0xeb
</TASK>
BUG: sleeping function called from invalid context at
include/linux/sched/mm.h:274
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, name:
swapper/5
preempt_count: 101, expected: 0
^ permalink raw reply
* Re: [PATCH 3/5] USB: core: Add wireless_status sysfs attribute
From: Bastien Nocera @ 2023-02-23 14:58 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <Y/dvesNrHivGV6JK@kroah.com>
On Thu, 2023-02-23 at 14:51 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:50PM +0100, Bastien Nocera wrote:
> > Add a wireless_status sysfs attribute to USB devices to keep track
> > of
> > whether a USB device that uses a receiver/emitter combo has its
> > emitter connected or disconnected.
>
> That's going to be very vague, and is starting to get very
> interface-specific as an attibute here.
>
> Why can't it just be an input device attribute? Why is "wireless"
> suddenly a special case for USB devices (we thought we got rid of the
> old wireless usb code...)
We already had that discussion in December:
https://patchwork.kernel.org/project/linux-usb/patch/d9f8b9413c10fcf067658979d16a4f5c7abe69e7.camel@hadess.net/
Do you want me to reference this discussion somewhere?
It's not an input device attribute because, in the rest of this
patchset, the fact that it uses HID is an implementation detail.
> > By default, the USB device will declare not to use a
> > receiver/emitter.
>
> I do not understand this statement, what do you mean by this?
That the default is for the wireless_status sysfs attribute not to be
present, which means that the device isn't a receiver/emitter. I can
remove that if it's confusing.
>
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> > Documentation/ABI/testing/sysfs-bus-usb | 12 ++++++
> > drivers/usb/core/sysfs.c | 50
> > +++++++++++++++++++++++++
> > drivers/usb/core/usb.h | 1 +
> > include/linux/usb.h | 10 +++++
> > 4 files changed, 73 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-usb
> > b/Documentation/ABI/testing/sysfs-bus-usb
> > index 545c2dd97ed0..0bd22ece05cd 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-usb
> > +++ b/Documentation/ABI/testing/sysfs-bus-usb
> > @@ -166,6 +166,18 @@ Description:
> > The file will be present for all speeds of USB
> > devices, and will
> > always read "no" for USB 1.1 and USB 2.0 devices.
> >
> > +What: /sys/bus/usb/devices/<INTERFACE>/wireless_status
> > +Date: January 2023
>
> January was last month :(
Sure.
> > +Contact: Bastien Nocera <hadess@hadess.net>
> > +Description:
> > + Some USB devices use a small USB receiver coupled
> > with a larger
> > + wireless device, usually communicating using
> > proprietary
> > + wireless protocols. This attribute will read either
> > "connected"
> > + or "disconnected" depending on whether the emitter
> > is turned on,
> > + in range and connected, on the interface which is
> > used to detect
> > + this state. If the device does not use a
> > receiver/emitter combo,
> > + then this attribute will not exist.
>
> So would you declare a wireless network device such a thing?
No, because the emitter and receiver together aren't considered a
single device.
> See, it gets tricky, I do not think this should be a generic USB
> attribute at all.
See above, it was already discussed.
> > +
> > What: /sys/bus/usb/devices/.../<hub_interface>/port<X>
> > Date: August 2012
> > Contact: Lan Tianyu <tianyu.lan@intel.com>
> > diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> > index 8217032dfb85..da3c0f0dd633 100644
> > --- a/drivers/usb/core/sysfs.c
> > +++ b/drivers/usb/core/sysfs.c
> > @@ -1232,9 +1232,59 @@ static const struct attribute_group
> > intf_assoc_attr_grp = {
> > .is_visible = intf_assoc_attrs_are_visible,
> > };
> >
> > +static ssize_t wireless_status_show(struct device *dev,
> > + struct device_attribute *attr,
> > char *buf)
> > +{
> > + struct usb_interface *intf;
> > +
> > + intf = to_usb_interface(dev);
> > + if (intf->wireless_status ==
> > USB_WIRELESS_STATUS_DISCONNECTED)
> > + return sysfs_emit(buf, "%s\n", "disconnected");
> > + return sysfs_emit(buf, "%s\n", "connected");
> > +}
> > +static DEVICE_ATTR_RO(wireless_status);
> > +
> > +static struct attribute *intf_wireless_status_attrs[] = {
> > + &dev_attr_wireless_status.attr,
> > + NULL
> > +};
> > +
> > +static umode_t intf_wireless_status_attr_is_visible(struct kobject
> > *kobj,
> > + struct attribute *a, int n)
> > +{
> > + struct device *dev = kobj_to_dev(kobj);
> > + struct usb_interface *intf = to_usb_interface(dev);
> > +
> > + if (a != &dev_attr_wireless_status.attr ||
> > + intf->wireless_status != USB_WIRELESS_STATUS_NA)
> > + return a->mode;
> > + return 0;
> > +}
> > +
> > +static const struct attribute_group intf_wireless_status_attr_grp
> > = {
> > + .attrs = intf_wireless_status_attrs,
> > + .is_visible = intf_wireless_status_attr_is_visible,
> > +};
> > +
> > +int usb_update_wireless_status_attr(struct usb_interface *intf)
> > +{
> > + struct device *dev = &intf->dev;
> > + int ret;
> > +
> > + ret = sysfs_update_group(&dev->kobj,
> > &intf_wireless_status_attr_grp);
> > + if (ret < 0)
> > + return ret;
> > +
> > + sysfs_notify(&dev->kobj, NULL, "wireless_status");
> > + kobject_uevent(&dev->kobj, KOBJ_CHANGE);
>
> That could be very noisy, why does that deserve a KOBJ_CHANGE event?
Because otherwise user-space can't know that the property either
appeared or changed. This change only happens when the device is
powered on or off while the receiver is plugged in, so this shouldn't
happen at any excessive rate.
^ permalink raw reply
* Re: [PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature
From: Bastien Nocera @ 2023-02-23 14:57 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <Y/dwiDpaBBQItu0a@kroah.com>
On Thu, 2023-02-23 at 14:56 +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 23, 2023 at 02:24:48PM +0100, Bastien Nocera wrote:
> > This is used in a number of headsets to report the voltage of the
> > battery.
> >
> > The voltage to capacity conversion is based on the C implementation
> > in HeadsetControl.
>
> What is "HeadsetControl"?
It the software referenced in the comment after which I don't need a
blank line, just below this comment ;)
>
> >
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 174
> > ++++++++++++++++++++++++++++++-
> > 1 file changed, 172 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index ff1fcebf2ec7..f6365cdf2e21 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -94,6 +94,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> > #define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL BIT(7)
> > #define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL BIT(8)
> > #define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL BIT(9)
> > +#define HIDPP_CAPABILITY_ADC_MEASUREMENT BIT(10)
> >
> > #define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit,
> > max, EV_KEY, (c))
> >
> > @@ -145,6 +146,7 @@ struct hidpp_battery {
> > u8 feature_index;
> > u8 solar_feature_index;
> > u8 voltage_feature_index;
> > + u8 adc_measurement_feature_index;
> > struct power_supply_desc desc;
> > struct power_supply *ps;
> > char name[64];
> > @@ -1744,6 +1746,164 @@ static int
> > hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
> > return ret;
> > }
> >
> > +/* ---------------------------------------------------------------
> > ----------- */
> > +/* 0x1f20: ADC
> > measurement */
> > +/* ---------------------------------------------------------------
> > ----------- */
> > +
> > +#define HIDPP_PAGE_ADC_MEASUREMENT 0x1f20
> > +
> > +#define CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT 0x00
> > +
> > +#define EVENT_ADC_MEASUREMENT_STATUS_BROADCAST 0x00
> > +
> > +static int hidpp20_map_adc_measurement_1f20_capacity(struct
> > hid_device *hid_dev, int voltage)
> > +{
> > + /* NB: This voltage curve doesn't necessarily map perfectly
> > to all
> > + * devices that implement the ADC_MEASUREMENT feature. This
> > is because
> > + * there are a few devices that use different battery
> > technology.
> > + *
> > + * Adapted from:
> > + *
> > https://github.com/Sapd/HeadsetControl/blob/acd972be0468e039b93aae81221f20a54d2d60f7/src/devices/logitech_g633_g933_935.c#L44-L52
> > + */
> > +
This is the software ^^^^^^^^^^
> No need for a blank line.
Sure.
> > + static const int voltages[] = {
> > + 4030, 4024, 4018, 4011, 4003, 3994, 3985, 3975,
> > 3963, 3951,
> > + 3937, 3922, 3907, 3893, 3880, 3868, 3857, 3846,
> > 3837, 3828,
> > + 3820, 3812, 3805, 3798, 3791, 3785, 3779, 3773,
> > 3768, 3762,
> > + 3757, 3752, 3747, 3742, 3738, 3733, 3729, 3724,
> > 3720, 3716,
> > + 3712, 3708, 3704, 3700, 3696, 3692, 3688, 3685,
> > 3681, 3677,
> > + 3674, 3670, 3667, 3663, 3660, 3657, 3653, 3650,
> > 3646, 3643,
> > + 3640, 3637, 3633, 3630, 3627, 3624, 3620, 3617,
> > 3614, 3611,
> > + 3608, 3604, 3601, 3598, 3595, 3592, 3589, 3585,
> > 3582, 3579,
> > + 3576, 3573, 3569, 3566, 3563, 3560, 3556, 3553,
> > 3550, 3546,
> > + 3543, 3539, 3536, 3532, 3529, 3525, 3499, 3466,
> > 3433, 3399,
> > + };
> > +
> > + int i;
> > +
> > + BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
>
> Why is 100 magic? If you want it to be 100, say so up in the
> declaraion
> so that the code will enforce that.
It's the same pattern used in hidpp20_map_battery_capacity() in the
same file. I'll send a patch to fix that too.
>
> > +
> > + if (voltage == 0)
> > + return 0;
> > +
> > + if (unlikely(voltage < 3400 || voltage >= 5000))
>
> Why unlikely? That should only ever be used if you can measure the
> performance impact, otherwise please remove it.
>
> > + hid_warn_once(hid_dev,
> > + "%s: possibly using the wrong voltage
> > curve\n",
> > + __func__);
> > +
> > + for (i = 0; i < ARRAY_SIZE(voltages); i++) {
> > + if (voltage >= voltages[i])
> > + return ARRAY_SIZE(voltages) - i;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hidpp20_map_adc_measurement_1f20(u8 data[3], int
> > *voltage)
> > +{
> > + int status, flags;
> > +
> > + flags = (int) data[2];
>
> Why is this now an int?
Sure.
> > +
> > + switch (flags) {
> > + case 0x01:
> > + status = POWER_SUPPLY_STATUS_DISCHARGING;
> > + break;
> > + case 0x03:
> > + status = POWER_SUPPLY_STATUS_CHARGING;
> > + break;
> > + case 0x07:
> > + status = POWER_SUPPLY_STATUS_FULL;
> > + break;
> > + case 0x0F:
> > + default:
>
> You are only checking it for a range 1-f, u8 is just fine, right?
Sure.
>
> > + status = POWER_SUPPLY_STATUS_UNKNOWN;
> > + break;
> > + }
> > +
> > + *voltage = get_unaligned_be16(data);
> > +
> > + dbg_hid("%s: Parsed 1f20 data as flag 0x%02x voltage
> > %dmV\n",
> > + __func__, flags, *voltage);
>
> I doubt you need the __func__ line, right? dynamic debug provides
> that
> for you automatically if you ask for it.
Removed.
^ permalink raw reply
* Re: [PATCH] selftest: hid: fix hid_bpf not set in config
From: Jiri Kosina @ 2023-02-23 14:46 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Shuah Khan, linux-input, linux-kselftest, linux-kernel
In-Reply-To: <20230223-fix_hid_bpf_kconfig-v1-1-71b213bd8e2b@redhat.com>
On Thu, 23 Feb 2023, Benjamin Tissoires wrote:
> Now that CONFIG_HID_BPF is not automatically implied by HID, we need
> to set it properly in the selftests config.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Applied to hid.git#for-6.3/upstream-fixes. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] selftest: hid: fix hid_bpf not set in config
From: Benjamin Tissoires @ 2023-02-23 14:17 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
Now that CONFIG_HID_BPF is not automatically implied by HID, we need
to set it properly in the selftests config.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
tools/testing/selftests/hid/config | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/hid/config b/tools/testing/selftests/hid/config
index 9c5a55abca6b..5b5cef445b54 100644
--- a/tools/testing/selftests/hid/config
+++ b/tools/testing/selftests/hid/config
@@ -17,5 +17,6 @@ CONFIG_FTRACE_SYSCALLS=y
CONFIG_FUNCTION_TRACER=y
CONFIG_HIDRAW=y
CONFIG_HID=y
+CONFIG_HID_BPF=y
CONFIG_INPUT_EVDEV=y
CONFIG_UHID=y
---
base-commit: 6c71297eaf713ece684a367ce9aff06069d715b9
change-id: 20230223-fix_hid_bpf_kconfig-899a690a7c75
Best regards,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply related
* Re: [PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature
From: Greg Kroah-Hartman @ 2023-02-23 13:56 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-1-hadess@hadess.net>
On Thu, Feb 23, 2023 at 02:24:48PM +0100, Bastien Nocera wrote:
> This is used in a number of headsets to report the voltage of the
> battery.
>
> The voltage to capacity conversion is based on the C implementation
> in HeadsetControl.
What is "HeadsetControl"?
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
> ---
> drivers/hid/hid-logitech-hidpp.c | 174 ++++++++++++++++++++++++++++++-
> 1 file changed, 172 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ff1fcebf2ec7..f6365cdf2e21 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -94,6 +94,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> #define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL BIT(7)
> #define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL BIT(8)
> #define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL BIT(9)
> +#define HIDPP_CAPABILITY_ADC_MEASUREMENT BIT(10)
>
> #define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>
> @@ -145,6 +146,7 @@ struct hidpp_battery {
> u8 feature_index;
> u8 solar_feature_index;
> u8 voltage_feature_index;
> + u8 adc_measurement_feature_index;
> struct power_supply_desc desc;
> struct power_supply *ps;
> char name[64];
> @@ -1744,6 +1746,164 @@ static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
> return ret;
> }
>
> +/* -------------------------------------------------------------------------- */
> +/* 0x1f20: ADC measurement */
> +/* -------------------------------------------------------------------------- */
> +
> +#define HIDPP_PAGE_ADC_MEASUREMENT 0x1f20
> +
> +#define CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT 0x00
> +
> +#define EVENT_ADC_MEASUREMENT_STATUS_BROADCAST 0x00
> +
> +static int hidpp20_map_adc_measurement_1f20_capacity(struct hid_device *hid_dev, int voltage)
> +{
> + /* NB: This voltage curve doesn't necessarily map perfectly to all
> + * devices that implement the ADC_MEASUREMENT feature. This is because
> + * there are a few devices that use different battery technology.
> + *
> + * Adapted from:
> + * https://github.com/Sapd/HeadsetControl/blob/acd972be0468e039b93aae81221f20a54d2d60f7/src/devices/logitech_g633_g933_935.c#L44-L52
> + */
> +
No need for a blank line.
> + static const int voltages[] = {
> + 4030, 4024, 4018, 4011, 4003, 3994, 3985, 3975, 3963, 3951,
> + 3937, 3922, 3907, 3893, 3880, 3868, 3857, 3846, 3837, 3828,
> + 3820, 3812, 3805, 3798, 3791, 3785, 3779, 3773, 3768, 3762,
> + 3757, 3752, 3747, 3742, 3738, 3733, 3729, 3724, 3720, 3716,
> + 3712, 3708, 3704, 3700, 3696, 3692, 3688, 3685, 3681, 3677,
> + 3674, 3670, 3667, 3663, 3660, 3657, 3653, 3650, 3646, 3643,
> + 3640, 3637, 3633, 3630, 3627, 3624, 3620, 3617, 3614, 3611,
> + 3608, 3604, 3601, 3598, 3595, 3592, 3589, 3585, 3582, 3579,
> + 3576, 3573, 3569, 3566, 3563, 3560, 3556, 3553, 3550, 3546,
> + 3543, 3539, 3536, 3532, 3529, 3525, 3499, 3466, 3433, 3399,
> + };
> +
> + int i;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
Why is 100 magic? If you want it to be 100, say so up in the declaraion
so that the code will enforce that.
> +
> + if (voltage == 0)
> + return 0;
> +
> + if (unlikely(voltage < 3400 || voltage >= 5000))
Why unlikely? That should only ever be used if you can measure the
performance impact, otherwise please remove it.
> + hid_warn_once(hid_dev,
> + "%s: possibly using the wrong voltage curve\n",
> + __func__);
> +
> + for (i = 0; i < ARRAY_SIZE(voltages); i++) {
> + if (voltage >= voltages[i])
> + return ARRAY_SIZE(voltages) - i;
> + }
> +
> + return 0;
> +}
> +
> +static int hidpp20_map_adc_measurement_1f20(u8 data[3], int *voltage)
> +{
> + int status, flags;
> +
> + flags = (int) data[2];
Why is this now an int?
> +
> + switch (flags) {
> + case 0x01:
> + status = POWER_SUPPLY_STATUS_DISCHARGING;
> + break;
> + case 0x03:
> + status = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> + case 0x07:
> + status = POWER_SUPPLY_STATUS_FULL;
> + break;
> + case 0x0F:
> + default:
You are only checking it for a range 1-f, u8 is just fine, right?
> + status = POWER_SUPPLY_STATUS_UNKNOWN;
> + break;
> + }
> +
> + *voltage = get_unaligned_be16(data);
> +
> + dbg_hid("%s: Parsed 1f20 data as flag 0x%02x voltage %dmV\n",
> + __func__, flags, *voltage);
I doubt you need the __func__ line, right? dynamic debug provides that
for you automatically if you ask for it.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 4/5] USB: core: Add API to change the wireless_status
From: Greg Kroah-Hartman @ 2023-02-23 13:52 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-4-hadess@hadess.net>
On Thu, Feb 23, 2023 at 02:24:51PM +0100, Bastien Nocera wrote:
> Allow device specific drivers to change the wireless status of a device.
> This will allow user-space to know whether the device is available,
> whether or not specific USB interfaces can detect it.
>
> This can be used by wireless headsets with USB receivers to propagate to
> user-space whether or not the headset is turned on, so as to consider it
> as unavailable, and not switch to it just because the receiver is
> plugged in.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> drivers/usb/core/message.c | 13 +++++++++++++
> drivers/usb/core/usb.c | 24 ++++++++++++++++++++++++
> include/linux/usb.h | 4 ++++
> 3 files changed, 41 insertions(+)
>
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 127fac1af676..d5c7749d515e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1908,6 +1908,18 @@ static void __usb_queue_reset_device(struct work_struct *ws)
> usb_put_intf(iface); /* Undo _get_ in usb_queue_reset_device() */
> }
>
> +/*
> + * Internal function to set the wireless_status sysfs attribute
> + * See usb_set_wireless_status() for more details
> + */
> +static void __usb_wireless_status_intf(struct work_struct *ws)
> +{
> + struct usb_interface *iface =
> + container_of(ws, struct usb_interface, wireless_status_work);
> +
> + usb_update_wireless_status_attr(iface);
> + usb_put_intf(iface); /* Undo _get_ in usb_set_wireless_status() */
> +}
Why is this in a workqueue? Why can't it just happen when asked?
I do not see any justification for that in your changelog or code :(
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/5] USB: core: Add wireless_status sysfs attribute
From: Greg Kroah-Hartman @ 2023-02-23 13:51 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-3-hadess@hadess.net>
On Thu, Feb 23, 2023 at 02:24:50PM +0100, Bastien Nocera wrote:
> Add a wireless_status sysfs attribute to USB devices to keep track of
> whether a USB device that uses a receiver/emitter combo has its
> emitter connected or disconnected.
That's going to be very vague, and is starting to get very
interface-specific as an attibute here.
Why can't it just be an input device attribute? Why is "wireless"
suddenly a special case for USB devices (we thought we got rid of the
old wireless usb code...)
> By default, the USB device will declare not to use a receiver/emitter.
I do not understand this statement, what do you mean by this?
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> Documentation/ABI/testing/sysfs-bus-usb | 12 ++++++
> drivers/usb/core/sysfs.c | 50 +++++++++++++++++++++++++
> drivers/usb/core/usb.h | 1 +
> include/linux/usb.h | 10 +++++
> 4 files changed, 73 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index 545c2dd97ed0..0bd22ece05cd 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -166,6 +166,18 @@ Description:
> The file will be present for all speeds of USB devices, and will
> always read "no" for USB 1.1 and USB 2.0 devices.
>
> +What: /sys/bus/usb/devices/<INTERFACE>/wireless_status
> +Date: January 2023
January was last month :(
> +Contact: Bastien Nocera <hadess@hadess.net>
> +Description:
> + Some USB devices use a small USB receiver coupled with a larger
> + wireless device, usually communicating using proprietary
> + wireless protocols. This attribute will read either "connected"
> + or "disconnected" depending on whether the emitter is turned on,
> + in range and connected, on the interface which is used to detect
> + this state. If the device does not use a receiver/emitter combo,
> + then this attribute will not exist.
So would you declare a wireless network device such a thing?
See, it gets tricky, I do not think this should be a generic USB
attribute at all.
> +
> What: /sys/bus/usb/devices/.../<hub_interface>/port<X>
> Date: August 2012
> Contact: Lan Tianyu <tianyu.lan@intel.com>
> diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
> index 8217032dfb85..da3c0f0dd633 100644
> --- a/drivers/usb/core/sysfs.c
> +++ b/drivers/usb/core/sysfs.c
> @@ -1232,9 +1232,59 @@ static const struct attribute_group intf_assoc_attr_grp = {
> .is_visible = intf_assoc_attrs_are_visible,
> };
>
> +static ssize_t wireless_status_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct usb_interface *intf;
> +
> + intf = to_usb_interface(dev);
> + if (intf->wireless_status == USB_WIRELESS_STATUS_DISCONNECTED)
> + return sysfs_emit(buf, "%s\n", "disconnected");
> + return sysfs_emit(buf, "%s\n", "connected");
> +}
> +static DEVICE_ATTR_RO(wireless_status);
> +
> +static struct attribute *intf_wireless_status_attrs[] = {
> + &dev_attr_wireless_status.attr,
> + NULL
> +};
> +
> +static umode_t intf_wireless_status_attr_is_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct usb_interface *intf = to_usb_interface(dev);
> +
> + if (a != &dev_attr_wireless_status.attr ||
> + intf->wireless_status != USB_WIRELESS_STATUS_NA)
> + return a->mode;
> + return 0;
> +}
> +
> +static const struct attribute_group intf_wireless_status_attr_grp = {
> + .attrs = intf_wireless_status_attrs,
> + .is_visible = intf_wireless_status_attr_is_visible,
> +};
> +
> +int usb_update_wireless_status_attr(struct usb_interface *intf)
> +{
> + struct device *dev = &intf->dev;
> + int ret;
> +
> + ret = sysfs_update_group(&dev->kobj, &intf_wireless_status_attr_grp);
> + if (ret < 0)
> + return ret;
> +
> + sysfs_notify(&dev->kobj, NULL, "wireless_status");
> + kobject_uevent(&dev->kobj, KOBJ_CHANGE);
That could be very noisy, why does that deserve a KOBJ_CHANGE event?
thanks,
greg k-h
^ permalink raw reply
* [PATCH 5/5] HID: logitech-hidpp: Set wireless_status for G935 receiver
From: Bastien Nocera @ 2023-02-23 13:24 UTC (permalink / raw)
To: linux-usb, linux-input
Cc: Greg Kroah-Hartman, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-1-hadess@hadess.net>
Set the USB interface "wireless_status" for the G935 receiver when
receiving battery notifications.
This will allow sound daemons such as Pipewire or PulseAudio to know
whether or not the headset is turned on and connected.
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
drivers/hid/hid-logitech-hidpp.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 166cfe92192e..7809d13bb84e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -74,6 +74,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS BIT(27)
#define HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS BIT(28)
#define HIDPP_QUIRK_HI_RES_SCROLL_1P0 BIT(29)
+#define HIDPP_QUIRK_WIRELESS_STATUS BIT(30)
/* These are just aliases for now */
#define HIDPP_QUIRK_KBD_SCROLL_WHEEL HIDPP_QUIRK_HIDPP_WHEELS
@@ -472,6 +473,26 @@ static void hidpp_prefix_name(char **name, int name_length)
*name = new_name;
}
+/*
+ * Updates the USB wireless_status based on whether the headset
+ * is turned on and reachable.
+ */
+static void hidpp_update_usb_wireless_status(struct hidpp_device *hidpp)
+{
+ struct hid_device *hdev = hidpp->hid_dev;
+ struct usb_interface *intf;
+
+ if (!(hidpp->quirks & HIDPP_QUIRK_WIRELESS_STATUS))
+ return;
+ if (!hid_is_usb(hdev))
+ return;
+
+ intf = to_usb_interface(hdev->dev.parent);
+ usb_set_wireless_status(intf, hidpp->battery.online ?
+ USB_WIRELESS_STATUS_CONNECTED :
+ USB_WIRELESS_STATUS_DISCONNECTED);
+}
+
/**
* hidpp_scroll_counter_handle_scroll() - Send high- and low-resolution scroll
* events given a high-resolution wheel
@@ -1876,6 +1897,7 @@ static int hidpp20_query_adc_measurement_info_1f20(struct hidpp_device *hidpp)
&hidpp->battery.voltage);
hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev,
hidpp->battery.voltage);
+ hidpp_update_usb_wireless_status(hidpp);
return 0;
}
@@ -1900,6 +1922,7 @@ static int hidpp20_adc_measurement_event_1f20(struct hidpp_device *hidpp,
hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev, voltage);
if (hidpp->battery.ps)
power_supply_changed(hidpp->battery.ps);
+ hidpp_update_usb_wireless_status(hidpp);
}
return 0;
}
@@ -4561,7 +4584,8 @@ static const struct hid_device_id hidpp_devices[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
{ /* G935 Gaming Headset */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87) },
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87),
+ .driver_data = HIDPP_QUIRK_WIRELESS_STATUS },
{ /* MX5000 keyboard over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
--
2.39.2
^ permalink raw reply related
* [PATCH 4/5] USB: core: Add API to change the wireless_status
From: Bastien Nocera @ 2023-02-23 13:24 UTC (permalink / raw)
To: linux-usb, linux-input
Cc: Greg Kroah-Hartman, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-1-hadess@hadess.net>
Allow device specific drivers to change the wireless status of a device.
This will allow user-space to know whether the device is available,
whether or not specific USB interfaces can detect it.
This can be used by wireless headsets with USB receivers to propagate to
user-space whether or not the headset is turned on, so as to consider it
as unavailable, and not switch to it just because the receiver is
plugged in.
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
drivers/usb/core/message.c | 13 +++++++++++++
drivers/usb/core/usb.c | 24 ++++++++++++++++++++++++
include/linux/usb.h | 4 ++++
3 files changed, 41 insertions(+)
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 127fac1af676..d5c7749d515e 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1908,6 +1908,18 @@ static void __usb_queue_reset_device(struct work_struct *ws)
usb_put_intf(iface); /* Undo _get_ in usb_queue_reset_device() */
}
+/*
+ * Internal function to set the wireless_status sysfs attribute
+ * See usb_set_wireless_status() for more details
+ */
+static void __usb_wireless_status_intf(struct work_struct *ws)
+{
+ struct usb_interface *iface =
+ container_of(ws, struct usb_interface, wireless_status_work);
+
+ usb_update_wireless_status_attr(iface);
+ usb_put_intf(iface); /* Undo _get_ in usb_set_wireless_status() */
+}
/*
* usb_set_configuration - Makes a particular device setting be current
@@ -2100,6 +2112,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
intf->dev.type = &usb_if_device_type;
intf->dev.groups = usb_interface_groups;
INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
+ INIT_WORK(&intf->wireless_status_work, __usb_wireless_status_intf);
intf->minor = -1;
device_initialize(&intf->dev);
pm_runtime_no_callbacks(&intf->dev);
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 11b15d7b357a..5f42c5b9d209 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -871,6 +871,30 @@ int usb_get_current_frame_number(struct usb_device *dev)
}
EXPORT_SYMBOL_GPL(usb_get_current_frame_number);
+/**
+ * usb_set_wireless_status - sets the wireless_status struct member
+ * @dev: the device to modify
+ * @status: the new wireless status
+ *
+ * Set the wireless_status struct member to the new value, and emit
+ * sysfs changes as necessary.
+ *
+ * Returns: 0 on success, -EALREADY if already set.
+ */
+int usb_set_wireless_status(struct usb_interface *iface,
+ enum usb_wireless_status status)
+{
+ if (iface->wireless_status == status)
+ return -EALREADY;
+
+ usb_get_intf(iface);
+ iface->wireless_status = status;
+ schedule_work(&iface->wireless_status_work);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(usb_set_wireless_status);
+
/*-------------------------------------------------------------------*/
/*
* __usb_get_extra_descriptor() finds a descriptor of specific type in the
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 517ae4b4e333..a48eeec62a66 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -262,6 +262,7 @@ struct usb_interface {
unsigned resetting_device:1; /* true: bandwidth alloc after reset */
unsigned authorized:1; /* used for interface authorization */
enum usb_wireless_status wireless_status;
+ struct work_struct wireless_status_work;
struct device dev; /* interface specific device info */
struct device *usb_dev;
@@ -897,6 +898,9 @@ static inline int usb_interface_claimed(struct usb_interface *iface)
extern void usb_driver_release_interface(struct usb_driver *driver,
struct usb_interface *iface);
+int usb_set_wireless_status(struct usb_interface *iface,
+ enum usb_wireless_status status);
+
const struct usb_device_id *usb_match_id(struct usb_interface *interface,
const struct usb_device_id *id);
extern int usb_match_one_id(struct usb_interface *interface,
--
2.39.2
^ permalink raw reply related
* [PATCH 3/5] USB: core: Add wireless_status sysfs attribute
From: Bastien Nocera @ 2023-02-23 13:24 UTC (permalink / raw)
To: linux-usb, linux-input
Cc: Greg Kroah-Hartman, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-1-hadess@hadess.net>
Add a wireless_status sysfs attribute to USB devices to keep track of
whether a USB device that uses a receiver/emitter combo has its
emitter connected or disconnected.
By default, the USB device will declare not to use a receiver/emitter.
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
Documentation/ABI/testing/sysfs-bus-usb | 12 ++++++
drivers/usb/core/sysfs.c | 50 +++++++++++++++++++++++++
drivers/usb/core/usb.h | 1 +
include/linux/usb.h | 10 +++++
4 files changed, 73 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 545c2dd97ed0..0bd22ece05cd 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -166,6 +166,18 @@ Description:
The file will be present for all speeds of USB devices, and will
always read "no" for USB 1.1 and USB 2.0 devices.
+What: /sys/bus/usb/devices/<INTERFACE>/wireless_status
+Date: January 2023
+Contact: Bastien Nocera <hadess@hadess.net>
+Description:
+ Some USB devices use a small USB receiver coupled with a larger
+ wireless device, usually communicating using proprietary
+ wireless protocols. This attribute will read either "connected"
+ or "disconnected" depending on whether the emitter is turned on,
+ in range and connected, on the interface which is used to detect
+ this state. If the device does not use a receiver/emitter combo,
+ then this attribute will not exist.
+
What: /sys/bus/usb/devices/.../<hub_interface>/port<X>
Date: August 2012
Contact: Lan Tianyu <tianyu.lan@intel.com>
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 8217032dfb85..da3c0f0dd633 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -1232,9 +1232,59 @@ static const struct attribute_group intf_assoc_attr_grp = {
.is_visible = intf_assoc_attrs_are_visible,
};
+static ssize_t wireless_status_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct usb_interface *intf;
+
+ intf = to_usb_interface(dev);
+ if (intf->wireless_status == USB_WIRELESS_STATUS_DISCONNECTED)
+ return sysfs_emit(buf, "%s\n", "disconnected");
+ return sysfs_emit(buf, "%s\n", "connected");
+}
+static DEVICE_ATTR_RO(wireless_status);
+
+static struct attribute *intf_wireless_status_attrs[] = {
+ &dev_attr_wireless_status.attr,
+ NULL
+};
+
+static umode_t intf_wireless_status_attr_is_visible(struct kobject *kobj,
+ struct attribute *a, int n)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct usb_interface *intf = to_usb_interface(dev);
+
+ if (a != &dev_attr_wireless_status.attr ||
+ intf->wireless_status != USB_WIRELESS_STATUS_NA)
+ return a->mode;
+ return 0;
+}
+
+static const struct attribute_group intf_wireless_status_attr_grp = {
+ .attrs = intf_wireless_status_attrs,
+ .is_visible = intf_wireless_status_attr_is_visible,
+};
+
+int usb_update_wireless_status_attr(struct usb_interface *intf)
+{
+ struct device *dev = &intf->dev;
+ int ret;
+
+ ret = sysfs_update_group(&dev->kobj, &intf_wireless_status_attr_grp);
+ if (ret < 0)
+ return ret;
+
+ sysfs_notify(&dev->kobj, NULL, "wireless_status");
+ kobject_uevent(&dev->kobj, KOBJ_CHANGE);
+
+ return 0;
+}
+
const struct attribute_group *usb_interface_groups[] = {
&intf_attr_grp,
&intf_assoc_attr_grp,
+ &intf_wireless_status_attr_grp,
NULL
};
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 0eac7d4285d1..3f14e15f07f6 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -15,6 +15,7 @@ extern int usb_create_sysfs_dev_files(struct usb_device *dev);
extern void usb_remove_sysfs_dev_files(struct usb_device *dev);
extern void usb_create_sysfs_intf_files(struct usb_interface *intf);
extern void usb_remove_sysfs_intf_files(struct usb_interface *intf);
+extern int usb_update_wireless_status_attr(struct usb_interface *intf);
extern int usb_create_ep_devs(struct device *parent,
struct usb_host_endpoint *endpoint,
struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 86d1c8e79566..517ae4b4e333 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -170,6 +170,12 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt,
return usb_find_common_endpoints_reverse(alt, NULL, NULL, NULL, int_out);
}
+enum usb_wireless_status {
+ USB_WIRELESS_STATUS_NA = 0,
+ USB_WIRELESS_STATUS_DISCONNECTED,
+ USB_WIRELESS_STATUS_CONNECTED,
+};
+
/**
* struct usb_interface - what usb device drivers talk to
* @altsetting: array of interface structures, one for each alternate
@@ -203,6 +209,8 @@ usb_find_last_int_out_endpoint(struct usb_host_interface *alt,
* @reset_ws: Used for scheduling resets from atomic context.
* @resetting_device: USB core reset the device, so use alt setting 0 as
* current; needs bandwidth alloc after reset.
+ * @wireless_status: if the USB device uses a receiver/emitter combo, whether
+ * the emitter is connected.
*
* USB device drivers attach to interfaces on a physical device. Each
* interface encapsulates a single high level function, such as feeding
@@ -253,6 +261,7 @@ struct usb_interface {
unsigned needs_binding:1; /* needs delayed unbind/rebind */
unsigned resetting_device:1; /* true: bandwidth alloc after reset */
unsigned authorized:1; /* used for interface authorization */
+ enum usb_wireless_status wireless_status;
struct device dev; /* interface specific device info */
struct device *usb_dev;
@@ -887,6 +896,7 @@ static inline int usb_interface_claimed(struct usb_interface *iface)
extern void usb_driver_release_interface(struct usb_driver *driver,
struct usb_interface *iface);
+
const struct usb_device_id *usb_match_id(struct usb_interface *interface,
const struct usb_device_id *id);
extern int usb_match_one_id(struct usb_interface *interface,
--
2.39.2
^ permalink raw reply related
* [PATCH 2/5] HID: logitech-hidpp: Add Logitech G935 headset
From: Bastien Nocera @ 2023-02-23 13:24 UTC (permalink / raw)
To: linux-usb, linux-input
Cc: Greg Kroah-Hartman, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230223132452.37958-1-hadess@hadess.net>
Add the Logitech G935 headset that uses the HID++ protocol to the
list of supported devices.
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
drivers/hid/hid-logitech-hidpp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f6365cdf2e21..166cfe92192e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4560,6 +4560,9 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* Logitech G Pro Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
+ { /* G935 Gaming Headset */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87) },
+
{ /* MX5000 keyboard over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
.driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
--
2.39.2
^ permalink raw reply related
* [PATCH 1/5] HID: logitech-hidpp: Add support for ADC measurement feature
From: Bastien Nocera @ 2023-02-23 13:24 UTC (permalink / raw)
To: linux-usb, linux-input
Cc: Greg Kroah-Hartman, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
This is used in a number of headsets to report the voltage of the
battery.
The voltage to capacity conversion is based on the C implementation
in HeadsetControl.
Signed-off-by: Bastien Nocera <hadess@hadess.net>
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=216483
---
drivers/hid/hid-logitech-hidpp.c | 174 ++++++++++++++++++++++++++++++-
1 file changed, 172 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ff1fcebf2ec7..f6365cdf2e21 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -94,6 +94,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_CAPABILITY_HIDPP20_HI_RES_WHEEL BIT(7)
#define HIDPP_CAPABILITY_HIDPP20_HI_RES_SCROLL BIT(8)
#define HIDPP_CAPABILITY_HIDPP10_FAST_SCROLL BIT(9)
+#define HIDPP_CAPABILITY_ADC_MEASUREMENT BIT(10)
#define lg_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
@@ -145,6 +146,7 @@ struct hidpp_battery {
u8 feature_index;
u8 solar_feature_index;
u8 voltage_feature_index;
+ u8 adc_measurement_feature_index;
struct power_supply_desc desc;
struct power_supply *ps;
char name[64];
@@ -1744,6 +1746,164 @@ static int hidpp_set_wireless_feature_index(struct hidpp_device *hidpp)
return ret;
}
+/* -------------------------------------------------------------------------- */
+/* 0x1f20: ADC measurement */
+/* -------------------------------------------------------------------------- */
+
+#define HIDPP_PAGE_ADC_MEASUREMENT 0x1f20
+
+#define CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT 0x00
+
+#define EVENT_ADC_MEASUREMENT_STATUS_BROADCAST 0x00
+
+static int hidpp20_map_adc_measurement_1f20_capacity(struct hid_device *hid_dev, int voltage)
+{
+ /* NB: This voltage curve doesn't necessarily map perfectly to all
+ * devices that implement the ADC_MEASUREMENT feature. This is because
+ * there are a few devices that use different battery technology.
+ *
+ * Adapted from:
+ * https://github.com/Sapd/HeadsetControl/blob/acd972be0468e039b93aae81221f20a54d2d60f7/src/devices/logitech_g633_g933_935.c#L44-L52
+ */
+
+ static const int voltages[] = {
+ 4030, 4024, 4018, 4011, 4003, 3994, 3985, 3975, 3963, 3951,
+ 3937, 3922, 3907, 3893, 3880, 3868, 3857, 3846, 3837, 3828,
+ 3820, 3812, 3805, 3798, 3791, 3785, 3779, 3773, 3768, 3762,
+ 3757, 3752, 3747, 3742, 3738, 3733, 3729, 3724, 3720, 3716,
+ 3712, 3708, 3704, 3700, 3696, 3692, 3688, 3685, 3681, 3677,
+ 3674, 3670, 3667, 3663, 3660, 3657, 3653, 3650, 3646, 3643,
+ 3640, 3637, 3633, 3630, 3627, 3624, 3620, 3617, 3614, 3611,
+ 3608, 3604, 3601, 3598, 3595, 3592, 3589, 3585, 3582, 3579,
+ 3576, 3573, 3569, 3566, 3563, 3560, 3556, 3553, 3550, 3546,
+ 3543, 3539, 3536, 3532, 3529, 3525, 3499, 3466, 3433, 3399,
+ };
+
+ int i;
+
+ BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
+
+ if (voltage == 0)
+ return 0;
+
+ if (unlikely(voltage < 3400 || voltage >= 5000))
+ hid_warn_once(hid_dev,
+ "%s: possibly using the wrong voltage curve\n",
+ __func__);
+
+ for (i = 0; i < ARRAY_SIZE(voltages); i++) {
+ if (voltage >= voltages[i])
+ return ARRAY_SIZE(voltages) - i;
+ }
+
+ return 0;
+}
+
+static int hidpp20_map_adc_measurement_1f20(u8 data[3], int *voltage)
+{
+ int status, flags;
+
+ flags = (int) data[2];
+
+ switch (flags) {
+ case 0x01:
+ status = POWER_SUPPLY_STATUS_DISCHARGING;
+ break;
+ case 0x03:
+ status = POWER_SUPPLY_STATUS_CHARGING;
+ break;
+ case 0x07:
+ status = POWER_SUPPLY_STATUS_FULL;
+ break;
+ case 0x0F:
+ default:
+ status = POWER_SUPPLY_STATUS_UNKNOWN;
+ break;
+ }
+
+ *voltage = get_unaligned_be16(data);
+
+ dbg_hid("%s: Parsed 1f20 data as flag 0x%02x voltage %dmV\n",
+ __func__, flags, *voltage);
+
+ return status;
+}
+
+/* Return value is whether the device is online */
+static bool hidpp20_get_adc_measurement_1f20(struct hidpp_device *hidpp,
+ u8 feature_index,
+ int *status, int *voltage)
+{
+ struct hidpp_report response;
+ int ret;
+ u8 *params = (u8 *)response.fap.params;
+
+ *status = POWER_SUPPLY_STATUS_UNKNOWN;
+ *voltage = 0;
+ ret = hidpp_send_fap_command_sync(hidpp, feature_index,
+ CMD_ADC_MEASUREMENT_GET_ADC_MEASUREMENT,
+ NULL, 0, &response);
+
+ if (ret > 0) {
+ hid_dbg(hidpp->hid_dev, "%s: received protocol error 0x%02x\n",
+ __func__, ret);
+ return false;
+ }
+
+ *status = hidpp20_map_adc_measurement_1f20(params, voltage);
+ return true;
+}
+
+static int hidpp20_query_adc_measurement_info_1f20(struct hidpp_device *hidpp)
+{
+ u8 feature_type;
+
+ if (hidpp->battery.adc_measurement_feature_index == 0xff) {
+ int ret;
+
+ ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_ADC_MEASUREMENT,
+ &hidpp->battery.adc_measurement_feature_index,
+ &feature_type);
+ if (ret)
+ return ret;
+
+ hidpp->capabilities |= HIDPP_CAPABILITY_ADC_MEASUREMENT;
+ }
+
+ hidpp->battery.online = hidpp20_get_adc_measurement_1f20(hidpp,
+ hidpp->battery.adc_measurement_feature_index,
+ &hidpp->battery.status,
+ &hidpp->battery.voltage);
+ hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev,
+ hidpp->battery.voltage);
+
+ return 0;
+}
+
+static int hidpp20_adc_measurement_event_1f20(struct hidpp_device *hidpp,
+ u8 *data, int size)
+{
+ struct hidpp_report *report = (struct hidpp_report *)data;
+ int status, voltage;
+
+ if (report->fap.feature_index != hidpp->battery.adc_measurement_feature_index ||
+ report->fap.funcindex_clientid != EVENT_ADC_MEASUREMENT_STATUS_BROADCAST)
+ return 0;
+
+ status = hidpp20_map_adc_measurement_1f20(report->fap.params, &voltage);
+
+ hidpp->battery.online = status != POWER_SUPPLY_STATUS_UNKNOWN;
+
+ if (voltage != hidpp->battery.voltage || status != hidpp->battery.status) {
+ hidpp->battery.status = status;
+ hidpp->battery.voltage = voltage;
+ hidpp->battery.capacity = hidpp20_map_adc_measurement_1f20_capacity(hidpp->hid_dev, voltage);
+ if (hidpp->battery.ps)
+ power_supply_changed(hidpp->battery.ps);
+ }
+ return 0;
+}
+
/* -------------------------------------------------------------------------- */
/* 0x2120: Hi-resolution scrolling */
/* -------------------------------------------------------------------------- */
@@ -3662,6 +3822,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
ret = hidpp20_battery_voltage_event(hidpp, data, size);
if (ret != 0)
return ret;
+ ret = hidpp20_adc_measurement_event_1f20(hidpp, data, size);
+ if (ret != 0)
+ return ret;
}
if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) {
@@ -3785,6 +3948,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
hidpp->battery.feature_index = 0xff;
hidpp->battery.solar_feature_index = 0xff;
hidpp->battery.voltage_feature_index = 0xff;
+ hidpp->battery.adc_measurement_feature_index = 0xff;
if (hidpp->protocol_major >= 2) {
if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750)
@@ -3798,6 +3962,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
ret = hidpp20_query_battery_info_1004(hidpp);
if (ret)
ret = hidpp20_query_battery_voltage_info(hidpp);
+ if (ret)
+ ret = hidpp20_query_adc_measurement_info_1f20(hidpp);
}
if (ret)
@@ -3827,7 +3993,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE ||
hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE ||
- hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+ hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE ||
+ hidpp->capabilities & HIDPP_CAPABILITY_ADC_MEASUREMENT)
battery_props[num_battery_props++] =
POWER_SUPPLY_PROP_CAPACITY;
@@ -3835,7 +4002,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
battery_props[num_battery_props++] =
POWER_SUPPLY_PROP_CAPACITY_LEVEL;
- if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
+ if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE ||
+ hidpp->capabilities & HIDPP_CAPABILITY_ADC_MEASUREMENT)
battery_props[num_battery_props++] =
POWER_SUPPLY_PROP_VOLTAGE_NOW;
@@ -4008,6 +4176,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
hidpp20_query_battery_voltage_info(hidpp);
else if (hidpp->capabilities & HIDPP_CAPABILITY_UNIFIED_BATTERY)
hidpp20_query_battery_info_1004(hidpp);
+ else if (hidpp->capabilities & HIDPP_CAPABILITY_ADC_MEASUREMENT)
+ hidpp20_query_adc_measurement_info_1f20(hidpp);
else
hidpp20_query_battery_info_1000(hidpp);
}
--
2.39.2
^ permalink raw reply related
* Re: [PATCH] HID: usbhid: enable remote wakeup for mice
From: Oliver Neukum @ 2023-02-23 12:01 UTC (permalink / raw)
To: Greg KH, Michael Wu
Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel,
mario.limonciello, richard.gong
In-Reply-To: <Y/dMq2KKYfdMdrjh@kroah.com>
On 23.02.23 12:23, Greg KH wrote:
>> I just want to explain the background of my patch, to prove we could use a
>> similar way to avoid such a "disturbing" situation.
>> To reduce the influence, my patch enables remote wakeup for USB mouse
>> devices refer to what keyboard do.
>
> Keyboards are not mice :)
>
OK, let me explain, why I never proposed switching on autosuspend
for mice or using them as a system wakeup source. The reasons are very
similar.
Basically the standard gives us no way to ask a device what constitutes
a wakeup event for it. We just get the blanket statement of support
or no support.
For runtime PM I would want my mouse to generate a remote wakeup
whenever a button is pressed or the mouse is moved. Under this
premise runtime PM with a mouse works wonderfully. Testing that,
however, is a challenge.
It turns out that mice that claim that they support remote wakeup
by and large deactivate their LED/laser when you send them into
suspension. Hence they react only to buttons being pressed or mouse
wheels moved.
As a system wakeup source a mouse that generates events when
it is moved, however, would make the system unsuspendable, whenever even
a bit of vibration is acting on the system.
And as S4 is used in many setups to prevent an uncontrolled shutdown
at low power, this must work.
I suspect that most, but _not_ _all_ mice, are designed for use
with a system that ties power management to the screen saver.
That is a mode of operation that due to the lack of coupling
between GUI and kernel is hard to copy.
Frankly given these constraints the only default safe in every
case seems to me to leave the kernel's default at off and
put the work into udev's hwdb. Not that that is a good solution,
merely the best I can come up with.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] HID: usbhid: enable remote wakeup for mice
From: Oliver Neukum @ 2023-02-23 11:47 UTC (permalink / raw)
To: Michael Wu, Oliver Neukum, jikos, benjamin.tissoires
Cc: linux-usb, linux-input, linux-kernel, richard.gong,
mario.limonciello
In-Reply-To: <6d7c8e3f-225e-400f-f8a4-bf14c538ad8a@allwinnertech.com>
On 23.02.23 12:22, Michael Wu wrote:
> Yes. I can't use any USB mouse which supports Remote Wakeup to wake up the system by default.
> If I enable the wakeup node in /sys/bus/usb/devices which is disabled by default, the mouse can wakeup the system successfully.
> Clearly, the only thing I can do is to register the wakeup source for USB Mouse devices.
>
Hi,
thank you for answering this. I feel a long discussion coming up,
which I would avoid if I could, so it is important that we are
clear and do not discuss about a misunderstanding.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] HID: usbhid: enable remote wakeup for mice
From: Oliver Neukum @ 2023-02-23 11:41 UTC (permalink / raw)
To: Limonciello, Mario, Greg KH, Michael Wu, Richard Gong
Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel
In-Reply-To: <c99a7ae3-b8ab-30e2-67bc-87b5a7736728@amd.com>
On 22.02.23 20:50, Limonciello, Mario wrote:
> Richard and I both sent out relatively similar patches in the past, but they never went anywhere.
The problem here is that the default will never satisfy a great majority. just a majority.
So if we admit that there is no optimal solution, why not pick the tested,
that is, current solution, as long as it does least?
> We did confirm that Windows does set a similar policy as well though, which is what prompted us to attempt this.
>
> As there is more interest again maybe we can revive that discussion and merge together some ideas from the sets of patches.
Well, the most obvious question, as this can also be done with a udev rule, is,
what are the risks of a change?
> Previous submissions:
>
> v4:
> https://lore.kernel.org/linux-usb/20220825045517.16791-1-mario.limonciello@amd.com/
This one at least restricts it to some cases. And as much as it pains me,
we need to ask whether there are risks in not emulating Windows.
And on the other hand, what happens if we do this. Are you sure, for example,
that you do not break use cases for S4 with this change?
If a laptop goes into S4 because power is too low, we really do not want it
to wake up just because somebody accidentally hits a mouse button.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH] HID: usbhid: enable remote wakeup for mice
From: Greg KH @ 2023-02-23 11:23 UTC (permalink / raw)
To: Michael Wu
Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel,
mario.limonciello, richard.gong
In-Reply-To: <9bf4463c-6541-a6cb-9bbc-6d070118509a@allwinnertech.com>
On Thu, Feb 23, 2023 at 07:18:12PM +0800, Michael Wu wrote:
> Dear Greg,
>
> On 2/22/2023 2:04 PM, Greg KH wrote:
> > On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
> > > This patch fixes a problem that USB mouse can't wake up the device that
> > > enters standby.
> >
> > This not a problem, it is that way by design.
>
> I got it, maybe it's a little problem to say that.
It is.
> > > At present, the kernel only checks whether certain USB manufacturers
> > > support wake-up, which will easily cause inconvenience to the
> > > development work of other manufacturers and add unnecessary work to the
> > > maintenance of kernel.
> > >
> > > The USB protocol supports judging whether a usb supports the wake-up
> > > function, so it should be more reasonable to add a wake-up source by
> > > directly checking the settings from the USB protocol.
> >
> > But you do not do that in this patch, why not?
>
> I just want to explain the background of my patch, to prove we could use a
> similar way to avoid such a "disturbing" situation.
> To reduce the influence, my patch enables remote wakeup for USB mouse
> devices refer to what keyboard do.
Keyboards are not mice :)
> > > There was a similar issue on the keyboard before, which was fixed by
> > > this patch (3d61510f4eca), but now the problem happened on the mouse.
> > > This patch uses a similar idea to fix this problem.
> > >
> > > Signed-off-by: Michael Wu <michael@allwinnertech.com>
> > > ---
> > > drivers/hid/usbhid/hid-core.c | 8 ++++++++
> > > drivers/hid/usbhid/usbmouse.c | 1 +
> > > 2 files changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > index be4c731aaa65..d3a6755cca09 100644
> > > --- a/drivers/hid/usbhid/hid-core.c
> > > +++ b/drivers/hid/usbhid/hid-core.c
> > > @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
> > > device_set_wakeup_enable(&dev->dev, 1);
> > > }
> > > + /**
> > > + * NOTE: enable remote wakeup by default for all mouse devices
> > > + * supporting the boot protocol.
> > > + */
> > > + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
> > > + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
> > > + device_set_wakeup_enable(&dev->dev, 1);
> >
> > Sorry, but we can not take this unless it is proven that this will work
> > properly for all of these devices. Other operating systems do not do
> > this last I checked, so there will be problems.
>
> As Mario Limonciello says, they has confirmed that the Microsoft Windows
> does set a similar policy as well. Can we talk about more in this topic: why
> does Linux not support it?
> Of course, if you have other great idea, I will appreciate that if we can
> have some further discussion.
You need to provide some sort of "proof" that this has been heavily
tested on a huge range of devices before we can change this.
When this was first implemented, Windows did not work this way and many
devices on the market were broken if this were to be enabled. I'm sure
the mailing list archives from 20+ years ago have many more details,
please dig around there for specifics.
If you feel strongly that this is the way forward, why not do it in
userspace today for your systems as part of testing this out? It should
not require a kernel change, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] HID: usbhid: enable remote wakeup for mice
From: Michael Wu @ 2023-02-23 11:22 UTC (permalink / raw)
To: Oliver Neukum, jikos, benjamin.tissoires
Cc: linux-usb, linux-input, linux-kernel, richard.gong,
mario.limonciello
In-Reply-To: <ef80c145-e33f-4aa8-c9e8-c77f34b1bb0e@suse.com>
Dear Oliver,
On 2/22/2023 5:34 PM, Oliver Neukum wrote:
> On 22.02.23 02:39, Michael Wu wrote:
>> This patch fixes a problem that USB mouse can't wake up the device that
>> enters standby.
>
> Hi,
>
> I am afraid I need to ask you to be a bit more precise here.
> Are you referring to USB mice being unable to wake up a system,
> even if you so request via sysfs, or that they won't by default?
>
> Regards
> Oliver
Yes. I can't use any USB mouse which supports Remote Wakeup to wake up
the system by default.
If I enable the wakeup node in /sys/bus/usb/devices which is disabled by
default, the mouse can wakeup the system successfully.
Clearly, the only thing I can do is to register the wakeup source for
USB Mouse devices.
--
Regards,
Michael Wu
^ permalink raw reply
* Re: [PATCH] HID: usbhid: enable remote wakeup for mice
From: Michael Wu @ 2023-02-23 11:18 UTC (permalink / raw)
To: Greg KH
Cc: jikos, benjamin.tissoires, linux-usb, linux-input, linux-kernel,
mario.limonciello, richard.gong
In-Reply-To: <Y/WwXBF37hoZBbQa@kroah.com>
Dear Greg,
On 2/22/2023 2:04 PM, Greg KH wrote:
> On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
>> This patch fixes a problem that USB mouse can't wake up the device that
>> enters standby.
>
> This not a problem, it is that way by design.
I got it, maybe it's a little problem to say that.
>
>> At present, the kernel only checks whether certain USB manufacturers
>> support wake-up, which will easily cause inconvenience to the
>> development work of other manufacturers and add unnecessary work to the
>> maintenance of kernel.
>>
>> The USB protocol supports judging whether a usb supports the wake-up
>> function, so it should be more reasonable to add a wake-up source by
>> directly checking the settings from the USB protocol.
>
> But you do not do that in this patch, why not?
I just want to explain the background of my patch, to prove we could use
a similar way to avoid such a "disturbing" situation.
To reduce the influence, my patch enables remote wakeup for USB mouse
devices refer to what keyboard do.
>
>> There was a similar issue on the keyboard before, which was fixed by
>> this patch (3d61510f4eca), but now the problem happened on the mouse.
>> This patch uses a similar idea to fix this problem.
>>
>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>> ---
>> drivers/hid/usbhid/hid-core.c | 8 ++++++++
>> drivers/hid/usbhid/usbmouse.c | 1 +
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index be4c731aaa65..d3a6755cca09 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>> device_set_wakeup_enable(&dev->dev, 1);
>> }
>>
>> + /**
>> + * NOTE: enable remote wakeup by default for all mouse devices
>> + * supporting the boot protocol.
>> + */
>> + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
>> + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
>> + device_set_wakeup_enable(&dev->dev, 1);
>
> Sorry, but we can not take this unless it is proven that this will work
> properly for all of these devices. Other operating systems do not do
> this last I checked, so there will be problems.
As Mario Limonciello says, they has confirmed that the Microsoft Windows
does set a similar policy as well. Can we talk about more in this topic:
why does Linux not support it?
Of course, if you have other great idea, I will appreciate that if we
can have some further discussion.
>
> thanks,
>
> greg k-h
--
Regards,
Michael Wu
^ permalink raw reply
* Re: [PATCH v2 1/2] HID: core: Provide new max_buffer_size attribute to over-ride the default
From: Jiri Kosina @ 2023-02-23 10:57 UTC (permalink / raw)
To: Lee Jones; +Cc: benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <20230123123912.360651-1-lee@kernel.org>
On Mon, 23 Jan 2023, Lee Jones wrote:
> Presently, when a report is processed, its proposed size, provided by
> the user of the API (as Report Size * Report Count) is compared against
> the subsystem default HID_MAX_BUFFER_SIZE (16k). However, some
> low-level HID drivers allocate a reduced amount of memory to their
> buffers (e.g. UHID only allocates UHID_DATA_MAX (4k) buffers), rending
> this check inadequate in some cases.
>
> In these circumstances, if the received report ends up being smaller
> than the proposed report size, the remainder of the buffer is zeroed.
> That is, the space between sizeof(csize) (size of the current report)
> and the rsize (size proposed i.e. Report Size * Report Count), which can
> be handled up to HID_MAX_BUFFER_SIZE (16k). Meaning that memset()
> shoots straight past the end of the buffer boundary and starts zeroing
> out in-use values, often resulting in calamity.
>
> This patch introduces a new variable into 'struct hid_ll_driver' where
> individual low-level drivers can over-ride the default maximum value of
> HID_MAX_BUFFER_SIZE (16k) with something more sympathetic to the
> interface.
>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> v1 => v2:
> - Edit the commit message to be less focused on UHID
Now applied to hid.git#for-6.3/upstream-fixes. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/2] HID: logitech-hidpp: Add support for the Pro X Superlight
From: Bastien Nocera @ 2023-02-23 9:30 UTC (permalink / raw)
To: Mavroudis Chatzilazaridis, linux-input; +Cc: jikos, benjamin.tissoires, lains
In-Reply-To: <20230222222800.83077-2-mavchatz@protonmail.com>
On Wed, 2023-02-22 at 22:29 +0000, Mavroudis Chatzilazaridis wrote:
> This patch adds support for the Pro X Superlight. Tested over USB.
>
> Co-developed-by: Filipe Laíns <lains@riseup.net>
> Signed-off-by: Filipe Laíns <lains@riseup.net>
> Signed-off-by: Mavroudis Chatzilazaridis <mavchatz@protonmail.com>
This patch is:
Reviewed-by: Bastien Nocera <hadess@hadess.net>
> ---
> drivers/hid/hid-logitech-hidpp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> logitech-hidpp.c
> index 9c1ee8e91e0c..62683a712f8e 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4350,6 +4350,8 @@ static const struct hid_device_id
> hidpp_devices[] = {
> .driver_data = HIDPP_QUIRK_CLASS_G920 |
> HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
> { /* Logitech G Pro Gaming Mouse over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
> + { /* Logitech G Pro X Superlight Gaming Mouse over USB */
> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC094) },
>
> { /* MX5000 keyboard over Bluetooth */
> HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
> --
> 2.34.1
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox