Use of bit fields in AHCI driver is not clearly justified based on the specs

[Jiri Svoboda]

The new AHCI driver (ahci_hw.h) uses C bit fields to describe hardware registers. C bit fields are not portable, as the packing of bit fields into words depends on the processor-specific ABI. For example, for SPARC and x86 this order is reverse (another question, of course, is the byte-endianness of the words we are packing into).

Drivers should work across disparate CPU architectures. I suggest replacing the use of bit fields with bitwise operations, with the help of bitops.h.

[Martin Decky]

I consider the use of bit fields superior to bitwise operations, the code is much more readable (even if macros are used to hide the bitwise operations). However, the bit fields certainly require both little-endian and big-endian declarations which are missing in the current AHCI code.

[Jiri Svoboda]

Okay so first, little-endian and big-endian are terms commonly used with byte order, not bit field packing. Bit field packing order is called high-order to low-order and low-order to high-order by the C1x standard.

Bit fields have the advantage of seemingly prettier-looking code, true, but that doesn't mean it makes it more apparent what the code does, as magic occurs behind the stages. More importantly, bit fields are much less portable.

The C1x standard only defines that bit fields are represented in binary notation and packed into a storage unit one next to another.

• It does not specify the size or alignment of the storage unit is unspecified. Specifically it is not mandated that unsigned char b:1; be packed into an unsigned char, it can be packed into an int instead.
• Even with the assumption that all ABIs ever only differ by packing order, having two versions of the definitions with for the two packing orders increases chance of bug/inconsistency, largely negating the benefit.

Also:

• bit fields cannot be used with definitions of I/O registers because they cannot / must not be used with pio functions - even if this compiled (depending on implementation of PIO functions), it would be semantically wrong because the compiler generates read-modify-write operation based on assumption that the bit field is stored in memory
• using bit fields natively limits you to just the host byte order. If you have a memory structure or I/O registers that use a fixed byte order (i.e. potentially different from host byte order), then you cannot use the bit fields anyway (without swapping the byte order before and after every access)

Considering the above I believe bit fields are best avoided, especially in driver code.

[Martin Decky]

little-endian and big-endian are terms commonly used with byte order

OK, I'll rename the ticket to reflect better what the real trouble is.

But do you know of any platform where the byte order and bit order are not aligned? In our case all little-endian platforms are high-to-low and all big-endiand platforms are low-to-high. Or am I mistaken?

It does not specify the size or alignment of the storage unit is unspecified. Specifically it is not mandated that unsigned char b:1; be packed into an unsigned char, it can be packed into an int instead.

Of course. Do you suspect the AHCI code to silently expects something like that?

having two versions of the definitions with for the two packing orders increases chance of bug/inconsistency, largely negating the benefit.

In my opinion, the chance of a bug/inconsistency is almost the same to hand-coded bitwise operations.

it would be semantically wrong because the compiler generates read-modify-write operation based on assumption that the bit field is stored in memory

Read-modify-write is not necessarily semantically wrong. It might be wrong for some I/O operations, but it might be fine for others, depending on how the I/O registers are defined.

Of course, you might be right for the AHCI case, but you also might be wrong. The AHCI I/O register sets should behave more like buffers than traditional I/O ports (such as in the ATA case).

However, I do suspect that some of the issues the original author reported on real hardware might be caused by this very issue. But I didn't have the time to investigate yet.

Considering the above I believe bit fields are best avoided, especially in driver code.

Well, the use of bit fields in driver code definitively needs to be justified. But this is true for for all instances where the C ABI and the "native" ABI might clash, not only for bit fields.

Otherwise, I believe you are just demonizing the bit fields. In cases where the ABI doesn't matter, it is perfectly fine to use them.

[Jiri Svoboda]

What I meant is using bit fields in structures with the intent of mapping to a specific external representation (which is the worse incarnation of using structures not containing bit fields for the same purpose). Using bit fields as regular data type (without assuming a specific implementation) is perfectly okay.