| Message ID | 20220831081603.3415-10-rrichter@amd.com |
|---|---|
| State | Superseded |
| Delegated to: | Dan Williams |
| Headers | show |
| Series | cxl: Add support for Restricted CXL hosts (RCD mode) | expand |
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. 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. 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. > + continue; > + > + dev_dbg(&adev->dev, "host uid: %llu\n", uid); > + > + if (uid > U32_MAX) > + continue; > + > dev_info(&host->dev, "host supports CXL\n"); > } >
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"); > > } > > >
On Thu, 1 Sep 2022 08:53:36 +0200 Robert Richter <rrichter@amd.com> wrote: > 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. Ok. > > > 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. Hmm. I don't like papering over such bugs as it leads to people not fixing their bios as early as they otherwise would, but fair enough I guess. > > > 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. Ok. All queries answered so Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > -Robert > > > > > > + continue; > > > + > > > + dev_dbg(&adev->dev, "host uid: %llu\n", uid); > > > + > > > + if (uid > U32_MAX) > > > + continue; > > > + > > > dev_info(&host->dev, "host supports CXL\n"); > > > } > > > > >
Robert Richter 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)) > + continue; > + > + dev_dbg(&adev->dev, "host uid: %llu\n", uid); > + > + if (uid > U32_MAX) > + continue; Looks redundant with existing _UID matching.
> On Aug 31, 2022, at 1:15 AM, 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)) The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail if the UID field has value such as ‘CX03’. > + continue; > + > + dev_dbg(&adev->dev, "host uid: %llu\n", uid); > + > + if (uid > U32_MAX) > + continue; > + > dev_info(&host->dev, "host supports CXL\n"); > } > > -- > 2.30.2 > >
Jonathan Zhang (Infra) wrote: > > > > On Aug 31, 2022, at 1:15 AM, 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)) > The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail > if the UID field has value such as ‘CX03’. The UID field is not 4 ASCII characters. We went through this before in the original code in drivers/cxl/acpi.c::add_host_bridge_dport(). The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer() to retrieve the UID to perform the comparison. I thought there was an errata filed to clarify this, but it seems the current spec still just says "value". The CFMWS also places _UID values in the target list, those are also handled as integers.
> On Sep 8, 2022, at 2:10 PM, Dan Williams <dan.j.williams@intel.com> wrote: > > !-------------------------------------------------------------------| > This Message Is From an External Sender > > |-------------------------------------------------------------------! > > Jonathan Zhang (Infra) wrote: >> >> >>> On Aug 31, 2022, at 1:15 AM, 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)) >> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail >> if the UID field has value such as ‘CX03’. > > The UID field is not 4 ASCII characters. > > We went through this before in the original code in > drivers/cxl/acpi.c::add_host_bridge_dport(). > > The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer() > to retrieve the UID to perform the comparison. I thought there was an > errata filed to clarify this, but it seems the current spec still just > says "value". The CFMWS also places _UID values in the target list, > those are also handled as integers. ACPI 6.4 spec section 6.1.12 describes _UID, it says the return value is: An Integer or String containing the Unique ID. In the BIOS I see, the _UIDs of PCIe devices hold ASCII characters (not NULL terminated).
Jonathan Zhang (Infra) wrote: > > > > On Sep 8, 2022, at 2:10 PM, Dan Williams <dan.j.williams@intel.com> wrote: > > > > !-------------------------------------------------------------------| > > This Message Is From an External Sender > > > > |-------------------------------------------------------------------! > > > > Jonathan Zhang (Infra) wrote: > >> > >> > >>> On Aug 31, 2022, at 1:15 AM, 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)) > >> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail > >> if the UID field has value such as ‘CX03’. > > > > The UID field is not 4 ASCII characters. > > > > We went through this before in the original code in > > drivers/cxl/acpi.c::add_host_bridge_dport(). > > > > The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer() > > to retrieve the UID to perform the comparison. I thought there was an > > errata filed to clarify this, but it seems the current spec still just > > says "value". The CFMWS also places _UID values in the target list, > > those are also handled as integers. > > ACPI 6.4 spec section 6.1.12 describes _UID, it says the return value is: > An Integer or String containing the Unique ID. > > In the BIOS I see, the _UIDs of PCIe devices hold ASCII characters (not NULL > terminated). ASCII characters without NULL termination means that data can be treated as binary data which is what current CFMWWS parsing code chooses to do. I think a spec clarification is needed to make resolve the ambiguity.
> On Sep 8, 2022, at 3:31 PM, Dan Williams <dan.j.williams@intel.com> wrote: > > !-------------------------------------------------------------------| > This Message Is From an External Sender > > |-------------------------------------------------------------------! > > Jonathan Zhang (Infra) wrote: >> >> >>> On Sep 8, 2022, at 2:10 PM, Dan Williams <dan.j.williams@intel.com> wrote: >>> >>> !-------------------------------------------------------------------| >>> This Message Is From an External Sender >>> >>> |-------------------------------------------------------------------! >>> >>> Jonathan Zhang (Infra) wrote: >>>> >>>> >>>>> On Aug 31, 2022, at 1:15 AM, 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)) >>>> The UID field holds 4 bytes of ASCII characters. kstrtoull() would fail >>>> if the UID field has value such as ‘CX03’. >>> >>> The UID field is not 4 ASCII characters. >>> >>> We went through this before in the original code in >>> drivers/cxl/acpi.c::add_host_bridge_dport(). >>> >>> The CEDT.CHBS defines _UID as an integer so use acpi_evaluate_integer() >>> to retrieve the UID to perform the comparison. I thought there was an >>> errata filed to clarify this, but it seems the current spec still just >>> says "value". The CFMWS also places _UID values in the target list, >>> those are also handled as integers. >> >> ACPI 6.4 spec section 6.1.12 describes _UID, it says the return value is: >> An Integer or String containing the Unique ID. >> >> In the BIOS I see, the _UIDs of PCIe devices hold ASCII characters (not NULL >> terminated). > > ASCII characters without NULL termination means that data can be > treated as binary data which is what current CFMWWS parsing code chooses > to do. I think a spec clarification is needed to make resolve the > ambiguity. Agreed. In this case, ACPI spec should be referred to.
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)) + continue; + + dev_dbg(&adev->dev, "host uid: %llu\n", uid); + + if (uid > U32_MAX) + continue; + dev_info(&host->dev, "host supports CXL\n"); }
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(+)