[PATCH v3 03/11] eeprom: starfive: Enable ID EEPROM configuration

[Bo Gan]

On 4/27/23 7:25 PM, Yanhong Wang wrote:
> +struct starfive_eeprom {
> +	struct eeprom_header header;
> +	struct starfive_eeprom_atom1 atom1;
> +	struct starfive_eeprom_atom4 atom4;
> +};
> +
> +static uchar eeprom_buf[STARFIVE_EEPROM_HATS_SIZE_MAX];
> +
> +struct starfive_eeprom *pbuf = (struct starfive_eeprom *)eeprom_buf;
> +
> +/* Set to 1 if we've read EEPROM into memory */
> +static int has_been_read;
> +

`pbuf` is only used as a const pointer variable, so why not just define pbuf as the buffer?
Something like:

static union {
	struct starfive_eeprom {
		struct eeprom_header header;
		struct starfive_eeprom_atom1 atom1;
		struct starfive_eeprom_atom4 atom4;
	};
	uchar eeprom_buf[STARFIVE_EEPROM_HATS_SIZE_MAX];
} pbuf;

Also all global variables in this file should have __section(".data") attribute. This is because
these uninitialized variables will by default get linked in .bss section. When calling this function
early, before .bss gets initialized, we could get random data, or corrupt data that's not yet relocated
from .bss region. This problem is more obvious when we have CONFIG_OF_SEPARATE=y. When set, device-tree
is expected to get loaded by previous stage, and it's expected to be loaded after _end, which is before
__bss_start. In general, we should not touch anything in .bss (either read/write) before `board_init_f`
completes.

I'm pretty confident on this because I've got a VF2 4GB board, and I hacked opensbi/u-boot to detect this
kind of issue using PMP. I was using https://github.com/starfive-tech/u-boot/tree/JH7110_VisionFive2_devel.
The eeprom implementation there were roughly the same as this patch. I was able to observe PMP access
violation fault when I only allow R/X permission to u-boot image region and R/W to u-boot stack. The
fault was originating from https://github.com/starfive-tech/u-boot/blob/JH7110_VisionFive2_devel/arch/riscv/cpu/jh7110/dram.c#L29
Although this patch is different than the github version, I still think we must avoid the same kind of
problem. Many other board/drivers are already doing __section(".data").