Re: [PATCH v3 17/18] pci: Drop parse_pci_split{_seg}()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.06.2026 19:21, Teddy Astie wrote:
> No user of these functions remain, collapse all the logic into the
> recently introduced pci_sbdf_t variants.
> 
> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

In order to see whether the style issues introduced in patch 08 actually help
here keeping the diff small, I looked at this patch first. Hence a question
and a few nits here right away:

> --- a/xen/drivers/pci/pci.c
> +++ b/xen/drivers/pci/pci.c
> @@ -151,18 +151,7 @@ void pci_intx(const struct pci_dev *pdev, bool enable)
>      pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
>  }
>  
> -const char *__init parse_pci_split(const char *s, unsigned int *seg_p,
> -                             unsigned int *bus_p, unsigned int *dev_p,
> -                             unsigned int *func_p)
> -{
> -    bool def_seg;
> -
> -    return parse_pci_seg(s, seg_p, bus_p, dev_p, func_p, &def_seg);
> -}
> -
> -const char *__init parse_pci_split_seg(const char *s, unsigned int *seg_p,
> -                                 unsigned int *bus_p, unsigned int *dev_p,
> -                                 unsigned int *func_p, bool *def_seg)
> +const char *__init parse_pci_seg(const char *s, pci_sbdf_t *sbdf, bool 
> *def_seg)
>  {
>      unsigned long seg = simple_strtoul(s, &s, 16), bus, dev, func = 0;
>  
> @@ -180,39 +169,21 @@ const char *__init parse_pci_split_seg(const char *s, 
> unsigned int *seg_p,
>          *def_seg = true;
>      }
>  
> -    if ( func_p && *s == '.' )
> +    if ( *s == '.' )
>          func = simple_strtoul(s + 1, &s, 0);

And the function part being optional goes away entirely silently?

> -    if ( seg != (seg_p ? (u16)seg : 0) ||
> +    if ( seg != (u16)seg ||

Nit: uint16_t please when you touch this line anyway.

>           bus != PCI_BUS(PCI_BDF(bus, 0)) ||
>           dev != PCI_SLOT(PCI_DEVFN(dev, 0)) ||
>           func != PCI_FUNC(PCI_DEVFN(0, func)) )
>          return NULL;
>  
> -    if ( seg_p )
> -        *seg_p = seg;
> -    *bus_p = bus;
> -    *dev_p = dev;
> -    if ( func_p )
> -        *func_p = func;
> -
> -    return s;
> -}
> -
> -const char *__init parse_pci_sbdf(const char *s, pci_sbdf_t *sbdf)
> -{
> -    unsigned int seg, bus, dev, func;
> -    const char *out = parse_pci(s, &seg, &bus, &dev, &func);
> -
>      *sbdf = PCI_SBDF(seg, bus, dev, func);
> -    return out;
> +    return s;
>  }

Nit: Blank line please ahead of a function's main "return", and ...

> -const char *__init parse_pci_sbdf_seg(const char *s, pci_sbdf_t *sbdf, bool 
> *def_seg)
> +const char *__init parse_pci(const char *s, pci_sbdf_t *sbdf)
>  {
> -    unsigned int seg, bus, dev, func;
> -    const char *out = parse_pci_seg(s, &seg, &bus, &dev, &func, def_seg);
> -
> -    *sbdf = PCI_SBDF(seg, bus, dev, func);
> -    return out;
> +    bool def_seg;
> +    return parse_pci_seg(s, sbdf, &def_seg);
>  }

... more generally between declartion(s) and statement(s).

Jan