[v8,08/17] ACPI: pci_link: Clear the dependencies after probe

Commit Message

Sunil V L Aug. 12, 2024, 12:59 a.m. UTC

RISC-V platforms need to use dependencies between PCI host bridge, Link
devices and the interrupt controllers to ensure probe order. The
dependency is like below.

Interrupt controller <-- Link Device <-- PCI Host bridge.

If there is no dependency between Link device and PCI Host Bridge,
then PCI devices may be probed prior to Link devices.  If a PCI
device is probed before its Link device, we won't be able to find
its INTx mapping.

So, add the link device's HID to dependency honor list and clear the
dependency after probe is done so that the dependent devices are
unblocked to probe.

Signed-off-by: Sunil V L <[e-mail]>
Tested-by: Björn Töpel <[e-mail]>
---
 drivers/acpi/pci_link.c | 2 ++
 drivers/acpi/scan.c     | 1 +
 2 files changed, 3 insertions(+)

Comments

Bjorn Helgaas Aug. 22, 2024, 9:44 p.m. UTC | #1

On Mon, Aug 12, 2024 at 06:29:20AM +0530, Sunil V L wrote:
> RISC-V platforms need to use dependencies between PCI host bridge, Link
> devices and the interrupt controllers to ensure probe order. The
> dependency is like below.
> 
> Interrupt controller <-- Link Device <-- PCI Host bridge.
> 
> If there is no dependency between Link device and PCI Host Bridge,
> then PCI devices may be probed prior to Link devices.  If a PCI
> device is probed before its Link device, we won't be able to find
> its INTx mapping.

This seems to explain why we want these dependencies, which is useful,
but *this* patch only removes the dependencies.

Maybe this description should be in the patch that *adds* the
dependencies, e.g., "ACPI: RISC-V: Implement function to add implicit
dependencies"?

> So, add the link device's HID to dependency honor list and clear the
> dependency after probe is done so that the dependent devices are
> unblocked to probe.

This still claims this patch adds HID, which I don't think it does.

> Signed-off-by: Sunil V L <[e-mail]>
> Tested-by: Björn Töpel <[e-mail]>
> ---
>  drivers/acpi/pci_link.c | 2 ++
>  drivers/acpi/scan.c     | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index aa1038b8aec4..b727db968f33 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device,
>  	if (result)
>  		kfree(link);
>  
> +	acpi_dev_clear_dependencies(device);
> +
>  	return result < 0 ? result : 1;
>  }
>  
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 28a221f956d7..753539a1f26b 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = {
>  	"INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */
>  	"RSCV0001", /* RISC-V PLIC */
>  	"RSCV0002", /* RISC-V APLIC */
> +	"PNP0C0F",  /* PCI Link Device */
>  	NULL
>  };
>  
> -- 
> 2.43.0
>

Sunil V L Aug. 23, 2024, 6:33 a.m. UTC | #2

Hi Bjorn,

On Thu, Aug 22, 2024 at 04:44:15PM -0500, Bjorn Helgaas wrote:
> On Mon, Aug 12, 2024 at 06:29:20AM +0530, Sunil V L wrote:
> > RISC-V platforms need to use dependencies between PCI host bridge, Link
> > devices and the interrupt controllers to ensure probe order. The
> > dependency is like below.
> > 
> > Interrupt controller <-- Link Device <-- PCI Host bridge.
> > 
> > If there is no dependency between Link device and PCI Host Bridge,
> > then PCI devices may be probed prior to Link devices.  If a PCI
> > device is probed before its Link device, we won't be able to find
> > its INTx mapping.
> 
> This seems to explain why we want these dependencies, which is useful,
> but *this* patch only removes the dependencies.
> 
> Maybe this description should be in the patch that *adds* the
> dependencies, e.g., "ACPI: RISC-V: Implement function to add implicit
> dependencies"?
>
Okay. Let me move this to the patch you suggested.
 
> > So, add the link device's HID to dependency honor list and clear the
> > dependency after probe is done so that the dependent devices are
> > unblocked to probe.
> 
> This still claims this patch adds HID, which I don't think it does.
>
Please see below.
 
