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  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]
Date:   Sat, 4 Jul 2020 02:37:36 +0200
From:   Kamil Domański <>
To:     Filipe Laíns <>,
Cc:     Jiri Kosina <>,
        Benjamin Tissoires <>,
        Nestor Lopez Casado <>
Subject: Re: [PATCH v2] HID: logitech-hidpp: add support for Logitech G533

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

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.


Powered by blists - more mailing lists