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

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> 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 :(

I think there were thoughts on a common 'struct cxl_device' indirection,
but fell through and never came back to cleanup these definitions.

I think for now add a comment about this being needed for the most
prevalent CXL object 'struct cxl_port', but that longer term we may want
to consider separate 'struct cxl_port_driver' and 'struct
cxl_memdev_driver' wrappers.

That would save needing to up-cast the 'struct device *' argument as the
first step of these probe routines.