> > Signed-off-by: Sunil V L <[e-mail]>
> > Tested-by: Björn Töpel <[e-mail]>
> > ---
> >  drivers/acpi/pci_link.c | 2 ++
> >  drivers/acpi/scan.c     | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > index aa1038b8aec4..b727db968f33 100644
> > --- a/drivers/acpi/pci_link.c
> > +++ b/drivers/acpi/pci_link.c
> > @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device,
> >  	if (result)
> >  		kfree(link);
> >  
> > +	acpi_dev_clear_dependencies(device);
> > +
> >  	return result < 0 ? result : 1;
> >  }
> >  
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 28a221f956d7..753539a1f26b 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = {
> >  	"INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */
> >  	"RSCV0001", /* RISC-V PLIC */
> >  	"RSCV0002", /* RISC-V APLIC */
> > +	"PNP0C0F",  /* PCI Link Device */
This is the change which I meant adding HID to the honor list. Do you
recommend to make this change separate patch so that it doesn't confuse
with adding a new HID to the probe match table?

Thanks!
Sunil

Bjorn Helgaas Aug. 23, 2024, 5:45 p.m. UTC | #3

On Fri, Aug 23, 2024 at 12:03:25PM +0530, Sunil V L wrote:
> On Thu, Aug 22, 2024 at 04:44:15PM -0500, Bjorn Helgaas wrote:
> > On Mon, Aug 12, 2024 at 06:29:20AM +0530, Sunil V L wrote:
> > > RISC-V platforms need to use dependencies between PCI host bridge, Link
> > > devices and the interrupt controllers to ensure probe order. The
> > > dependency is like below.
> > > 
> > > Interrupt controller <-- Link Device <-- PCI Host bridge.
> > > 
> > > If there is no dependency between Link device and PCI Host Bridge,
> > > then PCI devices may be probed prior to Link devices.  If a PCI
> > > device is probed before its Link device, we won't be able to find
> > > its INTx mapping.
> > 
> > This seems to explain why we want these dependencies, which is useful,
> > but *this* patch only removes the dependencies.
> > 
> > Maybe this description should be in the patch that *adds* the
> > dependencies, e.g., "ACPI: RISC-V: Implement function to add implicit
> > dependencies"?
> >
> Okay. Let me move this to the patch you suggested.

Given my forgetfulness and your pointing out that this *does* add the
dependencies (by virtue of adding PNP0C0F to the acpi_honor_dep_ids[]
list), it does make sense here.

> > > So, add the link device's HID to dependency honor list and clear the
> > > dependency after probe is done so that the dependent devices are
> > > unblocked to probe.

Maybe expanding this to "Add the link devices HID (PNP0C0F) to the
acpi_honor_dep_ids[] dependency list" would help connect this all
together?

> > This still claims this patch adds HID, which I don't think it does.
> >
> Please see below.
>  
> > > Signed-off-by: Sunil V L <[e-mail]>
> > > Tested-by: Björn Töpel <[e-mail]>
> > > ---
> > >  drivers/acpi/pci_link.c | 2 ++
> > >  drivers/acpi/scan.c     | 1 +
> > >  2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > > index aa1038b8aec4..b727db968f33 100644
> > > --- a/drivers/acpi/pci_link.c
> > > +++ b/drivers/acpi/pci_link.c
> > > @@ -748,6 +748,8 @@ static int acpi_pci_link_add(struct acpi_device *device,
> > >  	if (result)
> > >  		kfree(link);
> > >  
> > > +	acpi_dev_clear_dependencies(device);
> > > +
> > >  	return result < 0 ? result : 1;
> > >  }
> > >  
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 28a221f956d7..753539a1f26b 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -863,6 +863,7 @@ static const char * const acpi_honor_dep_ids[] = {
> > >  	"INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */
> > >  	"RSCV0001", /* RISC-V PLIC */
> > >  	"RSCV0002", /* RISC-V APLIC */
> > > +	"PNP0C0F",  /* PCI Link Device */
>
> This is the change which I meant adding HID to the honor list. Do you
> recommend to make this change separate patch so that it doesn't confuse
> with adding a new HID to the probe match table?

Oh, right, sorry.  I remember working this out in the past, but I had
forgotten.

I think it makes sense in this patch because the add and removal are
matched when they're in the same patch.

Sorry for the noise!

Bjorn

Patch

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index aa1038b8aec4..b727db968f33 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -748,6 +748,8 @@  static int acpi_pci_link_add(struct acpi_device *device,
 	if (result)
 		kfree(link);
 
+	acpi_dev_clear_dependencies(device);
+
 	return result < 0 ? result : 1;
 }
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 28a221f956d7..753539a1f26b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -863,6 +863,7 @@  static const char * const acpi_honor_dep_ids[] = {
 	"INTC10CF", /* IVSC (MTL) driver must be loaded to allow i2c access to camera sensors */
 	"RSCV0001", /* RISC-V PLIC */
 	"RSCV0002", /* RISC-V APLIC */
+	"PNP0C0F",  /* PCI Link Device */
 	NULL
 };