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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `legacy_isa` feature is completely non-functional because the driver fails to add the `mt6572` compatible string to `cmdq_of_ids` and `cmdq_get_mbox_priv()` is never updated to initialize the `legacy_isa` field. As a result, `pkt->priv.legacy_isa` always defaults to `false` (as packets are zero-allocated by callers), rendering the new legacy instruction support dead code.
- [High] The bitfield layout for `offset_legacy` and `subsys_legacy` inside the `__packed` struct relies on compiler-dependent little-endian bitfield ordering, which breaks on big-endian kernels.
- [High] `cmdq_pkt_poll_addr()` manually constructs a `CMDQ_CODE_POLL` instruction using standard hardware bitfields (like `sop` and `dst_t`), bypassing the `legacy_isa` encoding checks. This will emit corrupted instructions on MT6572.
--

--- Patch [2]: [PATCH 2/3] soc: mediatek: cmdq-helper: add legacy GCE ISA support ---
commit 79388ed6a8bb4f65925fd916d2fe04a8f2d226d6
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.
> 
> Extend cmdq_instruction struct with additional union and struct to hold
> the legacy format of instruction encoding. Add legacy_isa to select
> legacy GCE ISA when writing instruction. Finally, add special handing
> for legacy_isa cases.

> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index f8ee6c9ade898..0947324050805 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -31,18 +31,27 @@ struct cmdq_instruction {

> [ ... ]

> +		struct {
> +			u32 offset_legacy:22;
> +			u32 subsys_legacy:2;
> +		} __packed;

[Severity: High]
Could this bitfield layout break on big-endian kernels?

C bitfields are packed starting from the least significant bit on
little-endian architectures, but from the most significant bit on big-endian
architectures. If this is compiled on a big-endian kernel, the bit layout
will be inverted and the hardware might receive malformed instruction words.

Would it be better to use explicit bitwise shifts and masks, or an explicit
little-endian bitfield declaration for hardware-facing structures?

> [ ... ]
> @@ -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: High]
Does cmdq_pkt_poll_addr() also need an update for the new legacy format?

While cmdq_pkt_poll() checks for legacy_isa here, cmdq_pkt_poll_addr()
appears to still manually construct a poll instruction using standard hardware
bitfields like sop and dst_t.

If cmdq_pkt_poll_addr() is called on older hardware like mt6572, these bits
might be incorrectly reinterpreted by the hardware as random legacy offsets,
potentially causing hardware faults. Should cmdq_pkt_poll_addr() explicitly
return -EOPNOTSUPP if legacy_isa is active?

> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index 07c1bfbdb8c46..9fcebc4ca8645 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -72,6 +72,7 @@ struct cmdq_cb_data {
>  
>  struct cmdq_mbox_priv {
>  	u8 shift_pa;
> +	bool legacy_isa;

[Severity: High]
Is the legacy_isa flag ever initialized to true?

Looking at the driver, it appears the mt6572 compatible string is missing
from cmdq_of_ids, and cmdq_get_mbox_priv() is not updated to initialize
this field. 

Since packets are zero-allocated by callers, this flag will always default
to false, meaning the new legacy instruction branches will not be executed
and the feature remains non-functional.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-6572-gce-v1-0-750bba32689e@protonmail.com?part=2