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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1224b05db4ff4b7288ee05ca8062af3bd574531d.camel@domanski.co>
Date:   Sun, 17 May 2020 18:23:14 +0200
From:   Kamil Domański <kamil@...anski.co>
To:     Filipe Laíns <lains@...hlinux.org>,
        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-05-17 at 16:52 +0100, Filipe Laíns wrote:
> 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.

Will fix, thanks.

> > +
> > +	long flags = (long) data[2];
> 
> Use u8 instead. Why are we even using a variable for this?

I is analogous to how it's done in existing code for feature 0x1001 (voltage).
Besides that IMHO it greatly improves readability and the asignment
will be optimized out anyways.

> > +
> > +	*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.

IMHO it improves readability.
The asignments will be optimized out anyways.

> Here I would add a #define for each bit:
> 
> #define FLAG_ADC_MAP_STATUS_CONNECTED 0
> ...
> if (data[2] & FLAG_ADC_MAP_STATUS_CONNECTED)

Again, I did it analogous to existing code. I'll be glad to change
it (and perhaps in the existing code as well) if the others concur.

> > +
> > +	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;
> > +}
> > 

Thanks for pointing it out. I'll fix it.

> > @@ -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.

I frankly don't think it supports anything else since it self-identifies
as "Logitech, Inc. [G533 Wireless Headset Dongle]" and it only returns
feature indexes for this one headset, whether it's paired or not.
It also registers a USB audio device immediately, even if the headset
is not connected.

Thanks for all the help.

Best regards,
Kamil Domański