Re: [net PATCH 12/13] fbnic: Wait for the outbound flush to complete on mailbox teardown

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit makes the new flush-done poll in fbnic_mbx_clean()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c:204-209) wait for the…
- [Low] The commit message says fbnic_mbx_reset_desc_ring 'drops bus
  mastering and arms the outbound read and write flush in a single…

--

fbnic: Wait for the outbound flush to complete on mailbox teardown

This patch adds a poll on the PUL link reset status register at the end
of fbnic_mbx_clean(). The poll waits for the outbound read and write
flushes armed during mailbox teardown to report done before returning.
It has a 1ms timeout and logs a warning if the flush does not complete.

> The fbnic_mbx_reset_desc_ring function drops bus mastering and arms the
> outbound read and write flush in a single register write, but nothing
> waits for that flush to finish.

[Severity: Low]
Is this description accurate? In fbnic_mbx_reset_desc_ring(), two
separate writes to two different registers arm the write flush and the
read flush. mbx_idx selects which one runs:

    case FBNIC_IPC_MBX_RX_IDX:
        wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AW_CFG,
             FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH |
             FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH_MODE);
        break;
    case FBNIC_IPC_MBX_TX_IDX:
        wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG,
             FBNIC_PUL_OB_TLP_HDR_AR_CFG_FLUSH);
        break;

fbnic_mbx_clean() calls it once per ring, and the RX buffers are freed
in between. So the AW flush is armed before the AR flush, and no single
write arms both directions.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 09d03274be08c..32a18cb8990cb 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -187,10 +188,25 @@ static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
>  
>  void fbnic_mbx_clean(struct fbnic_dev *fbd)
>  {
> +	u32 flush_done = FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE |
> +			 FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE;
> +	u32 val;
>  	int i;
>  
>  	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
>  		fbnic_mbx_clean_desc_ring(fbd, i);
> +
> +	/* Arming the flush above only starts it. Wait for the outbound path
> +	 * to retire the requests it still owns so none of them can complete
> +	 * after the reset that follows. A lost link reads back as all ones,
> +	 * which ends the poll on the first pass.
> +	 */
> +	if (read_poll_timeout_atomic(fbnic_rd32, val,
> +				     (val & flush_done) == flush_done, 2, 1000,
> +				     false, fbd,
> +				     FBNIC_PUL_USER_IP_LINK_RST_STATUS))
> +		dev_warn(fbd->dev, "Outbound flush did not complete: %08x\n",
> +			 val);
>  }

[Severity: Medium]
Does this wait come too late to protect the TX mailbox pages?

By the time the poll runs, fbnic_mbx_clean_desc_ring() has already
unmapped and freed every buffer on both rings:

fbnic_mbx_clean()
  fbnic_mbx_clean_desc_ring(fbd, FBNIC_IPC_MBX_TX_IDX)
    fbnic_mbx_reset_desc_ring()
      wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG, ..._AR_CFG_FLUSH);
    fbnic_mbx_unmap_and_free_msg()
      dma_unmap_single(...);
      free_page(...);
  read_poll_timeout_atomic(...)

The new comment says arming the flush "only starts it" and that the
outbound path still owns requests at that point. If so, could a device
read of a TX page still be outstanding after dma_unmap_single() and
free_page() have run? With an IOMMU, it could also target an IOVA that
has already been unmapped or reused.

TX pages can still be on the ring at clean time if firmware has not set
FW_CMPL. One case is fbnic_mbx_flush_tx() timing out in
fbnic_fw_disable_mbx(). Another is the clean_mbx error path in
fbnic_mbx_poll_tx_ready() after the cap request has been posted.

The RX side sets FLUSH_MODE so parked writes do not land on the host.
The AR side has no equivalent before the pages are freed.

Would it be better to poll for the flush to complete in
fbnic_mbx_clean_desc_ring(), between fbnic_mbx_reset_desc_ring() and
the unmap/free loop?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179322311834.40667.5383039334126572422.stgit%40ahduyck-xeon-server.home.arpa