Re: [PATCH v4 2/3] PCI/ACPI: Use CXL _OSC instead of PCIe _OSC

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Tue, 2022-04-05 at 15:47 +0200, Rafael J. Wysocki wrote:
> First off, I would change the subject to something like "Prefer CXL
> _OSC to PCI _OSC for CXL host bridges".  As is, it kind of suggests
> that the CXL _OSC is preferred in all cases.
> 
> On Thu, Mar 31, 2022 at 10:20 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
> > 
> > From: Dan Williams <dan.j.williams@intel.com>
> > 
> > OB In preparation for negotiating OS control of CXL _OSC features, do the
> > minimal enabling to use CXL _OSC to handle the base PCIe feature
> > negotiation. Recall that CXL _OSC is a super-set of PCIe _OSC and the
> > CXL 2.0 specification mandates: "If a CXL Host Bridge device exposes CXL
> > _OSC, CXL aware OSPM shall evaluate CXL _OSC and not evaluate PCIe
> > _OSC."
> > 
> > Rather than pass a boolean flag alongside @root to all the helper
> > functions that need to consider PCIe specifics, add is_pcie() and
> > is_cxl() helper functions to check the flavor of @root. This also
> > allows for dynamic fallback to PCIe _OSC in cases where an attempt to
> > use CXL _OXC fails. This can happen on CXL 1.1 platforms that publish
> > ACPI0016 devices to indicate CXL host bridges, but do not publish the
> > optional CXL _OSC method. CXL _OSC is mandatory for CXL 2.0 hosts.
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Robert Moore <robert.moore@intel.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  include/linux/acpi.h    |  4 +++
> >  include/acpi/acpi_bus.h |  6 ++++
> >  drivers/acpi/pci_root.c | 70 ++++++++++++++++++++++++++++++++---------
> >  3 files changed, 65 insertions(+), 15 deletions(-)
> > 

[..]

> > 
> > @@ -587,8 +619,16 @@ static int acpi_pci_root_add(struct acpi_device *device,
> > 
> >         root->mcfg_addr = acpi_pci_root_get_mcfg_addr(handle);
> > 
> > -       is_pcie = strcmp(acpi_device_hid(device), "PNP0A08") == 0;
> > -       negotiate_os_control(root, &no_aspm, is_pcie);
> > +       acpi_hid = acpi_device_hid(root->device);
> > +       if (strcmp(acpi_hid, "PNP0A08") == 0)
> > +               root->bridge_type = ACPI_BRIDGE_TYPE_PCIE;
> > +       else if (strcmp(acpi_hid, "ACPI0016") == 0)
> > +               root->bridge_type = ACPI_BRIDGE_TYPE_CXL;
> > +       else
> > +               dev_warn(&device->dev, "unknown bridge type with hid: %s\n",
> > +                        acpi_hid);
> 
> Second, because "device" is an ACPI device object and it has a _HID,
> its name should include the ID in this case, so including it into the
> message once more is redundant.
> 
> Also, if this is neither PCIe nor CXL, it should be a "legacy" PCI
> host bridge and that is not an error, so maybe use dev_dbg("Assuming
> non-PCIe host bridge\n")?

Agreed with both points, I'll make the changes for v5.