On 21-11-22 17:48:16, Jonathan Cameron wrote:
> On Fri, 19 Nov 2021 16:02:48 -0800
> Ben Widawsky <ben.widawsky@xxxxxxxxx> wrote:
>
> > The port driver exists to do proper enumeration of the component
> > registers for ports, including HDM decoder resources. Any port which
> > follows the CXL specification to implement HDM decoder registers should
> > be handled by the port driver. This includes host bridge registers that
> > are currently handled within the cxl_acpi driver.
> >
> > In moving the responsibility from cxl_acpi to cxl_port, three primary
> > things are accomplished here:
> > 1. Multi-port host bridges are now handled by the port driver
> > 2. Single port host bridges are handled by the port driver
> > 3. Single port switches without component registers will be handled by
> > the port driver.
> >
> > While it's tempting to remove decoder APIs from cxl_core entirely, it is
> > still required that platform specific drivers are able to add decoders
> > which aren't specified in CXL 2.0+. An example of this is the CFMWS
> > parsing which is implementing in cxl_acpi.
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@xxxxxxxxx>
> One trivial suggestion inline, but looks fine to me otherwise.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> >
> > ---
> > Changes since RFCv2:
> > - Renamed subject
> > - Reworded commit message
> > - Handle *all* host bridge port enumeration in cxl_port (Dan)
> > - Handle passthrough decoding in cxl_port
> > ---
> > drivers/cxl/acpi.c | 41 +++-----------------------------
> > drivers/cxl/core/bus.c | 6 +++--
> > drivers/cxl/cxl.h | 2 ++
> > drivers/cxl/port.c | 54 +++++++++++++++++++++++++++++++++++++++++-
> > 4 files changed, 62 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index c12e4fed7941..c85a04ecbf7f 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -210,8 +210,6 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > struct acpi_pci_root *pci_root;
> > struct cxl_walk_context ctx;
> > - int single_port_map[1], rc;
> > - struct cxl_decoder *cxld;
> > struct cxl_dport *dport;
> > struct cxl_port *port;
> >
> > @@ -245,43 +243,9 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > return -ENODEV;
> > if (ctx.error)
> > return ctx.error;
> > - if (ctx.count > 1)
> > - return 0;
> >
> > - /* TODO: Scan CHBCR for HDM Decoder resources */
> > -
> > - /*
> > - * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability
> > - * Structure) single ported host-bridges need not publish a decoder
> > - * capability when a passthrough decode can be assumed, i.e. all
> > - * transactions that the uport sees are claimed and passed to the single
> > - * dport. Disable the range until the first CXL region is enumerated /
> > - * activated.
> > - */
> > - cxld = cxl_decoder_alloc(port, 1);
> > - if (IS_ERR(cxld))
> > - return PTR_ERR(cxld);
> > -
> > - cxld->interleave_ways = 1;
> > - cxld->interleave_granularity = PAGE_SIZE;
> > - cxld->target_type = CXL_DECODER_EXPANDER;
> > - cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
> > -
> > - device_lock(&port->dev);
> > - dport = list_first_entry(&port->dports, typeof(*dport), list);
> > - device_unlock(&port->dev);
> > -
> > - single_port_map[0] = dport->port_id;
> > -
> > - rc = cxl_decoder_add(cxld, single_port_map);
> > - if (rc)
> > - put_device(&cxld->dev);
> > - else
> > - rc = cxl_decoder_autoremove(host, cxld);
> > -
> > - if (rc == 0)
> > - dev_dbg(host, "add: %s\n", dev_name(&cxld->dev));
> > - return rc;
> > + /* Host bridge ports are enumerated by the port driver. */
> > + return 0;
> > }
> >
> > struct cxl_chbs_context {
> > @@ -448,3 +412,4 @@ module_platform_driver(cxl_acpi_driver);
> > MODULE_LICENSE("GPL v2");
> > MODULE_IMPORT_NS(CXL);
> > MODULE_IMPORT_NS(ACPI);
> > +MODULE_SOFTDEP("pre: cxl_port");
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 46a06cfe79bd..acfa212eea21 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -62,7 +62,7 @@ void cxl_unregister_topology_host(struct device *host)
> > }
> > EXPORT_SYMBOL_NS_GPL(cxl_unregister_topology_host, CXL);
> >
> > -static struct device *get_cxl_topology_host(void)
> > +struct device *get_cxl_topology_host(void)
> > {
> > down_read(&topology_host_sem);
> > if (cxl_topology_host)
> > @@ -70,12 +70,14 @@ static struct device *get_cxl_topology_host(void)
> > up_read(&topology_host_sem);
> > return NULL;
> > }
> > +EXPORT_SYMBOL_NS_GPL(get_cxl_topology_host, CXL);
> >
> > -static void put_cxl_topology_host(struct device *dev)
> > +void put_cxl_topology_host(struct device *dev)
> > {
> > WARN_ON(dev != cxl_topology_host);
> > up_read(&topology_host_sem);
> > }
> > +EXPORT_SYMBOL_NS_GPL(put_cxl_topology_host, CXL);
> >
> > static int decoder_match(struct device *dev, void *data)
> > {
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 24fa16157d5e..f8354241c5a3 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -170,6 +170,8 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> >
> > int cxl_register_topology_host(struct device *host);
> > void cxl_unregister_topology_host(struct device *host);
> > +struct device *get_cxl_topology_host(void);
> > +void put_cxl_topology_host(struct device *dev);
> >
> > /*
> > * cxl_decoder flags that define the type of memory / devices this
> > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> > index 3c03131517af..7a1fc726fe9f 100644
> > --- a/drivers/cxl/port.c
> > +++ b/drivers/cxl/port.c
> > @@ -233,12 +233,64 @@ static int enumerate_hdm_decoders(struct cxl_port *port,
> > return 0;
> > }
> >
> > +/*
> > + * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability Structure)
> > + * single ported host-bridges need not publish a decoder capability when a
> > + * passthrough decode can be assumed, i.e. all transactions that the uport sees
> > + * are claimed and passed to the single dport. Disable the range until the first
> > + * CXL region is enumerated / activated.
> > + */
> > +static int add_passthrough_decoder(struct cxl_port *port)
> > +{
> > + int single_port_map[1], rc;
> > + struct cxl_decoder *cxld;
> > + struct cxl_dport *dport;
> > +
> > + device_lock_assert(&port->dev);
> > +
> > + cxld = cxl_decoder_alloc(port, 1);
> > + if (IS_ERR(cxld))
> > + return PTR_ERR(cxld);
> > +
> > + cxld->interleave_ways = 1;
> > + cxld->interleave_granularity = PAGE_SIZE;
> > + cxld->target_type = CXL_DECODER_EXPANDER;
> > + cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
> > +
> > + dport = list_first_entry(&port->dports, typeof(*dport), list);
> > + single_port_map[0] = dport->port_id;
> > +
> > + rc = cxl_decoder_add_locked(cxld, single_port_map);
> > + if (rc)
> > + put_device(&cxld->dev);
>
> I would handle this error path entirely here, or use a goto rather
> than messing up the good path with conditionals on each element,
> particularly as there isn't much to do in the error paths.
> I guess this might get more complicated in later patches though.
>
> Obviously that tidy up would make this more complex than simply moving
> the code. (I might have commented on this before, but too long ago to remember ;)
>
> if (rc) {
> put_device(&cxld->dev);
> return rc;
> }
> rc = cxl_decoder...
> if (rc)
> return rc;
>
> dev_dbg(..
>
> return 0;
>
Since I changed this in v2 for the port driver, in the last patch, I think it
makes sense to do the move, and then cleanup.
Thanks.
> > + else
> > + rc = cxl_decoder_autoremove(&port->dev, cxld);
> > +
> > + if (rc == 0)
> > + dev_dbg(&port->dev, "add: %s\n", dev_name(&cxld->dev));
> > +
> > + return rc;
> > +}
> > +
