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
| ||
|
Date: Sat, 11 Nov 2017 18:40:02 +0200 From: Marcus Wolf <marcus.wolf@...rthome-wolf.de> To: Joe Perches <joe@...ches.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org, dan.carpenter@...cle.com Subject: Re: staging: pi433: Possible bug in rf69.c Hi Joe, thank you for your suggestion. The enums are necessary for the (old fashioned) ioctl interface, too. So the user space uses these enums in order to configure the driver. If we want to completely remove rf69_enum.h, we need to find a solution for that, too. From the optics/readability, I like your idea with the Macro for the cases. On the other hand, I have already prepared a patch, that uses setbit, resetbit and readmodifywrite inline fuctions instead of the macros WRITE_REG, ... That was an idea of Walter Harms in order to increase readability and reduce macros, because Walter prefers inline functions to macros. As discussed with Greg, I will provide the patch, as soon as 4.15rc1 is out. Maybe we should move the discussion to then, so you can have a look to that? Cheers, Marcus Am 11.11.2017 um 18:02 schrieb Joe Perches: > On Fri, 2017-11-10 at 18:23 +0100, Marcus Wolf wrote: >> Hi everybody! >> >> Just comparing the master of Gregs statging of pi433 with my local SVN >> to review all changes, that were done the last monthes. >> >> I am not sure, but maybe we imported a bug in rf69.c lines 378 and >> following: >> >> Gregs repo: >> case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) ); >> case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_MAX) ); >> case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) ); >> case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) ); >> case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) ); >> case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) ); >> case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) ); >> >> my repo: >> case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) ); >> case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_MAX) ); >> case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) ); >> case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) ); >> case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) ); >> case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) ); >> case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & >> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) ); >> >> Up to my opinion, my (old) version is better then Gregs (new) version. >> If you agree, I'll prepare a patch, to revert the modification. > > There seems to be a lot of enum/#define duplication in this driver. > > For instance: > > drivers/staging/pi433/rf69_registers.h > > #define LNA_GAIN_AUTO 0x00 /* default */ > #define LNA_GAIN_MAX 0x01 > #define LNA_GA > IN_MAX_MINUS_6 0x02 > #define LNA_GAIN_MAX_MINUS_12 > 0x03 > #define LNA_GAIN_MAX_MINUS_24 > 0x04 > #define LNA_GAIN_MAX_MINUS_36 0x05 > #d > efine LNA_GAIN_MAX_MINUS_48 0x06 > > vs > > drivers/staging/pi433/rf69_enum.h > > enum lnaGain > { > automatic, > max, > maxMinus6, > maxMinus12, > maxM > inus24, > maxMinus36, > maxMinus48, > undefined > }; > > My suggestion would be to remove drivers/staging/pi433/rf69_enum.h > where possible and convert all these switch/case entries into > macros like > > #define GAIN_CASE(type) \ > case type: return WRITE_REG(REG_LNA, \ > (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) | (type)); > > so for example this switch becomes > > switch (lnaGain) { > GAIN_CASE(LNA_GAIN_AUTO); > ... > } >
Powered by blists - more mailing lists