Re: [PATCH v2 03/15] pci: Use pci_sbdf_t in pci_device_detect()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/06/2026 3:50 pm, Teddy Astie wrote:
> diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
> index a5c79f56fc..27bcff0a8f 100644
> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -1510,25 +1508,24 @@ void __init ehci_dbgp_init(void)
>      }
>      else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
>      {
> -        unsigned int bus, slot, func;
> +        pci_sbdf_t sbdf = PCI_SBDF(0, 0, 0, 0);

Elsewhere you don't initialise sbdf, which should be fine here too.

If you do need to initialise it to all zeroes, use = {}; instead, which
is shorter.

>  
> -        e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
> +        e = parse_pci_sbdf(opt_dbgp + 8, &sbdf);
>          if ( !e || *e )
>              return;
>  
> -        dbgp->bus = bus;
> -        dbgp->slot = slot;
> -        dbgp->func = func;
> +        dbgp->bus = sbdf.bus;
> +        dbgp->slot = sbdf.dev;
> +        dbgp->func = sbdf.fn;
>  
> -        if ( !pci_device_detect(0, bus, slot, func) )
> +        if ( !pci_device_detect(sbdf) )
>              return;

Looking at both this patch and the next one, and despite what I said in
patch 2, I'm going to recommend separating the pci_device_detect() parts.

This patch has two separate changes going on; converting
pci_device_detect(), and using parse_pci_sbdf() in some places, while
patch 4 has some simple conversions to parse_pci_sbdf().

Break pci_device_detect() out on it's own.  Trivial caller adjustments
(e.g. acpi_parse_one_drhd(), register_one_satc(), even
_scan_pci_devices()) are fine to stay with that, but for non-trivial
ones (e.g. acpi_parse_dev_scope(), ehci_dbgp_init()), just wrap the
caller with PCI_SBDF(...) in the interim.

Then, you want a patch for acpi_parse_dev_scope(), and a separate patch
for the work in ehci-dbgp.c.

It makes the series longer but the result is easier to review because
the patches are doing fewer things at once.

> diff --git a/xen/drivers/passthrough/vtd/dmar.c 
> b/xen/drivers/passthrough/vtd/dmar.c
> index 71e5a51a58..61269c7e0d 100644
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -1185,8 +1177,9 @@ int cf_check intel_iommu_get_reserved_device_memory(
>  static int __init cf_check parse_rmrr_param(const char *str)
>  {
>      const char *s = str, *cur, *stmp;
> -    unsigned int seg, bus, dev, func, dev_count;
> +    unsigned int dev_count;
>      unsigned long start, end;
> +    pci_sbdf_t sbdf;
>  
>      do {
>          if ( nr_rmrr >= MAX_USER_RMRR )

You're missing a hunk here, and it doesn't compile.  You've changed the
variables, but not changed the parse_pci_seg() call.

They look like they're in patch 4, which I presume is just a rebasing error?

~Andrew