Re: [PATCH 09/15] cxl/acpi: Determine PCI host bridge's ACPI UID

From: Robert Richter <rrichter@amd.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Ben Widawsky <bwidawsk@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>
Subject: Re: [PATCH 09/15] cxl/acpi: Determine PCI host bridge's ACPI UID
Date: Thu, 1 Sep 2022 08:53:36 +0200	[thread overview]
Message-ID: <YxBW8Kz/bgoMsAee@rric.localdomain> (raw)
In-Reply-To: <20220831120027.000017b3@huawei.com>

On 31.08.22 12:00:27, Jonathan Cameron wrote:
> On Wed, 31 Aug 2022 10:15:57 +0200
> Robert Richter <rrichter@amd.com> wrote:
> 
> > The UID is needed to read the RCH's CEDT entry with the RCRB base
> > address. Determine the host's UID from its ACPI fw node.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/cxl/acpi.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index f9cdf23a91a8..b3146b7ae922 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -368,8 +368,20 @@ struct pci_host_bridge *cxl_find_next_rch(struct pci_host_bridge *host)
> >  static int __init cxl_restricted_host_probe(struct platform_device *pdev)
> >  {
> >  	struct pci_host_bridge *host = NULL;
> > +	struct acpi_device *adev;
> > +	unsigned long long uid = ~0;
> >  
> >  	while ((host = cxl_find_next_rch(host)) != NULL) {
> > +		adev = ACPI_COMPANION(&host->dev);
> > +		if (!adev || !adev->pnp.unique_id ||
> > +			(kstrtoull(adev->pnp.unique_id, 10, &uid) < 0))
> 
> There is an acpi_device_uid() accessor function that should probably be
> used here.

That accessor actually does not help really, there is no null pointer
check for adev. Using it actually adds more complexity since another
variable is introduced plus you need to look at the function's
implementation anyway.

The adev->pnp.unique_id access pattern is widely used in the kernel, I
don't expect changes in the data struct here.

> Also, should a fialure to convert to an integer (or one within
> limits) be something we paper over?  Feels like we should fail
> hard if that happens.

This is a real corner case and close to a broken firmware
implementation. I think current dbg messages are good to find where
the detection stops.

> Admittedly I can't immediately find any spec that states that
> the _UID should be either integer or under 32 bits...
> ACPI allows a string and CXL just says it's 4 bytes long.

IIRC the UID can be implemented as string or 8 bytes, there is no
limitation then. That's why the range check below.

-Robert

> 
> > +			continue;
> > +
> > +		dev_dbg(&adev->dev, "host uid: %llu\n", uid);
> > +
> > +		if (uid > U32_MAX)
> > +			continue;
> > +
> >  		dev_info(&host->dev, "host supports CXL\n");
> >  	}
> >  
> 