| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 2 Oct 2023 15:32:38 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Richter <rrichter@....com>
CC: Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>,
Ira Weiny <ira.weiny@...el.com>,
Ben Widawsky <bwidawsk@...nel.org>,
Dan Williams <dan.j.williams@...el.com>,
"Davidlohr Bueso" <dave@...olabs.net>,
Dave Jiang <dave.jiang@...el.com>,
Terry Bowman <terry.bowman@....com>,
<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v11 03/20] cxl/port: Fix @host confusion in
cxl_dport_setup_regs()
On Wed, 27 Sep 2023 17:43:22 +0200
Robert Richter <rrichter@....com> wrote:
> From: Dan Williams <dan.j.williams@...el.com>
>
> commit 5d2ffbe4b81a ("cxl/port: Store the downstream port's Component Register mappings in struct cxl_dport")
>
> ...moved the dport component registers from a raw component_reg_phys
> passed in at dport instantiation time to a 'struct cxl_register_map'
> populated with both the component register data *and* the "host" device
> for mapping operations.
>
> While typical CXL switch dports are mapped by their associated 'struct
> cxl_port', an RCH host bridge dport registered by cxl_acpi needs to wait
> until the cxl_mem driver makes the attachment to map the registers. This
> is because there are no intervening 'struct cxl_port' instances between
> the root cxl_port and the endpoint port in an RCH topology.
>
> For now just mark the host as NULL in the RCH dport case until code that
> needs to map the dport registers arrives. Name the field @reg_map,
> because @reg_map->host will be used for mapping operations beyond
> component registers (i.e. AER registers).
I'm not keen on a rename buried in here. Its fine in general but as
far as I can see little to do with the rest of what is going on here.
>
> This patch is not flagged for -stable since nothing in the current
> driver uses the dport->reg_map.
>
> Now, I am slightly uneasy that cxl_setup_comp_regs() sets map->host to a
> wrong value and then cxl_dport_setup_regs() fixes it up, but the
> alternatives I came up with are more messy. For example, adding an
> @logdev to 'struct cxl_register_map' that the dev_printk()s can fall
> back to when @host is NULL. I settled on "post-fixup+comment" since it
> is only RCH dports that have this special case where register probing is
> split between a host-bridge RCRB lookup and when cxl_mem_probe() does
> the association of the cxl_memdev and endpoint port.
>
> Fixes: 5d2ffbe4b81a ("cxl/port: Store the downstream port's Component Register mappings in struct cxl_dport")
> Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> [kept dev_dbg() message]
> Signed-off-by: Robert Richter <rrichter@....com>
Seems right to me otherwise, though I've lost track a little of what was
going on with this... Hence a tentative
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> ---
> drivers/cxl/core/port.c | 43 +++++++++++++++++++++++++++++------------
> drivers/cxl/cxl.h | 4 ++--
> 2 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 033651a5da30..99df86d72dbc 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -716,13 +716,23 @@ static int cxl_port_setup_regs(struct cxl_port *port,
> component_reg_phys);
> }
>
> -static int cxl_dport_setup_regs(struct cxl_dport *dport,
> +static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
> resource_size_t component_reg_phys)
> {
> + int rc;
> +
> if (dev_is_platform(dport->dport_dev))
> return 0;
> - return cxl_setup_comp_regs(dport->dport_dev, &dport->comp_map,
> - component_reg_phys);
> +
> + /*
> + * use @dport->dport_dev for the context for error messages during
> + * register probing, and fixup @host after the fact, since @host may be
> + * NULL.
> + */
> + rc = cxl_setup_comp_regs(dport->dport_dev, &dport->reg_map,
> + component_reg_phys);
> + dport->reg_map.host = host;
> + return rc;
> }
>
> static struct cxl_port *__devm_cxl_add_port(struct device *host,
> @@ -983,7 +993,16 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> if (!dport)
> return ERR_PTR(-ENOMEM);
>
> - if (rcrb != CXL_RESOURCE_NONE) {
> + dport->dport_dev = dport_dev;
> + dport->port_id = port_id;
> + dport->port = port;
> +
> + if (rcrb == CXL_RESOURCE_NONE) {
> + rc = cxl_dport_setup_regs(&port->dev, dport,
> + component_reg_phys);
> + if (rc)
> + return ERR_PTR(rc);
> + } else {
> dport->rcrb.base = rcrb;
> component_reg_phys = __rcrb_to_component(dport_dev, &dport->rcrb,
> CXL_RCRB_DOWNSTREAM);
> @@ -992,6 +1011,14 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> return ERR_PTR(-ENXIO);
> }
>
> + /*
> + * RCH @dport is not ready to map until associated with its
> + * memdev
> + */
> + rc = cxl_dport_setup_regs(NULL, dport, component_reg_phys);
> + if (rc)
> + return ERR_PTR(rc);
> +
> dport->rch = true;
> }
>
> @@ -999,14 +1026,6 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> dev_dbg(dport_dev, "Component Registers found for dport: %pa\n",
> &component_reg_phys);
>
> - dport->dport_dev = dport_dev;
> - dport->port_id = port_id;
> - dport->port = port;
> -
> - rc = cxl_dport_setup_regs(dport, component_reg_phys);
> - if (rc)
> - return ERR_PTR(rc);
> -
> cond_cxl_root_lock(port);
> rc = add_dport(port, dport);
> cond_cxl_root_unlock(port);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b5b015b661ea..68abf9944383 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -620,7 +620,7 @@ struct cxl_rcrb_info {
> /**
> * struct cxl_dport - CXL downstream port
> * @dport_dev: PCI bridge or firmware device representing the downstream link
> - * @comp_map: component register capability mappings
> + * @reg_map: component and ras register mapping parameters
> * @port_id: unique hardware identifier for dport in decoder target list
> * @rcrb: Data about the Root Complex Register Block layout
> * @rch: Indicate whether this dport was enumerated in RCH or VH mode
> @@ -628,7 +628,7 @@ struct cxl_rcrb_info {
> */
> struct cxl_dport {
> struct device *dport_dev;
> - struct cxl_register_map comp_map;
> + struct cxl_register_map reg_map;
> int port_id;
> struct cxl_rcrb_info rcrb;
> bool rch;
Powered by blists - more mailing lists