[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 11 Nov 2020 07:30:34 +0000
From: "Verma, Vishal L" <vishal.l.verma@...el.com>
To: "Widawsky, Ben" <ben.widawsky@...el.com>,
"hch@...radead.org" <hch@...radead.org>
CC: "Kelley, Sean V" <sean.v.kelley@...el.com>,
"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
"linux-cxl@...r.kernel.org" <linux-cxl@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Williams, Dan J" <dan.j.williams@...el.com>,
"Weiny, Ira" <ira.weiny@...el.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [RFC PATCH 1/9] cxl/acpi: Add an acpi_cxl module for the CXL
interconnect
On Wed, 2020-11-11 at 07:10 +0000, Christoph Hellwig wrote:
> On Tue, Nov 10, 2020 at 09:43:48PM -0800, Ben Widawsky wrote:
> > +menuconfig CXL_BUS
> > + tristate "CXL (Compute Express Link) Devices Support"
> > + help
> > + CXL is a bus that is electrically compatible with PCI-E, but layers
> > + three protocols on that signalling (CXL.io, CXL.cache, and CXL.mem). The
> > + CXL.cache protocol allows devices to hold cachelines locally, the
> > + CXL.mem protocol allows devices to be fully coherent memory targets, the
> > + CXL.io protocol is equivalent to PCI-E. Say 'y' to enable support for
> > + the configuration and management of devices supporting these protocols.
> > +
>
> Please fix the overly long lines.
>
> > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev)
>
> Another overly long line.
Hi Christpph,
I thought 100 col. lines were acceptable now.
>
> > +{
> > + dev_set_drvdata(dev, acpi_desc);
> > + acpi_desc->dev = dev;
> > +}
>
> But this helper seems pretty pointless to start with.
>
> > +static int acpi_cxl_remove(struct acpi_device *adev)
> > +{
> > + return 0;
> > +}
>
> The emptry remove callback is not needed.
Agreed on both of the above comments - these are just boilerplate for
now, I expect they will get filled in in the next revision as more
functionality gets fleshed out. If they are still empty/no-op by then I
will remove them.
>
> > +/*
> > + * 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_prepared(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_prepared);
>
> What is the point of this function? I doesn't realy do anything,
> not even a CXL specific check.
This gets a bit more fleshed out in patch 2. I kept that separate so
that it is easier to review the bulk of the _OSC work in that patch
without this driver boilerplate getting in the way.
>
> >
> > +/*******************************************************************************
> > + *
> > + * CEDT - CXL Early Discovery Table (ACPI 6.4)
> > + * Version 1
> > + *
> > + ******************************************************************************/
> > +
>
> Pleae use the normal Linux comment style.
>
>
> > +#define ACPI_CEDT_CHBS_VERSION_CXL11 (0)
> > +#define ACPI_CEDT_CHBS_VERSION_CXL20 (1)
> > +
> > +/* Values for length field above */
> > +
> > +#define ACPI_CEDT_CHBS_LENGTH_CXL11 (0x2000)
> > +#define ACPI_CEDT_CHBS_LENGTH_CXL20 (0x10000)
>
> No need for the braces.
For both of these - see the note in the commit message. I just followed
the ACPI header's style, and these hunks are only in this series to make
it usable. I expect the 'actual' struct definitions, naming etc will
come through ACPICA.
Powered by blists - more mailing lists