Linux kernel code vs FreeBSD kernel code

Linux driver code contains some serious garbage. I heard this refrain, but I did not realize how bad it was until I looked at it myself. Here is just one example.

Device drivers typically read static memory, typically known as EEPROM or ROM, from the chip to identify version, hard-coded information, device capabilities, etc. These values are used throughout execution of the driver. The reading process is among the first things when the device is attached and powered on.

In the case of FreeBSD, after the kernel reads the ROM, it uses a struct pointer with all the variables pre-populated, and points it at the ROM blob data stored in memory. For example:

struct r88e_rom {
	uint8_t		reserved1[16];
	uint8_t		cck_tx_pwr[R88E_GROUP_2G];
	uint8_t		ht40_tx_pwr[R88E_GROUP_2G - 1];
	uint8_t		tx_pwr_diff;
	uint8_t		reserved2[156];
	uint8_t		channel_plan;
	uint8_t		crystalcap;
#define R88E_ROM_CRYSTALCAP_DEF		0x20

	uint8_t		thermal_meter;
	uint8_t		reserved3[6];
	uint8_t		rf_board_opt;
	uint8_t		rf_feature_opt;
	uint8_t		rf_bt_opt;
	uint8_t		version;
	uint8_t		customer_id;
	uint8_t		reserved4[3];
	uint8_t		rf_ant_opt;
	uint8_t		reserved5[6];
	uint16_t	vid;
	uint16_t	pid;
	uint8_t		usb_opt;
	uint8_t		reserved6[2];
	uint8_t		macaddr[IEEE80211_ADDR_LEN];
	uint8_t		reserved7[2];
	uint8_t		string[33];	/* "realtek 802.11n NIC" */
	uint8_t		reserved8[256];
} __packed;

_Static_assert(sizeof(struct r88e_rom) == R88E_EFUSE_MAP_LEN,
    "R88E_EFUSE_MAP_LEN must be equal to sizeof(struct r88e_rom)!");

Notice the assertion at the bottom, which ensures that the ROM struct’s size equals a pre-defined length. The code will fail to compile if this assertion is not valid. Later, the kernel will instantiate a struct pointer and point it to the ROM, stored in the variable buf, as follows:

struct r88e_rom *rom = (struct r88e_rom *)buf;

Now, rom->channel_plan is set to the correct value. Simple.

Unfortunately, this is not how the same code is written on Linux. As mentioned, the Linux driver also begins by reading the ROM blob and storing it in a value called hwinfo. But rather than creating an equivalent struct pointer, the Linux code uses offset values of the ROM on an as-needed basis. For example, the driver reads the channel_plan as follows:

rtlefuse->eeprom_version = *(u16 *)&hwinfo[params[7]];

In this example, params[7] comes from a list of ROM offsets values set in the previous calling function. (That alone made tracing difficult.) The rtlefuse->eeprom_version is now the same as FreeBSD’s rom->version. This manual process repeats for every variable in the ROM.

While that may be just annoying and require a negligible bit more CPU power, this is not be a problem if it was done all in one place. But instead, the driver reads from the hwinfo blob on a seemingly as-needed during execution. And because these as-needed instances are during normal execution, the driver reads-in the same static value from hwinfo every a simple WiFi function occurs, such as changing the channel.

Okay, but even that might not be too difficult…right? Here’s the real kicker.

Sometimes, the driver works by using incrementing offsets from the ROM blob. For example, consider at read_power_value_fromprom (in drivers/net/wireless/realtek/rtlwifi/hw.c). It initializes eeaddr as a u32 (uint32_t), then assigns it with the offset value EEPROM_TX_PWR_INX. So far so good. But then, rather than using new offsets for every successive value, it increments the eeaddr value in multiple doubly-nested for-loops. Here is a simplified version of the code:

for (rfpath = 0 ; rfpath < MAX_RF_PATH ; rfpath++) {
		/*2.4G default value*/
		for (group = 0 ; group < MAX_CHNL_GROUP_24G; group++) { pwrinfo24g->index_cck_base[rfpath][group] =
			  hwinfo[eeaddr++];
			if (pwrinfo24g->index_cck_base[rfpath][group] == 0xFF)
				pwrinfo24g->index_cck_base[rfpath][group] =
				  0x2D;
		}
}

Notice the line hwinfo[eeaddr++]! Merely reading in that variable changes the offset. Its the Heisenberg Uncertainty Principle equivalent of code. This is a cleaned-up version of the 188-line function. The actual function has 6 nested for-loops, some with if-statements, each incrementing the eeaddr parameter as they go along.

Why would anyone do it this way? You are needlessly using up the CPU, making the code difficult to follow, repeatedly reading in static values and making any minor modifications and re-ordering or re-structuring will essentially break the entire function.

And perhaps the worst offender is when 20 functions deep you are not even working with hwinfo anymore. You are working to a pointer to hwinfo that has been incremented God-knows where, with their own offsets that are near impossible to track down.

In my efforts to port this driver to FreeBSD, I literally resorted to printing out the entire ROM, manually finding the memory, and backing into the equivalent offset. Other bizarre code: I have seen if-conditions that are impossible to reach, misplaced code that should go in the previous function, code that does bits of a tasks, while another function does the entire task – so repeat code, unnecessarily repeated code, etc.

How does this make it into the Linux Kernel?

To be fair, this does not appear to be the fault of Larry Finger, who maintains this driver. This is the fault of Realtek, for vomiting this terrible driver in the first place, providing absolutely zero documentation and refusing to respond to any contact attempts.

I hope my FreeBSD port is cleaner and more performant!