Re: [PATCH] cxl/hdm: Account for a firmware-armed Lock On Commit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] cxl/hdm: missing CXL_REGION_F_LOCK propagation in cxl_decoder_commit()
--

commit 38da830927e8d8e6e1d5d4154ac43502a9344546
Author: Davidlohr Bueso <dave@stgolabs.net>

cxl/hdm: Account for a firmware-armed Lock On Commit

This patch reads back the HDM decoder control register after committing a
decoder in cxl_decoder_commit() and sets CXL_DECODER_F_LOCK on cxld->flags if
CXL_HDM_DECODER0_CTRL_LOCK is set, so that an uncommitted decoder with Lock
On Commit armed by firmware is treated as locked once committed.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index fdcef20e01baa..d23076aabb89b 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -856,6 +856,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	int id = cxld->id, rc;
> +	u32 ctrl;
>  
>  	if (cxld->flags & CXL_DECODER_F_ENABLE)
>  		return 0;
> @@ -899,6 +900,14 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	port->commit_end++;
>  	cxld->flags |= CXL_DECODER_F_ENABLE;
>  
> +	/*
> +	 * Lock On Commit armed by firmware engages on this commit and
> +	 * makes the decoder read-only, like one found locked at probe.
> +	 */
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
> +		cxld->flags |= CXL_DECODER_F_LOCK;

[Severity: High]
Does setting CXL_DECODER_F_LOCK here in cxl_decoder_commit() leave the owning
cxl_region unlocked because CXL_REGION_F_LOCK is never propagated to
cxlr->flags?

For a decoder found committed and locked at probe time, init_hdm_decoder()
sets CXL_DECODER_F_LOCK before targets are attached. When targets are
subsequently attached via the attach_target() -> __attach_target() ->
cxl_region_attach() -> cxl_port_attach_region() call chain,
cxl_region_setup_flags() propagates the lock to the region:

drivers/cxl/core/region.c:cxl_region_setup_flags() {
	if (cxld->flags & CXL_DECODER_F_LOCK) {
		set_bit(CXL_REGION_F_LOCK, &cxlr->flags);
		clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
	}
}

However, when firmware arms Lock On Commit on an uncommitted decoder,
CXL_DECODER_F_LOCK is not set during cxl_port_attach_region(). It is only set
later when commit_store() -> __commit() -> cxl_region_decode_commit() ->
commit_decoder() -> cxl_decoder_commit() runs:

drivers/cxl/core/region.c:cxl_region_decode_commit() {
		/* commit bottom up */
		for (iter = cxled_to_port(cxled); !is_cxl_root(iter);
		     iter = to_cxl_port(iter->dev.parent)) {
			cxl_rr = cxl_rr_load(iter, cxlr);
			cxld = cxl_rr->decoder;
			rc = commit_decoder(cxld);
			if (rc)
				break;
		}
}

Because neither cxl_decoder_commit() nor cxl_region_decode_commit() updates
cxlr->flags after committing the decoder, CXL_REGION_F_LOCK remains clear on
the region.

Without CXL_REGION_F_LOCK set on cxlr->flags:

1. The region sysfs attribute locked_show() reports 0 even though the
   underlying decoder is locked.

2. Writing 0 to the region commit sysfs attribute in commit_store() bypasses
   the -EPERM check:

drivers/cxl/core/region.c:commit_store() {
	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
		return -EPERM;
}

3. When the region is reset or torn down via commit_store(),
   unregister_region(), or the error rollback path in
   cxl_region_decode_commit(), cxl_region_decode_reset() bypasses its early
   return:

drivers/cxl/core/region.c:cxl_region_decode_reset() {
	if (test_bit(CXL_REGION_F_LOCK, &cxlr->flags))
		return;
}

   As a result, cxl_region_decode_reset() resets any unlocked upstream switch,
   host-bridge, or peer endpoint decoders in the region (clearing their
   CXL_DECODER_F_ENABLE flag and allowing peer DPA to be freed) while
   cxl_decoder_reset() skips the locked decoder, leaving the hardware decode
   hierarchy partially torn down.

Should cxl_region_decode_commit() call cxl_region_setup_flags(cxlr, cxld) (or
propagate CXL_DECODER_F_LOCK to cxlr->flags) after commit_decoder() succeeds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028211836.939648-1-dave@stgolabs.net?part=1