linux-kernel - Re: [PATCH] HID: logitech-hidpp: add support for Logitech G533 headset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ad2e18d37772bed00f1fd7c8a62ce4f98d69fe8.camel@archlinux.org>
Date:   Sun, 17 May 2020 16:52:39 +0100
From:   Filipe Laíns <lains@...hlinux.org>
To:     Kamil Domański <kamil@...anski.co>,
        linux-kernel@...r.kernel.org
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Nestor Lopez Casado <nlopezcasad@...itech.com>
Subject: Re: [PATCH] HID: logitech-hidpp: add support for Logitech G533
 headset

On Sun, 2020-04-26 at 17:18 +0200, Kamil Domański wrote:

*snip*

> +/* -------------------------------------------------------------------------- */
> +/* 0x1F20: Analog-digital converter measurement                               */
> +/* -------------------------------------------------------------------------- */
> +
> +#define HIDPP_PAGE_ADC_MEASUREMENT 0x1F20
> +
> +#define CMD_ADC_MEASUREMENT_GET_VOLTAGE 0x01
> +
> +/**
> + * hidpp20_adc_map_status_voltage() - convert HID++ code to power supply status
> + * @hidpp: HID++ device struct.
> + * @data: ADC report data.
> + * @voltage: Pointer to variable where the ADC voltage shall be written.
> + *
> + * This function decodes the ADC voltage and charge status
> + * of the device's battery.
> + *
> + * Return: Returns the power supply charge status code.
> + */
> +static int hidpp20_adc_map_status_voltage(struct hidpp_device *hidpp,
> +						u8 data[3], int *voltage)
> +{
> +	bool isConnected;
> +	bool isCharging;
> +	bool chargingComplete;
> +	bool chargingFault;

We use snake case.

> +
> +	long flags = (long) data[2];

Use u8 instead. Why are we even using a variable for this?

> +
> +	*voltage = get_unaligned_be16(data);
> +	isConnected = test_bit(0, &flags);
> +	isCharging = test_bit(1, &flags);
> +	chargingComplete = test_bit(2, &flags);
> +	chargingFault = test_bit(3, &flags);

I don't think this is needed, just do it in the ifs directly.

Here I would add a #define for each bit:

#define FLAG_ADC_MAP_STATUS_CONNECTED 0
...
if (data[2] & FLAG_ADC_MAP_STATUS_CONNECTED)

> +
> +	if (!isConnected)
> +		return POWER_SUPPLY_STATUS_UNKNOWN;
> +
> +	if (chargingFault)
> +		return POWER_SUPPLY_STATUS_NOT_CHARGING;

From the spec:
> Only valid if 'isCharging' is 1.

> +
> +	if (chargingComplete)
> +		return POWER_SUPPLY_STATUS_FULL;

From the spec:
> Only valid if 'isCharging' is 1.

> +
> +	if (isCharging)
> +		return POWER_SUPPLY_STATUS_CHARGING;

Put the two previous checks inside this if.

> +	return POWER_SUPPLY_STATUS_DISCHARGING;
> +}
> 

*snip*

> @@ -3994,6 +4189,8 @@ static const struct hid_device_id hidpp_devices[] = {
>  
>  	{ /* Logitech G403 Wireless Gaming Mouse over USB */
>  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
> +	{ /* Logitech G533 Wireless Headset over USB */
> +	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0A66) },

Is this a receiver? If so, we need to know if it is used by other
devices. Or better, see if it supports the DJ protocol and add it to
hid-logitech-dj instead.

>  	{ /* Logitech G703 Gaming Mouse over USB */
>  	  HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
>  	{ /* Logitech G703 Hero Gaming Mouse over USB */

Cheers,
Filipe Laíns

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)