linux-kernel - RE: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values

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]

Date:	Fri, 20 May 2016 17:52:25 +0000
From:	Hartley Sweeten <HartleyS@...ionengravers.com>
To:	Ian Abbott <abbotti@....co.uk>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI
 9080 LASxRR values

On Friday, May 20, 2016 10:18 AM, Ian Abbott wrote:
> On 20/05/16 17:37, Hartley Sweeten wrote:
>> On Friday, May 20, 2016 6:49 AM, Ian Abbott wrote:
>>> Rename the macros for the PLX PCI 9080 LAS0RR and LAS1RR registers in
>>> "plx9080.h", using the prefix `PLX_LASRR_`.  Make use of the `BIT(x)`
>>> and `GENMASK(h,l)` macros to define the values.
>>>
>>> Define a macro `PLX_LASRR_PREFETCH` for the "prefetchable memory" bit in
>>> this register, and define a macro `PLX_LASRR_MLOC_MASK` to mask the PCI
>>> memory location control bits.

[snip]

>>> +#define PLX_LASRR_IO		BIT(0)		/* Map to: 1=I/O, 0=Mem */
>>> +#define PLX_LASRR_ANY32		(BIT(1) * 0)	/* Locate anywhere in 32 bit */
>>> +#define PLX_LASRR_LT1MB		(BIT(1) * 1)	/* Locate in 1st meg */
>>> +#define PLX_LASRR_ANY64		(BIT(1) * 2)	/* Locate anywhere in 64 bit */
>>
>> The (BIT(n) * x) looks ugly.
>
> You won't like the remaining patches then!

You are correct... ;-)

> FWIW, all the constants end up with the same type (unsigned long) this way.

That's probably good but it sure makes the defines look ugly, and a bit hard to
understand imoh. You also don't know what the 'max' value for the bit-field
is without further digging.

I applied your whole series to see what the final header looks like. To me it
actually looks worse than the original.

The original had a number of whitespace issues that made it hard to follow and
the defines were lacking namespace. Personally I also don't can for all the enums
since the symbols are not actually used as enums just as raw values. But the 'bit'
usage of the registers was fairly clear.

With your series applied the whtespace and namespace issues are addressed.
You also converted all the enums to defines which is great. But the 'bit' usage
now is a bit muddled.  I really don't care for the (BIT(n) * (x)) stuff. There are
also the various, unused and unnecessary, <foo>_SHIFT defines. Those just
add additional cruft.

I'm also not sure if all the bits require a comment. They seem to clutter the
header. Datasheets for the PLX-9080 are easy to find. Maybe just have a
comment for each register and remove all the bit comments.

> I have been looking for a solution to the problem where random people 
> change something like this:
>
> #define MY_COOLREG_VAL_FOO	(0 << 5)
> #define MY_COOLREG_VAL_BAR	(1 << 5)
> #define MY_COOLREG_VAL_BAZ	(2 << 5)
>
> to:
>
> #define MY_COOLREG_VAL_FOO	(0 << 5)
> #define MY_COOLREG_VAL_BAR	BIT(5)
> #define MY_COOLREG_VAL_BAZ	(2 << 5)
>
> and this seemed like one way to do it.

Like I stated previously, I prefer something like this for the multi-bit
fields of a register.

>> #define PLX_LASSR_MLOC(x)		(((x) & 0x3) << 1)
>> #define PLX_LASSR_MLOC_ANY32	PLX_LASSR_MLOC(0)
>> #define PLX_LASSR_MLOC_LT1MB	PLX_LASSR_MLOC(1)
>> #define PLX_LASSR_MLOC_ANY64	PLX_LASSR_MLOC(2)
>> #define PLX_LASSR_MLOC_MASK	PLX_LASSR_MLOC(3)
>
> It is handy when matching it up with the data sheet though.  I have a 
> field that occupies bits 2 and 1.  It also doesn't expose a fairly 
> useless PLX_LASRR_MLOC() macro to the user of the header file.

The (BIT(n) * (x)) just looks odd.

The GENMASK() for a multi-bit field also makes it more difficult to
figure out what the maximum value for the field is when there are
more than just a few bits and the lower bit is not 0.

Anyway.. Technically it looks like your series doesn't  break anything
I just don't feel that it adds much clarity.

I'm still looking it over... Maybe I'll change my mind... ;-)

Regards,
Hartley