Re: [PATCH v9 2/6] net: wwan: t9xx: Add control plane transaction layer

["Wu. JackBB (GSM)" <JackBB_Wu@compal.com>]

Thanks for the review.

On Sun, 4 Oct 2026 17:12:00 +0800 netdev-bot+sashiko@kernel.org wrote:
> This isn't a bug, but at this point in the series nothing calls
> mtk_ctrl_init() or mtk_ctrl_exit(). That means:
[...]
> The subject says "Add control plane transaction layer", but this patch
> only adds structures and helpers that nothing can reach yet.
[...]
> Could these helpers and the include move into the patch that adds their
> callers? Otherwise, could the subject say that this patch only adds the
> scaffolding?

Will fix in v10, the second way: the subject becomes "net: wwan: t9xx:
Add control plane transaction layer framework", and the commit message
gains a line naming the patch where the callers arrive.

That lifts into the subject what the message body already says three
times - "Introduce the control plane and transaction layer framework",
"initialization entry points that subsequent patches build upon", "The
actual DMA engine and TX/RX service implementations are added in
subsequent patches".  It is not a new claim, only one that was invisible
unless you read past the subject line.

> This isn't a bug, but mtk_ctrl_init() allocates ctrl_blk with
> devm_kzalloc(), and mtk_ctrl_exit() only clears mdev->ctrl_blk. Suppose
> init and exit ran more than once while the device stayed bound. Each
> mtk_ctrl_init() call would add another devres allocation and overwrite
> mdev->ctrl_blk without freeing the old block.
[...]
> Would it help to add a comment saying mtk_ctrl_init() may only run once
> per bind? That would keep a future reset path from calling it again.

Will fix in v10 with the comment you suggest, on mtk_ctrl_init():

    /* May only run once per bind.  The ctrl_blk allocation is devres
     * managed and is released at unbind, not by mtk_ctrl_exit().
     */

> This isn't a bug, but nothing assigns ctrl_blk->trans in this patch.
[...]
> mtk_ctrl_plane.h is a generic header, but it names the pcie-only struct
> mtk_ctrl_trans. It does not include or forward-declare the header that
> defines it.
[...]
> Could the ctrl_hw_priv member be added here directly? That would avoid
> adding a trans member that is removed one patch later.

Will fix in v10, exactly that: struct mtk_ctrl_blk is introduced with
void *ctrl_hw_priv instead of struct mtk_ctrl_trans *trans.  The member
is then not renamed one patch later, and the generic header stops naming
a PCIe-only type, which it should not have done.


Thanks,
Jack