Re: [PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Alison Schofield]

Thanks for the review Ben -

On Wed, Jun 16, 2021 at 09:08:16AM -0700, Ben Widawsky wrote:
> On 21-06-15 17:20:38, Alison Schofield wrote:
> > The base address for the Host Bridge port component registers is located
> > in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
> > Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
> > and include that base address in the port object.
> > 
> > Co-developed-by: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> > Signed-off-by: Vishal Verma <vishal.l.verma@xxxxxxxxx>
> > Signed-off-by: Alison Schofield <alison.schofield@xxxxxxxxx>
> > ---
> >  drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 99 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index be357eea552c..b6d9cd45428c 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -8,6 +8,61 @@
> >  #include <linux/pci.h>
> >  #include "cxl.h"
> >  
> > +static struct acpi_table_header *cedt_table;
> 
> cedt_header would really be a better name. "Table" is redundant as the 't' in
> CEDT is table.
> 

Agree. Renamed to acpi_cedt in v3. See if you like.

> > +
> > +static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
> > +{
> > +	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> > +	acpi_size len, cur = 0;
> > +	void *cedt_base;
> 
> maybe "cedt_body", or "cedt_subtables"

got it in v3.

> 
> > +	int rc = 0;
> > +
> > +	len = cedt_table->length - sizeof(*cedt_table);
> > +	cedt_base = cedt_table + 1;
> 
> As per naming recommendation above, this looks really funny...
> 
:)
> > +
> > +	while (cur < len) {
> > +		struct acpi_cedt_header *c = cedt_base + cur;
> 
> Okay, now I see why you may have not called the previous thing a header.
> 
> > +
> > +		if (c->type != ACPI_CEDT_TYPE_CHBS) {
> > +			cur += c->length;
> > +			continue;
> > +		}
> > +
> > +		chbs = cedt_base + cur;
> > +
> > +		if (chbs->header.length < sizeof(*chbs)) {
> > +			dev_err(dev, "Invalid CHBS header length: %u\n",
> > +				chbs->header.length);
> > +			rc = -EINVAL;
> > +			break;
> > +		}
> 
> I'd just continue here. Maybe there will be another chbs with the correct size.
>

Got it.

> > +
> > +		if (chbs->uid == uid && !chbs_match) {
> > +			chbs_match = chbs;
> > +			cur += c->length;
> > +			continue;
> > +		}
> > +
> > +		if (chbs->uid == uid && chbs_match) {
> > +			dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);
> > +			rc = -EINVAL;
> > +			break;
> > +		}
> 
> I'd also just continue here. I think if we have a match, we can just use it and
> ignore BIOS bugs. I'd probably write it like this:
> 
> if (chbs->uid == uid) {
> 	dev_WARN_ONCE(dev, chbs_match, "Duplicate CHBS UIDs %u\n", uid);
> 	chbs_match = chbs; /* last one wins */
> 	cur += c->length;
> 	continue;
> }
> 
> Up to you how you actually write it, but do consider not failing here.
>

Thanks for the snippet. I added the dev_WARN_ONCE to both the length
mismatch and duplicate cases. 


> > +		cur += c->length;
> > +	}
> > +	if (!chbs_match)
> > +		rc = -EINVAL;
> 
> Maybe ENODEV or something like it is more appropriate?

Got it.

> 
snip