Re: [PATCH v4 2/7] net: wwan: t9xx: Add control plane transaction layer

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
--- Patch [2]: [PATCH v4 2/7] net: wwan: t9xx: Add control plane transaction layer ---
commit a06b668b7ebf78322cb2957d8a6a11fdfb9235ad
Author: Jack Wu <jackbb_wu@xxxxxxxxxx>

net: wwan: t9xx: Add control plane transaction layer
    
The control plane implements TX services that reside in the
transaction layer. The services receive the packets from the
port layer and call the corresponding DMA components to
transmit data to the device. Meanwhile, TX services receive
and manage the port control commands from the port layer.
    
The control plane implements RX services that reside in the
transaction layer. The services receive the downlink packets
from the modem and transfer the packets to the corresponding
port layer interfaces.

> diff --git a/drivers/net/wwan/t9xx/mtk_ctrl_plane.c b/drivers/net/wwan/t9xx/mtk_ctrl_plane.c
> new file mode 100644
> index 0000000000000..cf6079218e1ce
> --- /dev/null
> +++ b/drivers/net/wwan/t9xx/mtk_ctrl_plane.c
> @@ -0,0 +1,45 @@
[ ... ]
> +int mtk_ctrl_init(struct mtk_md_dev *mdev)
> +{
> +	struct mtk_ctrl_blk *ctrl_blk;
> +
> +	ctrl_blk = devm_kzalloc(mdev->dev, sizeof(*ctrl_blk), GFP_KERNEL);
> +	if (!ctrl_blk)
> +		return -ENOMEM;
> +
> +	ctrl_blk->mdev = mdev;
> +	mdev->ctrl_blk = ctrl_blk;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mtk_ctrl_init);

[Severity: Medium]
Does this patch actually implement the TX and RX services described in the
commit message?

The commit message claims to implement control plane TX and RX services, but
this code merely introduces empty initialization stubs (mtk_ctrl_init() and
mtk_ctrl_exit()) that are currently dead code.

> diff --git a/drivers/net/wwan/t9xx/pcie/mtk_pci.c b/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> index 72259bf1b6033..68dddd652effc 100644
> --- a/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> +++ b/drivers/net/wwan/t9xx/pcie/mtk_pci.c
> @@ -467,6 +468,7 @@ static u32 mtk_pci_ext_h2d_evt_hw_bits(u32 chs)
>  
>  	SET_HW_BITS(hw_bits, chs, MHCCIF_RC2EP_EVT_DEVICE_RESET,
>  		    DEV_EVT_H2D_DEVICE_RESET);
> +
>  	return LE32_TO_U32(cpu_to_le32(hw_bits));
>  }

[Severity: High]
Does this code apply a double byte-swap on big-endian architectures?

Using cpu_to_le32() converts the bitmask, which is then cast to u32 and
passed to mtk_pci_write32() (wrapping iowrite32()). Because iowrite32()
intrinsically expects a host-endian value and applies its own conversion for
little-endian PCI memory, the value is byte-swapped twice, resulting in the
original big-endian layout being incorrectly written.

Additionally, this can cause ffs(hw_bits) in mtk_pci_send_ext_evt() to
calculate the wrong bit index on the already byte-swapped value.