Message-ID: <aRkx3OhRHQrCEhow@aschofie-mobl2.lan>
Date: Sat, 15 Nov 2025 18:07:24 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>
CC: <alejandro.lucero-palau@....com>, <linux-cxl@...r.kernel.org>,
	<netdev@...r.kernel.org>, <dan.j.williams@...el.com>, <edward.cree@....com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <dave.jiang@...el.com>, Ben Cheatham
	<benjamin.cheatham@....com>, Fan Ni <fan.ni@...sung.com>, Jonathan Cameron
	<Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v20 06/22] cxl: Move pci generic code

On Sat, Nov 15, 2025 at 08:16:29AM +0000, Alejandro Lucero Palau wrote:
> 
> On 11/14/25 00:25, Alison Schofield wrote:
> > On Mon, Nov 10, 2025 at 03:36:41PM +0000, alejandro.lucero-palau@....com wrote:
> > > From: Alejandro Lucero <alucerop@....com>
> > > 
> > > Inside cxl/core/pci.c there are helpers for CXL PCIe initialization
> > > meanwhile cxl/pci.c implements the functionality for a Type3 device
> > > initialization.
> > Hi Alejandro,
> > 
> > I'v been looking at Terry's set and the cxl-test build circular
> > dependencies. I think this patch may be 'stale', at least in
> > the comments, maybe in the wrapped function it removes.
> 
> 
> Hi Allison,
> 
> 
> I think you are right regarding the comments. I did not update them after
> Terry's changes.
> 

Here's how it looks to me, and looks odd :

Terry moves the entirety of cxl/pci.c into a new file
cxl/core/pci_drv.c

Then you move some of the things from that new cxl/core/pci_drv.c
into the existing cxl/core/pci.c.

My question is, for these pieces that belong in cxl/core/pci.c might
it be better for Terry just to move them there in the first place?

