Re: [PATCH 5/9] cxl/port: Move dport probe operations to a driver event

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Wed, 21 Jan 2026 19:33:26 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for adding more register setup to the cxl_port_add_dport()
> path (for RAS register mapping), move the dport creation event to a driver
> callback. This achieves two goals, it puts driver operations logically
> where they belong, in a driver, and it obviates the gymnastics of
> DECLARE_TESTABLE() which just makes a mess of grepping for CXL symbols.
> 
> In other words, a driver callback is less of an ongoing maintenance burden
> than this DECLARE_TESTABLE arrangement that does not scale and diminishes
> the grep-ability of the codebase.
> 
> cxl_port_add_dport() moves mostly unmodified from drivers/cxl/core/port.c.
> The only deliberate change is that it now assumes that the device_lock is
> held on entry and the driver is attached (just like cxl_port_probe()).
> 
> Reviewed-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Just a small comment on maybe adding some scary sounding docs, or
a struct wrapper to deal with the very specific callback inside
struct cxl_driver.

With at least a comment,
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/cxl/cxl.h                    | 25 +++------
>  tools/testing/cxl/exports.h          | 13 -----
>  drivers/cxl/core/hdm.c               |  6 +--
>  drivers/cxl/core/pci.c               |  8 +--
>  drivers/cxl/core/port.c              | 78 +++++++---------------------
>  drivers/cxl/port.c                   | 60 +++++++++++++++++++++
>  tools/testing/cxl/cxl_core_exports.c | 22 --------
>  tools/testing/cxl/test/mock.c        | 24 +++------
>  tools/testing/cxl/Kbuild             |  2 +
>  9 files changed, 102 insertions(+), 136 deletions(-)
>  delete mode 100644 tools/testing/cxl/exports.h
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6f3741a57932..75ff5f055f7f 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -840,8 +840,11 @@ struct cxl_endpoint_dvsec_info {
>  };
>  
>  int devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
> -int __devm_cxl_switch_port_decoders_setup(struct cxl_port *port);
>  int devm_cxl_endpoint_decoders_setup(struct cxl_port *port);
> +void cxl_port_update_decoder_targets(struct cxl_port *port,
> +				     struct cxl_dport *dport);
> +int cxl_port_setup_regs(struct cxl_port *port,
> +			resource_size_t component_reg_phys);
>  
>  struct cxl_dev_state;
>  int cxl_dvsec_rr_decode(struct cxl_dev_state *cxlds,
> @@ -855,6 +858,8 @@ struct cxl_driver {
>  	const char *name;
>  	int (*probe)(struct device *dev);
>  	void (*remove)(struct device *dev);
> +	struct cxl_dport *(*add_dport)(struct cxl_port *port,
> +				       struct device *dport_dev);

Having a single callback in a generic driver struct seems unusual.
Maybe wrap the structure or add some documentation to set expectations.

Probably not the time for it, but why do we have our own probe and remove rather
than using the ones in device_driver?  Maybe we talked about this long
ago, but I can't remember the answer :(


>  	struct device_driver drv;
>  	int id;
>  };