[RFC v1 1/2] HID: logitech-hidpp: Fix "Sw. Id." for HID++ 2.0 commands

[RFC v1 1/2] HID: logitech-hidpp: Fix "Sw. Id." for HID++ 2.0 commands

[Bastien Nocera]

Always set a non-zero "Sw. Id." in the lower nibble of the Function/ASE
and Software Identifier byte in HID++ 2.0 commands.

As per the "Protocol HID++2.0 essential features" section in
https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf
"
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).
"

Link: https://bugzilla.kernel.org/show_bug.cgi?id=215699
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/hid/hid-logitech-hidpp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 86e7a38d8a9a..02f8c99672c7 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644);
 MODULE_PARM_DESC(disable_tap_to_click,
 	"Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently).");
 
+/* Define a non-zero software ID to identify our own requests */
+#define LINUX_KERNEL_SW_ID			0x06
+
 #define REPORT_ID_HIDPP_SHORT			0x10
 #define REPORT_ID_HIDPP_LONG			0x11
 #define REPORT_ID_HIDPP_VERY_LONG		0x12
@@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
 	else
 		message->report_id = REPORT_ID_HIDPP_LONG;
 	message->fap.feature_index = feat_index;
-	message->fap.funcindex_clientid = funcindex_clientid;
+	message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID;
 	memcpy(&message->fap.params, params, param_count);
 
 	ret = hidpp_send_message_sync(hidpp, message, response);
-- 
2.37.2

[RFC v1 2/2] HID: logitech-hidpp: Remove hard-coded "Sw. Id." for HID++ 2.0 commands

[Bastien Nocera]

Some HID++ 2.0 commands had correctly set a non-zero software identifier
directly as part of their function identifiers, but it's more correct to
define the function identifier and the software identifier separately
before combined them when the command is sent.

As this is now done in the previous commit, remove the hard-coded 0x1
software identifiers in the function definitions.

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/hid/hid-logitech-hidpp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 02f8c99672c7..46b3e51cb854 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -859,8 +859,8 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
 #define HIDPP_PAGE_ROOT					0x0000
 #define HIDPP_PAGE_ROOT_IDX				0x00
 
-#define CMD_ROOT_GET_FEATURE				0x01
-#define CMD_ROOT_GET_PROTOCOL_VERSION			0x11
+#define CMD_ROOT_GET_FEATURE				0x00
+#define CMD_ROOT_GET_PROTOCOL_VERSION			0x10
 
 static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16 feature,
 	u8 *feature_index, u8 *feature_type)
@@ -937,9 +937,9 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
 
 #define HIDPP_PAGE_GET_DEVICE_NAME_TYPE			0x0005
 
-#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_GET_DEVICE_NAME_TYPE_GET_COUNT		0x00
+#define CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME	0x10
+#define CMD_GET_DEVICE_NAME_TYPE_GET_TYPE		0x20
 
 static int hidpp_devicenametype_get_count(struct hidpp_device *hidpp,
 	u8 feature_index, u8 *nameLength)
