linux-kernel - Re: [PATCH v2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <52609283-6d37-9e0e-be24-7df378e4fead@domanski.co>
Date:   Sat, 4 Jul 2020 02:37:36 +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 v2] HID: logitech-hidpp: add support for Logitech G533
 headset

Hi Filipe,

>> +	bool isConnected;
>> +	bool isCharging;
>> +	bool chargingComplete;
>> +	bool chargingFault;
> 
> From my initial comments:
> 
>> We use snake case.

Will be fixed in v3.

>> +
>> +	long flags = (long) data[2];
> 
>> Use u8 instead. Why are we even using a variable for this?
> 
> My main point here is that long means different things in different
> architectures, and we only want one byte so I would go for u8.

I used long, because the test_bit macro accepts long and the similar
function for voltage reading already used long too.
That will be changed in v3 - see next paragraph.

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

Yeah, I it will do exactly that for v3, which allows to drop the flag
variables and avoid using a long.

 
> Same thing here. We should see if the device supports the DJ protocol
> and add it in hid-logitech-dj instead.

It doesn't seem to be a DJ device. The DJ driver just detects the extra interfaces
and skips directly to hid_hw_start.

Regards,
Kamil