Re: [PATCH v2] cxl/region: Add cxl_decoder_is_passthrough() helper

On 05.08.26 14:59:34, Richard Cheng wrote:
> commit_decoder() open-codes the passthrough test.
> A NULL ->commit() plus a switch decoder with at most one target. The
> call site is hard to read, and it suggests a NULL ->commit() alone
> identifies a passthrough decoder. It does not.
> DVSEC-emulated endpoint decoders also leave ->commit() NULL, and root
> decoders never set it. A future caller testing ->commit() alone would
> silently include them.
> 
> Move the test into cxl_decoder_is_passthrough() and document what each
> condition rules out. The helper runs the same tests in the same order,
> no functional change.
> 
> Signed-off-by: Richard Cheng <[email protected]>
> ---
> Changelog:
> 
> v1 -> v2:
>     - Dropped the passthrough F_ENABLE restore patch. The bug was
>       already fixed.
>     - What remains is only the clarify helper function patch, so the
>       patch is retitled accordingly
> 
> v1:
> https://lore.kernel.org/linux-cxl/[email protected]/
> 
> Best regards,
> Richard Cheng.
> ---
>  drivers/cxl/core/region.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 27e63e6dab7c..cd73684fbe97 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -293,17 +293,33 @@ static void cxl_region_decode_reset(struct cxl_region *cxlr, int count)
>  	clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>  }
>  
> -static int commit_decoder(struct cxl_decoder *cxld)
> +/*
> + * A single-dport host-bridge need not publish an HDM decoder capability
> + * when passthrough decode can be assumed. The resulting decoder is a
> + * software-only construct with no registers to program, so it carries no
> + * ->commit() operation, see devm_cxl_add_passthrough_decoder().
> + *
> + * A NULL ->commit() alone does not identify one, it is also NULL for
> + * DVSEC-emulated endpoint decoders. Test the decoder type and target
> + * count as well.
> + */
> +static bool cxl_decoder_is_passthrough(struct cxl_decoder *cxld)

This is much noise for just checking if ->commit() is required in some
cases and generate an error if not.

>  {
> -	struct cxl_switch_decoder *cxlsd = NULL;
> +	if (cxld->commit)
> +		return false;

That duplicates the test and the code is never executed.

> +
> +	if (!is_switch_decoder(&cxld->dev))
> +		return false;
> +
> +	return to_cxl_switch_decoder(&cxld->dev)->nr_targets <= 1;

cxlsd->nr_targets is easier to read and explains that nr_targets is in
struct cxl_switch_decoder, which wouln't be obious else. So the change
does not improve readability and instead does multiple things in a
single line.

> +}
>  
> +static int commit_decoder(struct cxl_decoder *cxld)
> +{
>  	if (cxld->commit)
>  		return cxld->commit(cxld);
>  
> -	if (is_switch_decoder(&cxld->dev))
> -		cxlsd = to_cxl_switch_decoder(&cxld->dev);
> -
> -	if (dev_WARN_ONCE(&cxld->dev, !cxlsd || cxlsd->nr_targets > 1,
> +	if (dev_WARN_ONCE(&cxld->dev, !cxl_decoder_is_passthrough(cxld),
>  			  "->commit() is required\n"))

The original was pretty straight forward: if it is a switch decoder
with multiple targets, raise an error. It looks like an assert here as
!cxld->commit is only created in devm_cxl_add_passthrough_decoder()
which sets the number of targets to one (not looking into test code
here, but if that causes an issue, fix it there).

So I don't see much sense in this patch.

Thanks,

-Robert

>  		return -ENXIO;
>  	return 0;
> 
> base-commit: 212e015fc34712c849653cdb3179cd643c915015
> -- 
> 2.43.0
>