Hello Thomas,
On Wed, 6 Dec 2023 at 13:54, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> The type definition of struct pci_setup_rom in <asm/pci.h> requires
> struct setup_data from <asm/bootparam.h>. Many drivers include
> <linux/pci.h>, but do not use boot parameters. Changes to bootparam.h
> or its included header files could easily trigger a large, unnecessary
> rebuild of the kernel.
>
> Moving struct pci_setup_rom into its own header file avoid including
> <asm/bootparam.h> in <asm/pci.h>. Update the only two users of the
> struct in the x86 PCI code and in the EFI code. Also remove the include
> statement for x86_init.h, which is unnecessary but pulls in bootparams.h.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> arch/x86/include/asm/pci.h | 13 -------------
> arch/x86/include/asm/pci_setup.h | 19 +++++++++++++++++++
> arch/x86/pci/common.c | 1 +
> drivers/firmware/efi/libstub/x86-stub.c | 1 +
> 4 files changed, 21 insertions(+), 13 deletions(-)
> create mode 100644 arch/x86/include/asm/pci_setup.h
>
Thanks for cleaning this up.
Would it be more appropriate to move all setup_data related
definitions into a separate header entirely?
- the SETUP_ defines
- struct setup_data
- struct pci_setup_rom
- struct jailhouse_setup_data
etc etc
struct setup_header has a setup_data field which is the root of the
setup_data linked list, but it is typed as __u64 so it doesn't
actually need to know the real type of the associated structs.
That way, you can avoid creating a special asm/pci_setup.h that only
covers this one particular definition.
> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index b40c462b4af3..b3ab80a03365 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -10,7 +10,6 @@
> #include <linux/numa.h>
> #include <asm/io.h>
> #include <asm/memtype.h>
> -#include <asm/x86_init.h>
>
> struct pci_sysdata {
> int domain; /* PCI domain */
> @@ -124,16 +123,4 @@ cpumask_of_pcibus(const struct pci_bus *bus)
> }
> #endif
>
> -struct pci_setup_rom {
> - struct setup_data data;
> - uint16_t vendor;
> - uint16_t devid;
> - uint64_t pcilen;
> - unsigned long segment;
> - unsigned long bus;
> - unsigned long device;
> - unsigned long function;
> - uint8_t romdata[];
> -};
> -
> #endif /* _ASM_X86_PCI_H */
> diff --git a/arch/x86/include/asm/pci_setup.h b/arch/x86/include/asm/pci_setup.h
> new file mode 100644
> index 000000000000..b4b246ef6f2b
> --- /dev/null
> +++ b/arch/x86/include/asm/pci_setup.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_PCI_SETUP_H
> +#define _ASM_X86_PCI_SETUP_H
> +
> +#include <asm/bootparam.h>
> +
> +struct pci_setup_rom {
> + struct setup_data data;
> + uint16_t vendor;
> + uint16_t devid;
> + uint64_t pcilen;
> + unsigned long segment;
> + unsigned long bus;
> + unsigned long device;
> + unsigned long function;
> + uint8_t romdata[];
> +};
> +
> +#endif /* _ASM_X86_PCI_SETUP_H */
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index ddb798603201..c6cbb9182160 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -17,6 +17,7 @@
> #include <asm/segment.h>
> #include <asm/io.h>
> #include <asm/smp.h>
> +#include <asm/pci_setup.h>
> #include <asm/pci_x86.h>
> #include <asm/setup.h>
> #include <asm/irqdomain.h>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 1bfdae34df39..0c878ebe5257 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -17,6 +17,7 @@
> #include <asm/boot.h>
> #include <asm/kaslr.h>
> #include <asm/sev.h>
> +#include <asm/pci_setup.h>
>
> #include "efistub.h"
> #include "x86-stub.h"
> --
> 2.43.0
>
