Michael Kelley wrote:
[..]
> > Here is the replacement fixup that I will fold in if it looks good to
> > you:
> >
> > -- 8< --
> > diff --git a/drivers/pci/controller/pci-hyperv.c
> > b/drivers/pci/controller/pci-hyperv.c
> > index cfe9806bdbe4..f1079a438bff 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> > {
> > struct pci_host_bridge *bridge;
> > struct hv_pcibus_device *hbus;
> > - u16 dom_req, dom;
> > + int ret, dom;
> > + u16 dom_req;
> > char *name;
> > - int ret;
> >
> > bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> > if (!bridge)
> > @@ -3673,8 +3673,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > * collisions) in the same VM.
> > */
> > dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> > - dom = pci_bus_find_emul_domain_nr(dom_req);
> > -
>
> As an additional paragraph the larger comment block above, let's include a
> massaged version of the comment associated with HVPCI_DOM_INVALID.
> Perhaps:
>
> *
> * Because Gen1 VMs use domain 0, don't allow picking domain 0 here, even
> * if bytes 4 and 5 of the instance GUID are both zero.
> */
That looks good to me.
>
> > + dom = pci_bus_find_emul_domain_nr(dom_req, 1, U16_MAX);
> > if (dom < 0) {
> > dev_err(&hdev->device,
> > "Unable to use dom# 0x%x or other numbers", dom_req);
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index f60244ff9ef8..30935fe85af9 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -881,7 +881,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
> > unsigned long features)
> > pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
> >
> > sd->vmd_dev = vmd->dev;
> > - sd->domain = pci_bus_find_emul_domain_nr(PCI_DOMAIN_NR_NOT_SET);
> > +
> > + /*
> > + * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> > + * domains. Per ACPI r6.0, sec 6.5.6, _SEG returns an integer, of
> > + * which the lower 16 bits are the PCI Segment Group (domain) number.
> > + * Other bits are currently reserved.
> > + */
> > + sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
> > if (sd->domain < 0)
> > return sd->domain;
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 833ebf2d5213..de42e53f07d0 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6695,34 +6695,15 @@ static void pci_no_domains(void)
> > #ifdef CONFIG_PCI_DOMAINS
> > static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> >
> > -/*
> > - * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > - * fallback to the first free domain number above the last ACPI segment
> > number.
> > - * Caller may have a specific domain number in mind, in which case try to
> > - * reserve it.
> > - *
> > - * Note that this allocation is freed by pci_release_host_bridge_dev().
> > +/**
> > + * pci_bus_find_emul_domain_nr() - allocate a PCI domain number per
> > constraints
> > + * @hint: desired domain, 0 if any id in the range of @min to @max is
> > acceptable
> > + * @min: minimum allowable domain
> > + * @max: maximum allowable domain, no ids higher than INT_MAX will be
> > returned
> > */
> > -int pci_bus_find_emul_domain_nr(int hint)
> > +u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
>
> Shouldn't the return type here still be "int"? ida_alloc_range() can return
> a negative
> errno if it fails. And the call sites in hv_pci_probe() and
> vmd_enable_domain()
> store the return value into an "int".
Yup, good catch.
> Other than that, and my suggested added comment, this looks good.
Thanks!
