| Message ID | 20220831081603.3415-8-rrichter@amd.com |
|---|---|
| State | Superseded |
| Delegated to: | Dan Williams |
| Headers | show |
| Series | cxl: Add support for Restricted CXL hosts (RCD mode) | expand |
On Wed, 31 Aug 2022 10:15:55 +0200 Robert Richter <rrichter@amd.com> wrote: > An RCH is a root bridge and has "PNP0A08" or "ACPI0016" ACPI ID set. > Check this. > > Signed-off-by: Robert Richter <rrichter@amd.com> Hi Robert, I'm a little uncomfortable with repurposing acpi_device_id as you have done here. Might be better to do the same as in pci_root.c where the matches are done more directly. > --- > drivers/cxl/acpi.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index a19e3154dd44..ffdf439adb87 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -312,9 +312,16 @@ static int add_root_nvdimm_bridge(struct device *match, void *data) > return 1; > } > > +static const struct acpi_device_id cxl_host_ids[] = { > + { "ACPI0016", 0 }, > + { "PNP0A08", 0 }, > + { }, Trivial but no comma after a null terminator. Always good to make it harder for people to add things where they really shouldn't! pci_root.c avoids using an acpi_device_id table for similar matching. I think the point being to separate probe type use of this table from cases where we aren't using a normal device probe. So to remain consistent with that, I would just grab the hid and match it directly in this code. I don't feel that strongly about this if the ACPI maintainers are fine with reusing this infrastructure as you have it here. > +}; > + > struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host) > { > struct pci_bus *bus = host ? host->bus : NULL; > + struct acpi_device *adev; > > while ((bus = pci_find_next_bus(bus)) != NULL) { > host = bus ? to_pci_host_bridge(bus->bridge) : NULL; > @@ -323,6 +330,19 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host) > > dev_dbg(&host->dev, "PCI bridge found\n"); > > + /* Must be a root bridge */ > + if (host->bus->parent) > + continue; > + > + dev_dbg(&host->dev, "PCI bridge is root bridge\n"); > + > + adev = ACPI_COMPANION(&host->dev); > + if (acpi_match_device_ids(adev, cxl_host_ids)) > + continue; > + > + dev_dbg(&host->dev, "PCI ACPI host found: %s\n", > + acpi_dev_name(adev)); > + > return host; > } >
On 31.08.22 11:20:28, Jonathan Cameron wrote: > Robert Richter <rrichter@amd.com> wrote: > > +static const struct acpi_device_id cxl_host_ids[] = { > > + { "ACPI0016", 0 }, > > + { "PNP0A08", 0 }, > > + { }, > > Trivial but no comma after a null terminator. Always good to make > it harder for people to add things where they really shouldn't! Can do this. > pci_root.c avoids using an acpi_device_id table for similar matching. > I think the point being to separate probe type use of this table > from cases where we aren't using a normal device probe. > So to remain consistent with that, I would just grab the hid > and match it directly in this code. Grabbing the hid only is actually a violation of the acpi spec as a cid could be used interchangeable. It must also work then. It is also not possible to use something like probe or a handler matching the ids because the hosts must be enabled with the already existing drivers and handlers. Suppose there are multiple handlers for the same ids, the first handler wins and all other never get called. To me it looks sane and simple to use acpi_match_device_ids() here. -Robert > > I don't feel that strongly about this if the ACPI maintainers are > fine with reusing this infrastructure as you have it here.
On Thu, 1 Sep 2022 08:16:52 +0200 Robert Richter <rrichter@amd.com> wrote: > On 31.08.22 11:20:28, Jonathan Cameron wrote: > > Robert Richter <rrichter@amd.com> wrote: > > > > +static const struct acpi_device_id cxl_host_ids[] = { > > > + { "ACPI0016", 0 }, > > > + { "PNP0A08", 0 }, > > > + { }, > > > > Trivial but no comma after a null terminator. Always good to make > > it harder for people to add things where they really shouldn't! > > Can do this. > > > pci_root.c avoids using an acpi_device_id table for similar matching. > > I think the point being to separate probe type use of this table > > from cases where we aren't using a normal device probe. > > So to remain consistent with that, I would just grab the hid > > and match it directly in this code. > > Grabbing the hid only is actually a violation of the acpi spec as a > cid could be used interchangeable. It must also work then. > > It is also not possible to use something like probe or a handler > matching the ids because the hosts must be enabled with the already > existing drivers and handlers. Suppose there are multiple handlers for > the same ids, the first handler wins and all other never get called. > > To me it looks sane and simple to use acpi_match_device_ids() here. Ok. One for the ACPI maintainers to comment on if they wish - I'm fine with this Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > -Robert > > > > > I don't feel that strongly about this if the ACPI maintainers are > > fine with reusing this infrastructure as you have it here.
Robert Richter wrote: > An RCH is a root bridge and has "PNP0A08" or "ACPI0016" ACPI ID set. > Check this. > > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/acpi.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index a19e3154dd44..ffdf439adb87 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -312,9 +312,16 @@ static int add_root_nvdimm_bridge(struct device *match, void *data) > return 1; > } > > +static const struct acpi_device_id cxl_host_ids[] = { > + { "ACPI0016", 0 }, > + { "PNP0A08", 0 }, > + { }, > +}; > + > struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host) > { > struct pci_bus *bus = host ? host->bus : NULL; > + struct acpi_device *adev; > > while ((bus = pci_find_next_bus(bus)) != NULL) { > host = bus ? to_pci_host_bridge(bus->bridge) : NULL; > @@ -323,6 +330,19 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host) > > dev_dbg(&host->dev, "PCI bridge found\n"); > > + /* Must be a root bridge */ > + if (host->bus->parent) > + continue; > + > + dev_dbg(&host->dev, "PCI bridge is root bridge\n"); > + > + adev = ACPI_COMPANION(&host->dev); > + if (acpi_match_device_ids(adev, cxl_host_ids)) > + continue; > + > + dev_dbg(&host->dev, "PCI ACPI host found: %s\n", > + acpi_dev_name(adev)); > + Again, finding all the ACPI0016 devices is add_host_bridge_dport() already does.
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index a19e3154dd44..ffdf439adb87 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -312,9 +312,16 @@ static int add_root_nvdimm_bridge(struct device *match, void *data) return 1; } +static const struct acpi_device_id cxl_host_ids[] = { + { "ACPI0016", 0 }, + { "PNP0A08", 0 }, + { }, +}; + struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host) { struct pci_bus *bus = host ? host->bus : NULL; + struct acpi_device *adev; while ((bus = pci_find_next_bus(bus)) != NULL) { host = bus ? to_pci_host_bridge(bus->bridge) : NULL; @@ -323,6 +330,19 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host) dev_dbg(&host->dev, "PCI bridge found\n"); + /* Must be a root bridge */ + if (host->bus->parent) + continue; + + dev_dbg(&host->dev, "PCI bridge is root bridge\n"); + + adev = ACPI_COMPANION(&host->dev); + if (acpi_match_device_ids(adev, cxl_host_ids)) + continue; + + dev_dbg(&host->dev, "PCI ACPI host found: %s\n", + acpi_dev_name(adev)); + return host; }
An RCH is a root bridge and has "PNP0A08" or "ACPI0016" ACPI ID set. Check this. Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/acpi.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)