Re: [PATCH v2 02/13] cxl/hdm: Add function to restore one HDM decoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `cxl_decoder_recommit()` falsely reports success if the hardware is disconnected and the MMIO read returns `~0`.
--

--- Patch [2]: [PATCH v2 02/13] cxl/hdm: Add function to restore one HDM decoder ---
commit 15afab37836901527b4313972b91d202d46a3f32
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

cxl/hdm: Add function to restore one HDM decoder

This commit adds cxl_decoder_recommit() to restore a single HDM decoder's
settings if hardware no longer reports it as committed after a reset. The
function reprograms the registers and commits the changes, which is a step
toward restoring decoders below a reset port.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..d55d764895f88 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
[ ... ]
> @@ -849,6 +849,77 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	return 0;
>  }
>  
> +/**
> + * cxl_decoder_recommit - reprogram @cxld's HDM decoder registers and commit
[ ... ]
> +int cxl_decoder_recommit(struct cxl_decoder *cxld, u32 ctrl)
> +{
> +	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 hw_ctrl;
> +	int rc;
> +
> +	if ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)
> +		return 0;
> +
> +	if (!cxld->commit)
> +		return 0;
> +
> +	hw_ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, hw_ctrl)) {

[Severity: High]
If the hardware is disconnected or failed to recover from reset, readl() could
return ~0 (all 1s). In that case, FIELD_GET() will evaluate the COMMITTED bit
as true, causing the function to immediately return 0 (success) without
reprogramming the decoder or checking for COMMIT_ERROR.

Could this lead to falsely reporting successful decoder restoration during a
hardware failure, potentially routing traffic to a dead path?

> +		dev_dbg(&cxld->dev, "%s: still committed, no reprogram needed\n",
> +			__func__);
> +		return 0;
> +	}
> +
> +	writel(ctrl & ~(CXL_HDM_DECODER0_CTRL_COMMIT |
> +			CXL_HDM_DECODER0_CTRL_COMMITTED |
> +			CXL_HDM_DECODER0_CTRL_COMMIT_ERROR),
> +	       hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=2