Re: [PATCH v2 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing Hardware Capability Validation for Legacy ISA APIs
--

commit 28dadad6fede624ec82b707f4b97ba89645a0db7
Author: Roman Vivchar <rva333@protonmail.com>

soc: mediatek: cmdq-helper: add legacy GCE ISA support

Some SoCs, such as mt6572, have different WRITE and POLL instruction
encoding. Instead of 16-bit offset and 8-bit subsystem ID, old GCEs have
22-bit offset and 2-bit subsystem ID.

> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index f8ee6c9ade898..fb82f2a85a210 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c

[ ... ]

> @@ -86,7 +95,7 @@ int cmdq_dev_get_client_reg(struct device *dev,
>  		client_reg->subsys = CMDQ_SUBSYS_INVALID;
>  
>  		/*
> -		 * All GCEs support writing register PA with mask without subsys,
> +		 * GCE v2/v3 support writing register PA with mask without subsys,
>  		 * but this requires extra GCE instructions to convert the PA into
>  		 * a format that GCE can handle, which is less performance than
>  		 * directly using subsys. Therefore, when subsys is available,

[Severity: Medium]
When a client device on a legacy GCE is missing the mediatek,gce-client-reg
property, cmdq_dev_get_client_reg() defaults to assigning cmdq_pkt_write_pa
as the write function:

drivers/soc/mediatek/mtk-cmdq-helper.c:cmdq_dev_get_client_reg() {
    ...
		/*
		 * GCE v2/v3 support writing register PA with mask without subsys,
		 * but this requires extra GCE instructions to convert the PA into
		 * a format that GCE can handle, which is less performance than
		 * directly using subsys. Therefore, when subsys is available,
		 * we prefer to use subsys for writing register PA.
		 */
		client_reg->pkt_write = cmdq_pkt_write_pa;
    ...
}

However, cmdq_pkt_write_pa() unconditionally uses SPRs, which legacy GCE
hardware does not support:

drivers/soc/mediatek/mtk-cmdq-helper.c:cmdq_pkt_write_pa() {
    ...
	err = cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX0, CMDQ_ADDR_HIGH(pa_base));
    ...
}

Does this code need a check for pkt->priv.legacy_isa in cmdq_pkt_write_pa()
to prevent generating unsupported instructions that could cause silent queue
corruption or a GCE hardware fault?

[ ... ]

> @@ -459,10 +474,16 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
>  {
>  	struct cmdq_instruction inst = {
>  		.op = CMDQ_CODE_POLL,
> -		.value = value,
> -		.offset = offset,
> -		.subsys = subsys
> +		.value = value
>  	};
> +
> +	if (pkt->priv.legacy_isa) {
> +		inst.offset_legacy = offset;
> +		inst.subsys_legacy = subsys;
> +	} else {
> +		inst.offset = offset;
> +		inst.subsys = subsys;
> +	}
>  	return cmdq_pkt_append_command(pkt, inst);
>  }

[Severity: Medium]
Similarly, cmdq_pkt_poll_addr() unconditionally relies on GPR registers, which
legacy hardware does not implement:

drivers/soc/mediatek/mtk-cmdq-helper.c:cmdq_pkt_poll_addr() {
    ...
	inst.op = CMDQ_CODE_POLL;
	inst.dst_t = CMDQ_REG_TYPE;
	inst.sop = CMDQ_POLL_ADDR_GPR;
    ...
}

Should there be a capability validation for legacy_isa in
cmdq_pkt_poll_addr() as well to avoid generating invalid microcode
instructions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-6572-gce-v2-0-412a5b037290@protonmail.com?part=2