Re: [PATCH 06/36] Add Cavium OCTEON processor CSR definitions

[David Daney <ddaney@caviumnetworks.com>]

Ralf Baechle wrote:

> struct foo {
> 	unsigned int	x:1;
> 	unsigned int	y:4;
> };
> 
> void bar(volatile struct foo *p)
> {
> 	p->x = 1;
> 	p->y++;
> }
> 
> which gcc 4.3 for a MIPS32R2 target will compile into:
> 
> 	lw	$3, 0($4)
> 	li	$2, 1
> 	ins	$3, $2, 31, 1
> 	sw	$3, 0($4)
> 	lw	$2, 0($4)
> 	lw	$3, 0($4)
> 	ext	$2, $2, 27, 4
> 	addiu	$2, $2, 1
> 	ins	$3, $2, 27, 4
> 	sw	$3, 0($4)
> 	j	$31
> 	nop
> 
> Imagine struct foo was describing a hardware register so the pointer to it
> was marked volatile.  A human coder wouldn't have done multiple loads /
> stores.  Worse, if you actually want to change multiple fields in a
> register atomically then with bitfields you have _no_ possibility to enforce
> that.
> 

This is bogus as you use a false premise.  For your test case the 
compiler is generating exactly the accesses you are requesting by 
declaring the thing as volatile.

> The Linux programming programming model relies on accessor functions like
> readl, ioread32 etc.  Those take addresses as arguments - but bitfields
> don't have addresses in C ...
> 

We too use accessor functions, as they are required on certain classes 
of registers to obtain correct semantics.

The real question is if manipulating the values after they are obtained 
with the accessor is done correctly.  My assertion is that although it 
may not be the One True Kernel Way, our structure bitfield definitions 
result in correct semantics and have better compile time error checking 
than can be obtained when coding a bunch of explicit masks and shifts.

That is not to say that I am adverse to eliminating the structure 
bitfield definitions, but we should not use incorrect register access 
semantics as the reason, as it is not a valid argument.

David Daney