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 for Android: free password hash cracker in your pocket

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

Message-ID: <908bf287f370a947e60bc7817b395c91c5be63d9.camel@archlinux.org>
Date:   Sat, 04 Jul 2020 15:48:30 +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 v2] HID: logitech-hidpp: add support for Logitech G533
 headset

On Sat, 2020-07-04 at 02:37 +0200, Kamil Domański wrote:
> Hi Filipe,
> 
> > 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.

The compiler can handle these conversions. Explicit is generally better
than implicit, and in cases where the implicit assumptions might break,
explicit is definitely better. We know the data size, so let's use it
:D

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

Thanks for looking into this!

Cheers,
Filipe Laíns

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