@@ -1969,8 +1969,8 @@ static int hidpp_touchpad_fw_items_set(struct hidpp_device *hidpp,
 
 #define HIDPP_PAGE_TOUCHPAD_RAW_XY			0x6100
 
-#define CMD_TOUCHPAD_GET_RAW_INFO			0x01
-#define CMD_TOUCHPAD_SET_RAW_REPORT_STATE		0x21
+#define CMD_TOUCHPAD_GET_RAW_INFO			0x00
+#define CMD_TOUCHPAD_SET_RAW_REPORT_STATE		0x20
 
 #define EVENT_TOUCHPAD_RAW_XY				0x00
 
-- 
2.37.2

Re: [RFC v1 1/2] HID: logitech-hidpp: Fix "Sw. Id." for HID++ 2.0 commands

[Peter F. Patel-Schneider]

On 8/29/22 09:48, Bastien Nocera wrote:
> Always set a non-zero "Sw. Id." in the lower nibble of the Function/ASE
> and Software Identifier byte in HID++ 2.0 commands.
>
> As per the "Protocol HID++2.0 essential features" section in
> https://lekensteyn.nl/files/logitech/logitech_hidpp_2.0_specification_draft_2012-06-04.pdf
> "
> 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).
> "
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215699
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>   drivers/hid/hid-logitech-hidpp.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 86e7a38d8a9a..02f8c99672c7 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -41,6 +41,9 @@ module_param(disable_tap_to_click, bool, 0644);
>   MODULE_PARM_DESC(disable_tap_to_click,
>   	"Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently).");
>   
> +/* Define a non-zero software ID to identify our own requests */
> +#define LINUX_KERNEL_SW_ID			0x06
> +
>   #define REPORT_ID_HIDPP_SHORT			0x10
>   #define REPORT_ID_HIDPP_LONG			0x11
>   #define REPORT_ID_HIDPP_VERY_LONG		0x12
> @@ -343,7 +346,7 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
>   	else
>   		message->report_id = REPORT_ID_HIDPP_LONG;
>   	message->fap.feature_index = feat_index;
> -	message->fap.funcindex_clientid = funcindex_clientid;
> +	message->fap.funcindex_clientid = funcindex_clientid | LINUX_KERNEL_SW_ID;
>   	memcpy(&message->fap.params, params, param_count);
>   
>   	ret = hidpp_send_message_sync(hidpp, message, response);



Looks good to me.  It might be better to use ID 0x01 to signifiy the "first" 
software but that is a minor quibble.


peter

Re: [RFC v1 2/2] HID: logitech-hidpp: Remove hard-coded "Sw. Id." for HID++ 2.0 commands

[Peter F. Patel-Schneider]

On 8/29/22 09:48, Bastien Nocera wrote:
> Some HID++ 2.0 commands had correctly set a non-zero software identifier
> directly as part of their function identifiers, but it's more correct to
> define the function identifier and the software identifier separately
> before combined them when the command is sent.
>
> As this is now done in the previous commit, remove the hard-coded 0x1
> software identifiers in the function definitions.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>   drivers/hid/hid-logitech-hidpp.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 02f8c99672c7..46b3e51cb854 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -859,8 +859,8 @@ static int hidpp_unifying_init(struct hidpp_device *hidpp)
>   #define HIDPP_PAGE_ROOT					0x0000
>   #define HIDPP_PAGE_ROOT_IDX				0x00
>   
> -#define CMD_ROOT_GET_FEATURE				0x01
> -#define CMD_ROOT_GET_PROTOCOL_VERSION			0x11
> +#define CMD_ROOT_GET_FEATURE				0x00
> +#define CMD_ROOT_GET_PROTOCOL_VERSION			0x10
>   
>   static int hidpp_root_get_feature(struct hidpp_device *hidpp, u16 feature,
>   	u8 *feature_index, u8 *feature_type)
> @@ -937,9 +937,9 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
>   
>   #define HIDPP_PAGE_GET_DEVICE_NAME_TYPE			0x0005
>   
> -#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_GET_DEVICE_NAME_TYPE_GET_COUNT		0x00
> +#define CMD_GET_DEVICE_NAME_TYPE_GET_DEVICE_NAME	0x10
> +#define CMD_GET_DEVICE_NAME_TYPE_GET_TYPE		0x20
>   
>   static int hidpp_devicenametype_get_count(struct hidpp_device *hidpp,
>   	u8 feature_index, u8 *nameLength)
> @@ -1969,8 +1969,8 @@ static int hidpp_touchpad_fw_items_set(struct hidpp_device *hidpp,
>   
>   #define HIDPP_PAGE_TOUCHPAD_RAW_XY			0x6100
>   
> -#define CMD_TOUCHPAD_GET_RAW_INFO			0x01
> -#define CMD_TOUCHPAD_SET_RAW_REPORT_STATE		0x21
> +#define CMD_TOUCHPAD_GET_RAW_INFO			0x00
> +#define CMD_TOUCHPAD_SET_RAW_REPORT_STATE		0x20
>   
>   #define EVENT_TOUCHPAD_RAW_XY				0x00
>   



Looks good to me.


peter