RE: [PATCH 03/11] net: wwan: t9xx: Add control DMA interface

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

Hi Jagielski,

Thank you for the review. Below are the changes and responses for v2.

> > +int i, hif_id;
> > +struct trb *trb;
> > +u32 txqno;
>
> please stick to RCT

Reordered variable declarations to follow reverse Christmas tree
style.

> > +again:
> > +for (i = 0; i < txq->nr_gpds; i++) {
> > ...
> > +state = drv_ops->cldma_check_intr_status(drv_info, DIR_TX, txqno, QUEUE_XFER_DONE);
> > +if (state) {
> > ...
> > +goto again;
>
> are we sure we won't be locked here?

The loop is bounded: each iteration of the for loop processes at
most nr_gpds descriptors, and the goto again path only triggers
when a new XFER_DONE interrupt arrives while processing. Since the
TX ring has a fixed number of slots, forward progress is guaranteed
— once all completed descriptors are consumed, the for loop breaks
at the HWO check. cond_resched() prevents soft lockup. This pattern
is consistent with the RX work handler in the same file.

> > +err = mtk_cldma_check_rx_req(drv_info, rxq);
> > +if (!err)
> > +goto again;
>
> unclear for me
> repeat when 0 is returned
> do not repeat when -EAGAIN is returned by mtk_cldma_check_rx_req?

mtk_cldma_check_rx_req() returns 0 when there are more RX
descriptors ready for processing (HW current address differs from
the software free index and HWO bit is cleared), so the loop
continues. Non-zero means either no more data is available or an
error occurred:
- -EAGAIN: HW is still working on the current descriptor, or HWO
  bit didn't clear in time — no more data to process.
- -ENXIO: HW current address read back as 0, indicating a link
  error.

We agree the semantics could be clearer. Would you prefer we
rename/restructure this — for example, returning a bool (true =
more work) and handling errors separately, or using a different
error code instead of -EAGAIN? Open to suggestions on what would
be most intuitive here.

> so how EAGAIN is actually used here?

-EAGAIN is returned by mtk_cldma_submit_tx() when req_budget == 0
(TX descriptor ring full). In mtk_ctrl_trb_handler, this triggers
flow control: if packets were already batched (tx_burst_cnt > 0),
flush them; otherwise return immediately and leave the skb in the
queue for retry after TX completion frees budget.

We agree the semantics could be clearer. Could you suggest which
error code would be more appropriate for this case?

> > +static int mtk_cldma_rxq_free(struct cldma_drv_info *drv_info, u32 rxqno)
>
> please make it void

Changed to void return type.

> > +int ret = 0;
> > ...
> > +return ret;
>
> just return 0, no need to zeroinit ret

Removed zeroinit and return 0 directly in mtk_cldma_start_xfer().

> > +int mtk_cldma_exit(struct mtk_ctrl_trans *trans)
>
> void?

Changed to void return type.

> > +int err = 0;
>
> please be consistent within the series
> either you name 'ret'  either 'err'

Renamed all 'err' to 'ret' consistently throughout the patch.

> > +int err = 0;
>
> no need to zeroinit

Removed unnecessary zero-initialization in mtk_cldma_tx().

> > +if (unlikely(!drv_info)) {
> > +ret = -EINVAL;
> > +goto out;
> > +}
>
> why cannot return directly?

Changed to return directly instead of goto out in
mtk_cldma_submit_tx() error paths.

> > +if (unlikely(!drv_info)) {
>
> what's te benefit of using unlikely here?

Removed unlikely() from validation paths in
mtk_cldma_check_ch_cfg().

> > +u32 addr;
> > +u32 val;
> > +u32 sta;
>
> please squash

Squashed into a single declaration line.

Thanks.

Jack Wu


================================================================================================================================================================
This message may contain information which is private, privileged or confidential of Compal Electronics, Inc. If you are not the intended recipient of this message, please notify the sender and destroy/delete the message. Any review, retransmission, dissemination or other use of, or taking of any action in reliance upon this information, by persons or entities other than the intended recipient is prohibited.
================================================================================================================================================================