Re: [PATCH v9 1/2] PCI: Introduce named defines for pci rom

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Dec 11, 2025 at 08:59:05PM +0800, Guixin Liu wrote:
> Convert the magic numbers associated with PCI ROM into named
> definitions. Some of these definitions will be used in the second
> fix patch.

Please, give at least 24h between the versions in order other being able (quick
enough) to review the changes.

...

> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>

> +#include <linux/sizes.h>

'i' goes before 'l', yeah it's hard to see in small monospace fonts.

...


> +#define PCI_ROM_HEADER_SIZE			0x1A
> +#define PCI_ROM_POINTER_TO_DATA_STRUCT		0x18
> +#define PCI_ROM_LAST_IMAGE_INDICATOR		0x15
> +#define PCI_ROM_LAST_IMAGE_INDICATOR_BIT	BIT(7)

> +#define PCI_ROM_IMAGE_LEN			0x10
> +#define PCI_ROM_IMAGE_LEN_UNIT_BYTES		SZ_512

I'm a bit confused by the naming here.

There are two definitions that end with _LEN, but the meaning of them is
the offset where the respective "len" can be read from.

Now, there is a _LEN_UNIT_BYTES, which seems related to _LEN, but in unclear
way.

With all the AA55 signature it pretty much sounds to me like a sector division
(legacy from the era of floppies).

That said, I would name the size of the "unit" as

#define PCI_ROM_IMAGE_SECTOR_SIZE		SZ_512

(without or with _BYTES on your choice, usually we don't use unit for bytes
 when there is no room for misinterpretation).

> +#define PCI_ROM_IMAGE_SIGNATURE			0xAA55

Despite the below comment explains this, I would explicitly state it here

/* Data structure signature is "PCIR" in ASCII representation */

> +#define PCI_ROM_DATA_STRUCT_SIGNATURE		0x52494350
> +#define PCI_ROM_DATA_STRUCT_LEN			0x0A

-- 
With Best Regards,
Andy Shevchenko