lkml.org 
[lkml]   [2020]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC PATCH 1/9] cxl/acpi: Add an acpi_cxl module for the CXL interconnect
    Date
    On Mon, 2020-11-16 at 17:59 +0000, Jonathan Cameron wrote:
    > On Tue, 10 Nov 2020 21:43:48 -0800
    > 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).
    >
    > I think the qemu series notes that this ACPI0017 is just a proposal at
    > this stage. Please make sure that's highlighted here as well unless
    > that status is out of date.

    Hi Jonathan,

    Thank you for the review. The cover letter talks about this, but I agree
    it would be worth repeating in this patch briefly as I did with the OSC
    patch. If it is still in a proposal state by the next posting, I'll make
    sure to add a note here too.

    >
    > > +
    > > +static void acpi_cxl_desc_init(struct acpi_cxl_desc *acpi_desc, struct device *dev)
    > > +{
    > > + dev_set_drvdata(dev, acpi_desc);
    >
    > No need to have this wrapper + it hides the fact you are not just initialsing
    > the acpi_desc structure.
    >
    > > + acpi_desc->dev = dev;
    > > +}
    > > +
    > > +static void acpi_cedt_put_table(void *table)
    > > +{
    > > + acpi_put_table(table);
    > > +}
    > > +
    > > +static int acpi_cxl_add(struct acpi_device *adev)
    > > +{
    > > + struct acpi_cxl_desc *acpi_desc;
    > > + struct device *dev = &adev->dev;
    > > + struct acpi_table_header *tbl;
    > > + acpi_status status = AE_OK;
    >
    > Set below, so don't do it here.
    >
    > > + acpi_size sz;
    > > + int rc = 0;
    >
    > Set in paths in which it's used so don't do it here.
    >
    > > +
    > > + status = acpi_get_table(ACPI_SIG_CEDT, 0, &tbl);
    > > + if (ACPI_FAILURE(status)) {
    > > + dev_err(dev, "failed to find CEDT at startup\n");
    > > + return 0;
    > > + }
    > > +
    > > + rc = devm_add_action_or_reset(dev, acpi_cedt_put_table, tbl);
    > > + if (rc)
    > > + return rc;
    >
    > blank line here preferred for readability (do something, then check errors as
    > block)
    >
    > > + sz = tbl->length;
    > > + dev_info(dev, "found CEDT at startup: %lld bytes\n", sz);
    > > +
    > > + acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
    > > + if (!acpi_desc)
    > > + return -ENOMEM;
    >
    > blank line here slightly helps readability.
    >
    > > + acpi_cxl_desc_init(acpi_desc, &adev->dev);
    > > +
    > > + acpi_desc->acpi_header = *tbl;
    > > +
    > > + return 0;
    > > +}
    > > +
    > > +static int acpi_cxl_remove(struct acpi_device *adev)
    > > +{
    > > + return 0;
    >
    > Don't think empty remove is needed.
    >

    Agreed with all of the above, I'll fix for the next posting.

    > > +
    > > +/* Values for CEDT structure types */
    > > +
    > > +enum acpi_cedt_type {
    > > + ACPI_CEDT_TYPE_HOST_BRIDGE = 0, /* CHBS - CXL Host Bridge Structure */
    > > + ACPI_CEDT_TYPE_CFMWS = 1, /* CFMWS - CXL Fixed Memory Window Structure */
    >
    > This isn't in the 2.0 spec, so I guess also part of some proposed changes.

    Yes this got in accidentally, will remove.


    \
     
     \ /
      Last update: 2020-11-16 19:27    [W:6.267 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site