| Message ID | 165237928334.3832067.16326444799383620141.stgit@dwillia2-desk3.amr.corp.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Fix "mem_enable" handling | expand |
On Thu, 12 May 2022 11:14:43 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > In preparation for validating DVSEC ranges against the platform declared > CXL memory ranges (ACPI CFMWS) move port enumeration before the > endpoint's decoder validation. Ultimately this logic will move to the > port driver, but create a bisect point before that larger move. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/cxl/mem.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index fed7f10ef9b2..80e75a410499 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev) > if (work_pending(&cxlmd->detach_work)) > return -EBUSY; > > - rc = cxlds->wait_media_ready(cxlds); > - if (rc) { > - dev_err(dev, "Media not active (%d)\n", rc); > - return rc; > - } > - > - /* > - * If DVSEC ranges are being used instead of HDM decoder registers there > - * is no use in trying to manage those. > - */ > - if (!cxl_hdm_decode_init(cxlds)) { > - dev_err(dev, > - "Legacy range registers configuration prevents HDM operation.\n"); > - return -EBUSY; > - } > - > rc = devm_cxl_enumerate_ports(cxlmd); > if (rc) > return rc; > @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev) > dev_err(dev, "CXL port topology %s not enabled\n", > dev_name(&parent_port->dev)); > rc = -ENXIO; > - goto out; > + goto unlock; > } > > rc = create_endpoint(cxlmd, parent_port); > -out: > +unlock: Trivial but I think this rename of the label would be better in the earlier patch where you first added the if (rc) below. I suppose it is arguable that the previous patch is a fix so should be minimal though so I'm not that bothered if you leave it here. > device_unlock(&parent_port->dev); > put_device(&parent_port->dev); > if (rc) > return rc; > > + rc = cxlds->wait_media_ready(cxlds); > + if (rc) { > + dev_err(dev, "Media not active (%d)\n", rc); > + return rc; > + } > + > + /* > + * If DVSEC ranges are being used instead of HDM decoder registers there > + * is no use in trying to manage those. > + */ > + if (!cxl_hdm_decode_init(cxlds)) { > + dev_err(dev, > + "Legacy range registers configuration prevents HDM operation.\n"); > + return -EBUSY; > + } > + > /* > * The kernel may be operating out of CXL memory on this device, > * there is no spec defined way to determine whether this device >
On Wed, May 18, 2022 at 9:15 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 12 May 2022 11:14:43 -0700 > Dan Williams <dan.j.williams@intel.com> wrote: > > > In preparation for validating DVSEC ranges against the platform declared > > CXL memory ranges (ACPI CFMWS) move port enumeration before the > > endpoint's decoder validation. Ultimately this logic will move to the > > port driver, but create a bisect point before that larger move. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/cxl/mem.c | 36 ++++++++++++++++++------------------ > > 1 file changed, 18 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > index fed7f10ef9b2..80e75a410499 100644 > > --- a/drivers/cxl/mem.c > > +++ b/drivers/cxl/mem.c > > @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev) > > if (work_pending(&cxlmd->detach_work)) > > return -EBUSY; > > > > - rc = cxlds->wait_media_ready(cxlds); > > - if (rc) { > > - dev_err(dev, "Media not active (%d)\n", rc); > > - return rc; > > - } > > - > > - /* > > - * If DVSEC ranges are being used instead of HDM decoder registers there > > - * is no use in trying to manage those. > > - */ > > - if (!cxl_hdm_decode_init(cxlds)) { > > - dev_err(dev, > > - "Legacy range registers configuration prevents HDM operation.\n"); > > - return -EBUSY; > > - } > > - > > rc = devm_cxl_enumerate_ports(cxlmd); > > if (rc) > > return rc; > > @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev) > > dev_err(dev, "CXL port topology %s not enabled\n", > > dev_name(&parent_port->dev)); > > rc = -ENXIO; > > - goto out; > > + goto unlock; > > } > > > > rc = create_endpoint(cxlmd, parent_port); > > -out: > > +unlock: > > Trivial but I think this rename of the label would be better in the earlier > patch where you first added the if (rc) below. > I suppose it is arguable that the previous patch is a fix so should be minimal > though so I'm not that bothered if you leave it here. > True, the rename is just as much a part of what "9ea4dcf49878 PM: CXL: Disable suspend" was missing as the fix, I'll go ahead and make the adjustment.
On Wed, 18 May 2022 09:41:43 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > On Wed, May 18, 2022 at 9:15 AM Jonathan Cameron > <Jonathan.Cameron@huawei.com> wrote: > > > > On Thu, 12 May 2022 11:14:43 -0700 > > Dan Williams <dan.j.williams@intel.com> wrote: > > > > > In preparation for validating DVSEC ranges against the platform declared > > > CXL memory ranges (ACPI CFMWS) move port enumeration before the > > > endpoint's decoder validation. Ultimately this logic will move to the > > > port driver, but create a bisect point before that larger move. > > > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > > --- > > > drivers/cxl/mem.c | 36 ++++++++++++++++++------------------ > > > 1 file changed, 18 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > index fed7f10ef9b2..80e75a410499 100644 > > > --- a/drivers/cxl/mem.c > > > +++ b/drivers/cxl/mem.c > > > @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev) > > > if (work_pending(&cxlmd->detach_work)) > > > return -EBUSY; > > > > > > - rc = cxlds->wait_media_ready(cxlds); > > > - if (rc) { > > > - dev_err(dev, "Media not active (%d)\n", rc); > > > - return rc; > > > - } > > > - > > > - /* > > > - * If DVSEC ranges are being used instead of HDM decoder registers there > > > - * is no use in trying to manage those. > > > - */ > > > - if (!cxl_hdm_decode_init(cxlds)) { > > > - dev_err(dev, > > > - "Legacy range registers configuration prevents HDM operation.\n"); > > > - return -EBUSY; > > > - } > > > - > > > rc = devm_cxl_enumerate_ports(cxlmd); > > > if (rc) > > > return rc; > > > @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev) > > > dev_err(dev, "CXL port topology %s not enabled\n", > > > dev_name(&parent_port->dev)); > > > rc = -ENXIO; > > > - goto out; > > > + goto unlock; > > > } > > > > > > rc = create_endpoint(cxlmd, parent_port); > > > -out: > > > +unlock: > > > > Trivial but I think this rename of the label would be better in the earlier > > patch where you first added the if (rc) below. > > I suppose it is arguable that the previous patch is a fix so should be minimal > > though so I'm not that bothered if you leave it here. > > > > True, the rename is just as much a part of what "9ea4dcf49878 PM: CXL: > Disable suspend" was missing as the fix, I'll go ahead and make the > adjustment. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> I'm lazy enough not to look again for something so trivial ;)
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index fed7f10ef9b2..80e75a410499 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev) if (work_pending(&cxlmd->detach_work)) return -EBUSY; - rc = cxlds->wait_media_ready(cxlds); - if (rc) { - dev_err(dev, "Media not active (%d)\n", rc); - return rc; - } - - /* - * If DVSEC ranges are being used instead of HDM decoder registers there - * is no use in trying to manage those. - */ - if (!cxl_hdm_decode_init(cxlds)) { - dev_err(dev, - "Legacy range registers configuration prevents HDM operation.\n"); - return -EBUSY; - } - rc = devm_cxl_enumerate_ports(cxlmd); if (rc) return rc; @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev) dev_err(dev, "CXL port topology %s not enabled\n", dev_name(&parent_port->dev)); rc = -ENXIO; - goto out; + goto unlock; } rc = create_endpoint(cxlmd, parent_port); -out: +unlock: device_unlock(&parent_port->dev); put_device(&parent_port->dev); if (rc) return rc; + rc = cxlds->wait_media_ready(cxlds); + if (rc) { + dev_err(dev, "Media not active (%d)\n", rc); + return rc; + } + + /* + * If DVSEC ranges are being used instead of HDM decoder registers there + * is no use in trying to manage those. + */ + if (!cxl_hdm_decode_init(cxlds)) { + dev_err(dev, + "Legacy range registers configuration prevents HDM operation.\n"); + return -EBUSY; + } + /* * The kernel may be operating out of CXL memory on this device, * there is no spec defined way to determine whether this device
In preparation for validating DVSEC ranges against the platform declared CXL memory ranges (ACPI CFMWS) move port enumeration before the endpoint's decoder validation. Ultimately this logic will move to the port driver, but create a bisect point before that larger move. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/mem.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)