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  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]
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)

Powered by blists - more mailing lists