[PATCH] PCI/ACPI: Don't reset a fwnode set by OF

[PATCH] PCI/ACPI: Don't reset a fwnode set by OF

[Jean-Philippe Brucker]

Commit 375553a93201 ("PCI: Setup ACPI fwnode early and at the same time
with OF") added a call to pci_set_acpi_fwnode() in pci_setup_device(),
which unconditionally clears any fwnode previously set by
pci_set_of_node().

pci_set_acpi_fwnode() looks for ACPI_COMPANION(), which only returns the
existing fwnode if it was set by ACPI_COMPANION_SET(). If it was set by
OF instead, ACPI_COMPANION() returns NULL and pci_set_acpi_fwnode()
accidentally clears the fwnode. To fix this, look for any fwnode instead
of just ACPI companions.

Fixes: 375553a93201 ("PCI: Setup ACPI fwnode early and at the same time with OF")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
This fixes boot of virtio-iommu under OF on v5.15-rc1
---
 drivers/pci/pci-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index a1b1e2a01632..483a9e50f6ca 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -937,7 +937,7 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev);
 
 void pci_set_acpi_fwnode(struct pci_dev *dev)
 {
-	if (!ACPI_COMPANION(&dev->dev) && !pci_dev_is_added(dev))
+	if (!dev->dev.fwnode && !pci_dev_is_added(dev))
 		ACPI_COMPANION_SET(&dev->dev,
 				   acpi_pci_find_companion(&dev->dev));
 }
-- 
2.33.0

Re: [PATCH] PCI/ACPI: Don't reset a fwnode set by OF

[Bjorn Helgaas]

[+cc Rob]

On Mon, Sep 13, 2021 at 06:23:59PM +0100, Jean-Philippe Brucker wrote:
> Commit 375553a93201 ("PCI: Setup ACPI fwnode early and at the same time
> with OF") added a call to pci_set_acpi_fwnode() in pci_setup_device(),
> which unconditionally clears any fwnode previously set by
> pci_set_of_node().
> 
> pci_set_acpi_fwnode() looks for ACPI_COMPANION(), which only returns the
> existing fwnode if it was set by ACPI_COMPANION_SET(). If it was set by
> OF instead, ACPI_COMPANION() returns NULL and pci_set_acpi_fwnode()
> accidentally clears the fwnode. To fix this, look for any fwnode instead
> of just ACPI companions.
> 
> Fixes: 375553a93201 ("PCI: Setup ACPI fwnode early and at the same time with OF")
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> This fixes boot of virtio-iommu under OF on v5.15-rc1
> ---
>  drivers/pci/pci-acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index a1b1e2a01632..483a9e50f6ca 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -937,7 +937,7 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev);
>  
>  void pci_set_acpi_fwnode(struct pci_dev *dev)
>  {
> -	if (!ACPI_COMPANION(&dev->dev) && !pci_dev_is_added(dev))
> +	if (!dev->dev.fwnode && !pci_dev_is_added(dev))

I don't doubt that this is correct, but it seems excessively subtle,
like we're violating some layering or something.

Rafael, Rob, is there anything better we can do here?

>  		ACPI_COMPANION_SET(&dev->dev,
>  				   acpi_pci_find_companion(&dev->dev));
>  }
> -- 
> 2.33.0
> 

Re: [PATCH] PCI/ACPI: Don't reset a fwnode set by OF

[Rob Herring]

On Mon, Sep 13, 2021 at 2:28 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rob]
>
> On Mon, Sep 13, 2021 at 06:23:59PM +0100, Jean-Philippe Brucker wrote:
> > Commit 375553a93201 ("PCI: Setup ACPI fwnode early and at the same time
> > with OF") added a call to pci_set_acpi_fwnode() in pci_setup_device(),
> > which unconditionally clears any fwnode previously set by
> > pci_set_of_node().
> >
> > pci_set_acpi_fwnode() looks for ACPI_COMPANION(), which only returns the
> > existing fwnode if it was set by ACPI_COMPANION_SET(). If it was set by
> > OF instead, ACPI_COMPANION() returns NULL and pci_set_acpi_fwnode()
> > accidentally clears the fwnode. To fix this, look for any fwnode instead
> > of just ACPI companions.
> >
> > Fixes: 375553a93201 ("PCI: Setup ACPI fwnode early and at the same time with OF")
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > This fixes boot of virtio-iommu under OF on v5.15-rc1
> > ---
> >  drivers/pci/pci-acpi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > index a1b1e2a01632..483a9e50f6ca 100644
> > --- a/drivers/pci/pci-acpi.c
> > +++ b/drivers/pci/pci-acpi.c
> > @@ -937,7 +937,7 @@ static struct acpi_device *acpi_pci_find_companion(struct device *dev);
> >
> >  void pci_set_acpi_fwnode(struct pci_dev *dev)
> >  {
> > -     if (!ACPI_COMPANION(&dev->dev) && !pci_dev_is_added(dev))
> > +     if (!dev->dev.fwnode && !pci_dev_is_added(dev))
>
> I don't doubt that this is correct, but it seems excessively subtle,
> like we're violating some layering or something.

That stems from DT and ACPI node handle handling being asymmetric in
struct device. DT has its own node pointer plus the fwnode handle
while ACPI only uses fwnode handle. It's that way because who is going
to replace all the dev->of_node occurrences? Only ~7500 of them based
on a quick grep.

> Rafael, Rob, is there anything better we can do here?

I don't think so. Using dev_fwnode() would be slightly better than
direct access.

Rob