Re: [RFC PATCH v3 02/16] cxl/acpi: Add an acpi_cxl module for the CXL interconnect

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

On Wed, 2021-01-13 at 13:40 +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 12, 2021 at 1:29 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > 
> > From: Vishal Verma <vishal.l.verma@intel.com>
> > 
> > Add an acpi_cxl module to coordinate the ACPI portions of the CXL
> > (Compute eXpress Link) interconnect. This driver binds to ACPI0017
> > objects in the ACPI tree, and coordinates access to the resources
> > provided by the ACPI CEDT (CXL Early Discovery Table).
> > 
> > It also coordinates operations of the root port _OSC object to notify
> > platform firmware that the OS has native support for the CXL
> > capabilities of endpoints.
> 
> This doesn't happen here, but in the next patch.
> 
> > Note: the actbl1.h changes are speculative. The expectation is that they
> > will arrive through the ACPICA tree in due time.
> 
> So why don't you put them into a separate patch and drop it from the
> series when not necessary any more?

[snip]

> > +/*
> > + * If/when CXL support is defined by other platform firmware the kernel
> > + * will need a mechanism to select between the platform specific version
> > + * of this routine, until then, hard-code ACPI assumptions
> > + */
> > +int cxl_bus_acquire(struct pci_dev *pdev)
> > +{
> > +       struct acpi_device *adev;
> > +       struct pci_dev *root_port;
> > +       struct device *root;
> > +
> > +       root_port = pcie_find_root_port(pdev);
> > +       if (!root_port)
> > +               return -ENXIO;
> > +
> > +       root = root_port->dev.parent;
> > +       if (!root)
> > +               return -ENXIO;
> > +
> > +       adev = ACPI_COMPANION(root);
> > +       if (!adev)
> > +               return -ENXIO;
> > +
> > +       /* TODO: OSC enabling */
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_bus_acquire);
> 
> I would move the addition of cxl_bus_acquire() entirely to the next
> patch, it looks quite confusing to me as is.

Makes sense - and also agreed with all of your other comments. I've
cleaned this up for the next revision. Thanks Rafael!
>