> 
> > > Move helper functions from cxl/pci.c to cxl/core/pci.c in order to be
> > > exported and shared with CXL Type2 device initialization.
> > Terry moves the whole file cxl/pci.c to cxl/core/pci_drv.c.
> > That is reflected in what you actually do below, but not in this
> > comment.
> > 
> > > Fix cxl mock tests affected by the code move, deleting a function which
> > > indeed was not being used since commit 733b57f262b0("cxl/pci: Early
> > > setup RCH dport component registers from RCRB").
> > This I'm having trouble figuring out. I see __wrap_cxl_rcd_component_reg_phys()
> > deleted below. Why is that OK? The func it wraps is still in use below, ie it's
> > one you move from core/pci_drv.c to core/pci.c.
> 
> 
> I think the comment refers to usage inside the tests. Are you having
> problems or seeing any problem with this removal?

You may have seen, Terry's set had build problems around that function.
If you see it is no longer needed, can you spin that off and let's do
that clean up separately. Correct me if it is indeed tied to this
patch or patchset. I don't set it.

Thanks!

> 
> 
> Thank you.
> 
> 
> 
> 
> > 
> > For my benefit, what is the intended difference between what will be
> > in core/pci.c and core/pci_drv.c ?
> > 
> > --Alison
> > 
> > > Signed-off-by: Alejandro Lucero <alucerop@....com>
> > > Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> > > Reviewed-by: Ben Cheatham <benjamin.cheatham@....com>
> > > Reviewed-by: Fan Ni <fan.ni@...sung.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > > Reviewed-by: Alison Schofield <alison.schofield@...el.com>
> > > Reviewed-by: Dan Williams <dan.j.williams@...el.com>
> > > ---
> > >   drivers/cxl/core/core.h       |  3 ++
> > >   drivers/cxl/core/pci.c        | 62 +++++++++++++++++++++++++++++++
> > >   drivers/cxl/core/pci_drv.c    | 70 -----------------------------------
> > >   drivers/cxl/core/regs.c       |  1 -
> > >   drivers/cxl/cxl.h             |  2 -
> > >   drivers/cxl/cxlpci.h          | 13 +++++++
> > >   tools/testing/cxl/Kbuild      |  1 -
> > >   tools/testing/cxl/test/mock.c | 17 ---------
> > >   8 files changed, 78 insertions(+), 91 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > > index a7a0838c8f23..2b2d3af0b5ec 100644
> > > --- a/drivers/cxl/core/core.h
> > > +++ b/drivers/cxl/core/core.h
> > > @@ -232,4 +232,7 @@ static inline bool cxl_pci_drv_bound(struct pci_dev *pdev) { return false; };
> > >   static inline int cxl_pci_driver_init(void) { return 0; }
> > >   static inline void cxl_pci_driver_exit(void) { }
> > >   #endif
> > > +
> > > +resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
> > > +					   struct cxl_dport *dport);
> > >   #endif /* __CXL_CORE_H__ */
> > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > > index a66f7a84b5c8..566d57ba0579 100644
> > > --- a/drivers/cxl/core/pci.c
> > > +++ b/drivers/cxl/core/pci.c
> > > @@ -775,6 +775,68 @@ bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port)
> > >   }
> > >   EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_reset_detected, "CXL");
> > > +static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
> > > +				  struct cxl_register_map *map,
> > > +				  struct cxl_dport *dport)
> > > +{
> > > +	resource_size_t component_reg_phys;
> > > +
> > > +	*map = (struct cxl_register_map) {
> > > +		.host = &pdev->dev,
> > > +		.resource = CXL_RESOURCE_NONE,
> > > +	};
> > > +
> > > +	struct cxl_port *port __free(put_cxl_port) =
> > > +		cxl_pci_find_port(pdev, &dport);
> > > +	if (!port)
> > > +		return -EPROBE_DEFER;
> > > +
> > > +	component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
> > > +	if (component_reg_phys == CXL_RESOURCE_NONE)
> > > +		return -ENXIO;
> > > +
> > > +	map->resource = component_reg_phys;
> > > +	map->reg_type = CXL_REGLOC_RBI_COMPONENT;
> > > +	map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> > > +			      struct cxl_register_map *map)
> > > +{
> > > +	int rc;
> > > +
> > > +	rc = cxl_find_regblock(pdev, type, map);
> > > +
> > > +	/*
> > > +	 * If the Register Locator DVSEC does not exist, check if it
> > > +	 * is an RCH and try to extract the Component Registers from
> > > +	 * an RCRB.
> > > +	 */
> > > +	if (rc && type == CXL_REGLOC_RBI_COMPONENT && is_cxl_restricted(pdev)) {
> > > +		struct cxl_dport *dport;
> > > +		struct cxl_port *port __free(put_cxl_port) =
> > > +			cxl_pci_find_port(pdev, &dport);
> > > +		if (!port)
> > > +			return -EPROBE_DEFER;
> > > +
> > > +		rc = cxl_rcrb_get_comp_regs(pdev, map, dport);
> > > +		if (rc)
> > > +			return rc;
> > > +
> > > +		rc = cxl_dport_map_rcd_linkcap(pdev, dport);
> > > +		if (rc)
> > > +			return rc;
> > > +
> > > +	} else if (rc) {
> > > +		return rc;
> > > +	}
> > > +
> > > +	return cxl_setup_regs(map);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
> > > +
> > >   int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c)
> > >   {
> > >   	int speed, bw;
> > > diff --git a/drivers/cxl/core/pci_drv.c b/drivers/cxl/core/pci_drv.c
> > > index 18ed819d847d..a35e746e6303 100644
> > > --- a/drivers/cxl/core/pci_drv.c
> > > +++ b/drivers/cxl/core/pci_drv.c
> > > @@ -467,76 +467,6 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds, bool irq_avail)
> > >   	return 0;
> > >   }
> > > -/*
> > > - * Assume that any RCIEP that emits the CXL memory expander class code
> > > - * is an RCD
> > > - */
> > > -static bool is_cxl_restricted(struct pci_dev *pdev)
> > > -{
> > > -	return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END;
> > > -}
> > > -
> > > -static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
> > > -				  struct cxl_register_map *map,
> > > -				  struct cxl_dport *dport)
> > > -{
> > > -	resource_size_t component_reg_phys;
> > > -
> > > -	*map = (struct cxl_register_map) {
> > > -		.host = &pdev->dev,
> > > -		.resource = CXL_RESOURCE_NONE,
> > > -	};
> > > -
> > > -	struct cxl_port *port __free(put_cxl_port) =
> > > -		cxl_pci_find_port(pdev, &dport);
> > > -	if (!port)
> > > -		return -EPROBE_DEFER;
> > > -
> > > -	component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
> > > -	if (component_reg_phys == CXL_RESOURCE_NONE)
> > > -		return -ENXIO;
> > > -
> > > -	map->resource = component_reg_phys;
> > > -	map->reg_type = CXL_REGLOC_RBI_COMPONENT;
> > > -	map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > > -static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> > > -			      struct cxl_register_map *map)
> > > -{
> > > -	int rc;
> > > -
> > > -	rc = cxl_find_regblock(pdev, type, map);
> > > -
> > > -	/*
> > > -	 * If the Register Locator DVSEC does not exist, check if it
> > > -	 * is an RCH and try to extract the Component Registers from
> > > -	 * an RCRB.
> > > -	 */
> > > -	if (rc && type == CXL_REGLOC_RBI_COMPONENT && is_cxl_restricted(pdev)) {
> > > -		struct cxl_dport *dport;
> > > -		struct cxl_port *port __free(put_cxl_port) =
> > > -			cxl_pci_find_port(pdev, &dport);
> > > -		if (!port)
> > > -			return -EPROBE_DEFER;
> > > -
> > > -		rc = cxl_rcrb_get_comp_regs(pdev, map, dport);
> > > -		if (rc)
> > > -			return rc;
> > > -
> > > -		rc = cxl_dport_map_rcd_linkcap(pdev, dport);
> > > -		if (rc)
> > > -			return rc;
> > > -
> > > -	} else if (rc) {
> > > -		return rc;
> > > -	}
> > > -
> > > -	return cxl_setup_regs(map);
> > > -}
> > > -
> > >   static int cxl_pci_ras_unmask(struct pci_dev *pdev)
> > >   {
> > >   	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> > > index fb70ffbba72d..fc7fbd4f39d2 100644
> > > --- a/drivers/cxl/core/regs.c
> > > +++ b/drivers/cxl/core/regs.c
> > > @@ -641,4 +641,3 @@ resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
> > >   		return CXL_RESOURCE_NONE;
> > >   	return __rcrb_to_component(dev, &dport->rcrb, CXL_RCRB_UPSTREAM);
> > >   }
> > > -EXPORT_SYMBOL_NS_GPL(cxl_rcd_component_reg_phys, "CXL");
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index 1517250b0ec2..536c9d99e0e6 100644
> > > --- a/drivers/cxl/cxl.h
> > > +++ b/drivers/cxl/cxl.h
> > > @@ -222,8 +222,6 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
> > >   		      struct cxl_register_map *map);
> > >   int cxl_setup_regs(struct cxl_register_map *map);
> > >   struct cxl_dport;
> > > -resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
> > > -					   struct cxl_dport *dport);
> > >   int cxl_dport_map_rcd_linkcap(struct pci_dev *pdev, struct cxl_dport *dport);
> > >   #define CXL_RESOURCE_NONE ((resource_size_t) -1)
> > > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> > > index 3526e6d75f79..24aba9ff6d2e 100644
> > > --- a/drivers/cxl/cxlpci.h
> > > +++ b/drivers/cxl/cxlpci.h
> > > @@ -74,6 +74,17 @@ static inline bool cxl_pci_flit_256(struct pci_dev *pdev)
> > >   	return lnksta2 & PCI_EXP_LNKSTA2_FLIT;
> > >   }
> > > +/*
> > > + * Assume that the caller has already validated that @pdev has CXL
> > > + * capabilities, any RCiEP with CXL capabilities is treated as a
> > > + * Restricted CXL Device (RCD) and finds upstream port and endpoint
> > > + * registers in a Root Complex Register Block (RCRB).
> > > + */
> > > +static inline bool is_cxl_restricted(struct pci_dev *pdev)
> > > +{
> > > +	return pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END;
> > > +}
> > > +
> > >   int devm_cxl_port_enumerate_dports(struct cxl_port *port);
> > >   struct cxl_dev_state;
> > >   void read_cdat_data(struct cxl_port *port);
> > > @@ -89,4 +100,6 @@ static inline void cxl_uport_init_ras_reporting(struct cxl_port *port,
> > >   						struct device *host) { }
> > >   #endif
> > > +int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> > > +		       struct cxl_register_map *map);
> > >   #endif /* __CXL_PCI_H__ */
> > > diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> > > index d8b8272ef87b..d422c81cefa3 100644
> > > --- a/tools/testing/cxl/Kbuild
> > > +++ b/tools/testing/cxl/Kbuild
> > > @@ -7,7 +7,6 @@ ldflags-y += --wrap=nvdimm_bus_register
> > >   ldflags-y += --wrap=devm_cxl_port_enumerate_dports
> > >   ldflags-y += --wrap=cxl_await_media_ready
> > >   ldflags-y += --wrap=devm_cxl_add_rch_dport
> > > -ldflags-y += --wrap=cxl_rcd_component_reg_phys
> > >   ldflags-y += --wrap=cxl_endpoint_parse_cdat
> > >   ldflags-y += --wrap=cxl_dport_init_ras_reporting
> > >   ldflags-y += --wrap=devm_cxl_endpoint_decoders_setup
> > > diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> > > index 995269a75cbd..92fd5c69bef3 100644
> > > --- a/tools/testing/cxl/test/mock.c
> > > +++ b/tools/testing/cxl/test/mock.c
> > > @@ -226,23 +226,6 @@ struct cxl_dport *__wrap_devm_cxl_add_rch_dport(struct cxl_port *port,
> > >   }
> > >   EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_add_rch_dport, "CXL");
> > > -resource_size_t __wrap_cxl_rcd_component_reg_phys(struct device *dev,
> > > -						  struct cxl_dport *dport)
> > > -{
> > > -	int index;
> > > -	resource_size_t component_reg_phys;
> > > -	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> > > -
> > > -	if (ops && ops->is_mock_port(dev))
> > > -		component_reg_phys = CXL_RESOURCE_NONE;
> > > -	else
> > > -		component_reg_phys = cxl_rcd_component_reg_phys(dev, dport);
> > > -	put_cxl_mock_ops(index);
> > > -
> > > -	return component_reg_phys;
> > > -}
> > > -EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcd_component_reg_phys, "CXL");
> > > -
> > >   void __wrap_cxl_endpoint_parse_cdat(struct cxl_port *port)
> > >   {
> > >   	int index;
> > > -- 
> > > 2.34.1
> > > 

Powered by blists - more mailing lists