Bug 215699 - bug in HID++ driver that can cause incorrect multiplier for scrolling
Summary: bug in HID++ driver that can cause incorrect multiplier for scrolling
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Input Devices (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_input-devices
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-03-18 14:57 UTC by Peter F. Patel-Schneider
Modified: 2023-02-03 16:30 UTC (History)
1 user (show)

See Also:
Kernel Version: 5.16.14-200.fc35.x86_64
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Peter F. Patel-Schneider 2022-03-18 14:57:47 UTC
I was looking into problems with slow and fast scrolling with my Logitech MX Master 3 mouse and noticed a some strange responses from it. Here is a trace of the messages it sends when it is turned on and interpretations of them.  (I can't see what is sent to the mouse.)  I'm using the hidconsole tool from Solaar.

>> (  60.317) [11 01 0400 01010100000000000000000000000000]
>> b'\x11\x01\x04\x00\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	WIRELESS DEVICE STATUS notification asking for reconfiguration
>> (  60.327) [11 01 0800 64320000000000000000000000000000]
>> b'\x11\x01\x08\x00d2\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	BATTERY STATUS Level Status notification or response 
>> (  60.343) [11 01 0800 64320000000000000000000000000000]
>> b'\x11\x01\x08\x00d2\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	BATTERY STATUS Level Status response or notification
>> (  60.351) [11 01 0810 0404C078050000000000000000000000]
>> b'\x11\x01\x08\x10\x04\x04\xc0x\x05\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	BATTERY STATUS Capability reponse (but with a zero software ID field)
>> (  60.375) [11 01 0001 0E000100000000000000000000000000]
>> b'\x11\x01\x00\x01\x0e\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	ROOT Feature response for index 0E with a one sofware ID field
>> (  60.399) [11 01 0E20 02000000000000000000000000000000] b'\x11\x01\x0e
>> \x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	HIRES WHEEL Set Mode response or Movement notification (but has to be a Mode response with a zero software ID field)
>> (  60.423) [11 01 0001 0E000100000000000000000000000000]
>> b'\x11\x01\x00\x01\x0e\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	ROOT Feature response for index 0E with a one sofware ID field
>> (  60.447) [11 01 0E00 0F1C1818000000000000000000000000]
>> b'\x11\x01\x0e\x00\x0f\x1c\x18\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
	HIRES WHEEL Capability response (but with a zero software ID field)

A partial specification of the protocol being used is at https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf
It says that the lower four bits of the fourth byte of these messages is a sofware identifier and further says:

Software identifier (4 bits, unsigned)
A number uniquely defining the software that sends a request. The firmware must copy the software identifier in the
response but does not use it in any other ways.
0 Do not use (allows to distinguish a notification from a response).

The messages whose third byte is 0x0E on my mouse is for the Logitech HID++ 2.0 HiRes Wheel feature. (These devices have a mapping from Logitech feature numbers to feature indexes and the mouse maps the HIRES WHEEL feature 0x2121 to index 0x0E) The specification for this feature is at https://lekensteyn.nl/files/logitech/x2121_hires_wheel.pdf

The biggest problem that I see is that a HIRES WHEEL Movement notification could be mistaken for HIRES WHEEL Capability response and completely mess up the calculation of the multiplier in hidpp_hrw_get_wheel_capability.

Here are the constants that are used for the fourth byte of commands, combining the four-bit command and the four-bit software identifier.

#define CMD_ROOT_GET_FEATURE				0x01
#define CMD_ROOT_GET_PROTOCOL_VERSION			0x11

#define CMD_GET_DEVICE_NAME_TYPE_GET_COUNT		0x01
#define CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME	0x11
#define CMD_GET_DEVICE_NAME_TYPE_GET_TYPE		0x21

#define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_LEVEL_STATUS	0x00
#define CMD_BATTERY_LEVEL_STATUS_GET_BATTERY_CAPABILITY		0x10

#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00

#define CMD_UNIFIED_BATTERY_GET_CAPABILITIES			0x00
#define CMD_UNIFIED_BATTERY_GET_STATUS				0x10

#define CMD_HI_RESOLUTION_SCROLLING_SET_HIGHRES_SCROLLING_MODE	0x10

#define CMD_HIRES_WHEEL_GET_WHEEL_CAPABILITY	0x00
#define CMD_HIRES_WHEEL_SET_WHEEL_MODE		0x20

#define CMD_SOLAR_SET_LIGHT_MEASURE			0x00

#define CMD_TOUCHPAD_FW_ITEMS_SET			0x10

#define CMD_TOUCHPAD_GET_RAW_INFO			0x01
#define CMD_TOUCHPAD_SET_RAW_REPORT_STATE		0x21

#define HIDPP_FF_GET_INFO		0x01
#define HIDPP_FF_RESET_ALL		0x11
#define HIDPP_FF_DOWNLOAD_EFFECT	0x21
#define HIDPP_FF_SET_EFFECT_STATE	0x31
#define HIDPP_FF_DESTROY_EFFECT		0x41
#define HIDPP_FF_GET_APERTURE		0x51
#define HIDPP_FF_SET_APERTURE		0x61
#define HIDPP_FF_GET_GLOBAL_GAINS	0x71
#define HIDPP_FF_SET_GLOBAL_GAINS	0x81

It appears to me that all these should never end in a 0, and probably should all end in a 1.

What is a good way to have this change made?

peter
Comment 1 Peter F. Patel-Schneider 2022-03-18 15:01:08 UTC
Oops, forgot to mention that the driver in question is https://github.com/torvalds/linux/blob/master/drivers/hid/hid-logitech-hidpp.c
Comment 2 Bastien Nocera 2022-09-14 14:18:24 UTC
Should be fixed by https://lore.kernel.org/linux-input/20220830132549.7240-4-hadess@hadess.net/T/#u
Comment 3 Peter F. Patel-Schneider 2022-09-14 14:40:26 UTC
Yes, thanks.
Comment 4 Bastien Nocera 2023-02-03 16:30:09 UTC
This was fixed in f7b7393cc3b04e288de85790b1c8e62af99799c2, 0799617f3809d9f096fa18942391ef98ebb023b7 and 8b7e58409b1813c58eea542d9f3b8db35b4ac1f7.

Note You need to log in before you can comment on or make changes to this bug.