Re: [PATCH v1 02/29] cxl/pci: Moving code in cxl_hdm_decode_init()

[Robert Richter <rrichter@amd.com>]

On 07.01.25 11:18:39, Gregory Price wrote:
> On Tue, Jan 07, 2025 at 03:09:48PM +0100, Robert Richter wrote:
> > Commit 3f9e07531778 ("cxl/pci: simplify the check of mem_enabled in
> > cxl_hdm_decode_init()") changed the code flow in this function. The
> > root port is determined before a check to leave the function. Since
> > the root port is not used by the check it can be moved to run the
> > check first. This improves code readability and avoids unnesessary
> > code execution.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/cxl/core/pci.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 3e8d20f8955c..d206378c4cbc 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -419,14 +419,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >  	if (!hdm)
> >  		return -ENODEV;
> >  
> > -	root = to_cxl_port(port->dev.parent);
> > -	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
> > -		root = to_cxl_port(root->dev.parent);
> > -	if (!is_cxl_root(root)) {
> > -		dev_err(dev, "Failed to acquire root port for HDM enable\n");
> > -		return -ENODEV;
> > -	}
> > -
> 
> Can't say definitively, but my reading of the original ordering suggests
> the intent was to bail out of enabling anything if the cxl root cannot
> be found (which suggests much larger issues).
> 
> This code flow allows the device to have its bits twiddled when the root
> cannot be found - is that what we want?

A soon as a port is created, the cxl root should always exist and in
practice never fails. There is no other ways to allocate the
port. Variable 'root' is used later below in the code and that is the
reason to determine it here.

> 
> >  	if (!info->mem_enabled) {
> >  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> >  		if (rc)
> > @@ -435,6 +427,14 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >  		return devm_cxl_enable_mem(&port->dev, cxlds);
> >  	}

For the above reasons just enabling the memory without checking root
is safe.

-Robert

> >  
> > +	root = to_cxl_port(port->dev.parent);
> > +	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
> > +		root = to_cxl_port(root->dev.parent);
> > +	if (!is_cxl_root(root)) {
> > +		dev_err(dev, "Failed to acquire root port for HDM enable\n");
> > +		return -ENODEV;
> > +	}
> > +
> >  	for (i = 0, allowed = 0; i < info->ranges; i++) {
> >  		struct device *cxld_dev;
> >  
> > -- 
> > 2.39.5
> >