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

[Guixin Liu <kanie@linux.alibaba.com>]

在 2025/12/11 22:05, Andy Shevchenko 写道:
> 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.
>
> ...
OK, I will be more carefule next time.
>> +#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.
>
> ...
OK, changed in v10.
>
>> +#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).
OK, I will change this to PCI_ROM_IMAGE_SECTOR_SIZE in next version.
>> +#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 */
Added in v10, thanks.

Best Regards,
Guixin Liu
>> +#define PCI_ROM_DATA_STRUCT_SIGNATURE		0x52494350
>> +#define PCI_ROM_DATA_STRUCT_LEN			0x0A