Re: [RFC XEN PATCH v6 4/5] libxl: Use gsi instead of irq for mapping pirq

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Thu, Mar 28, 2024 at 02:34:01PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 96cb4da0794e..2cec83e0b734 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1478,8 +1478,14 @@ static void pci_add_dm_done(libxl__egc *egc,
>      fclose(f);
>      if (!pci_supp_legacy_irq())
>          goto out_no_irq;
> -    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
>                                  pci->bus, pci->dev, pci->func);
> +    r = access(sysfs_path, F_OK);
> +    if (r && errno == ENOENT) {
> +        /* To compitable with old version of kernel, still need to use irq */

There's a typo, this would be "To be compatible ...". Also maybe
something like "Fallback to "/irq" for compatibility with old version of
the kernel." might sound better.

> +        sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +                               pci->bus, pci->dev, pci->func);
> +    }
>      f = fopen(sysfs_path, "r");
>      if (f == NULL) {
>          LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
> @@ -2229,9 +2235,15 @@ skip_bar:
>      if (!pci_supp_legacy_irq())
>          goto skip_legacy_irq;
>  
> -    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +    sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain,
>                             pci->bus, pci->dev, pci->func);
>  
> +    rc = access(sysfs_path, F_OK);

Please, don't use the variable `rc` here, this one is reserved for libxl
error/return code in libxl. Introduce `int r` instead.

> +    if (rc && errno == ENOENT) {
> +        /* To compitable with old version of kernel, still need to use irq */
> +        sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
> +                               pci->bus, pci->dev, pci->func);
> +    }
>      f = fopen(sysfs_path, "r");
>      if (f == NULL) {
>          LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);

With those two things fixed: Reviewed-by: Anthony PERARD 
<anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD