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@xxxxxxxxx> wrote:
> >
> > From: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> >
> > 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!
>
