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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 11 Dec 2025, Guixin Liu wrote:

Please capitalize PCI and ROM in the subject.

> This is a preparation patch for the next fix patch.
> Convert some magic numbers to named defines.

This should be reasonable to read even without reading what's in the 
subject first, so it would be better to duplicate the information that 
this relates to PCI ROM header fields ("some" is extremely vague :-)).

> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>  drivers/pci/rom.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/rom.c b/drivers/pci/rom.c
> index e18d3a4383ba..3cb0e94f0e86 100644
> --- a/drivers/pci/rom.c
> +++ b/drivers/pci/rom.c
> @@ -5,6 +5,8 @@
>   * (C) Copyright 2004 Jon Smirl <jonsmirl@yahoo.com>
>   * (C) Copyright 2004 Silicon Graphics, Inc. Jesse Barnes <jbarnes@sgi.com>
>   */
> +
> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/pci.h>
> @@ -12,6 +14,16 @@
>  
>  #include "pci.h"
>  
> +#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_SZ_512		512

I'm sorry I didn't notice immediately, but this is what I meant:

#define PCI_ROM_IMAGE_LEN_UNIT_BYTES		SZ_512

+ you need linux/sizes.h for SZ_512.

> +#define PCI_ROM_IMAGE_SIGNATURE			0xAA55
> +#define PCI_ROM_DATA_STRUCT_SIGNATURE		0x52494350
> +#define PCI_ROM_DATA_STRUCT_LEN			0x0A
> +
>  /**
>   * pci_enable_rom - enable ROM decoding for a PCI device
>   * @pdev: PCI device to enable
> @@ -91,26 +103,27 @@ static size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom,
>  	do {
>  		void __iomem *pds;
>  		/* Standard PCI ROMs start out with these bytes 55 AA */
> -		if (readw(image) != 0xAA55) {
> -			pci_info(pdev, "Invalid PCI ROM header signature: expecting 0xaa55, got %#06x\n",
> -				 readw(image));
> +		if (readw(image) != PCI_ROM_IMAGE_SIGNATURE) {
> +			pci_info(pdev, "Invalid PCI ROM header signature: expecting %#06x, got %#06x\n",
> +				 PCI_ROM_IMAGE_SIGNATURE, readw(image));
>  			break;
>  		}
>  		/* get the PCI data structure and check its "PCIR" signature */
> -		pds = image + readw(image + 24);
> -		if (readl(pds) != 0x52494350) {
> -			pci_info(pdev, "Invalid PCI ROM data signature: expecting 0x52494350, got %#010x\n",
> -				 readl(pds));
> +		pds = image + readw(image + PCI_ROM_POINTER_TO_DATA_STRUCT);
> +		if (readl(pds) != PCI_ROM_DATA_STRUCT_SIGNATURE) {
> +			pci_info(pdev, "Invalid PCI ROM data signature: expecting %#010x, got %#010x\n",
> +				 PCI_ROM_DATA_STRUCT_SIGNATURE, readl(pds));
>  			break;
>  		}
> -		last_image = readb(pds + 21) & 0x80;
> -		length = readw(pds + 16);
> -		image += length * 512;
> +		last_image = readb(pds + PCI_ROM_LAST_IMAGE_INDICATOR) &
> +				   PCI_ROM_LAST_IMAGE_INDICATOR_BIT;
> +		length = readw(pds + PCI_ROM_IMAGE_LEN);
> +		image += length * PCI_ROM_IMAGE_LEN_UNIT_SZ_512;
>  		/* Avoid iterating through memory outside the resource window */
>  		if (image >= rom + size)
>  			break;
>  		if (!last_image) {
> -			if (readw(image) != 0xAA55) {
> +			if (readw(image) != PCI_ROM_IMAGE_SIGNATURE) {
>  				pci_info(pdev, "No more image in the PCI ROM\n");
>  				break;
>  			}
> 

The code change looked fine otherwise.

-- 
 i.