Re: [net PATCH 5/5] 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 1 potential
issue(s) to consider.

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

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] If the new poll times out, fbnic_mbx_clean_desc_ring()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c) still frees the mailbox…

--

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

After fbnic_mbx_reset_desc_ring() arms the flush, this patch polls the AW
or AR flush-done bit in FBNIC_PUL_USER_IP_LINK_RST_STATUS. The poll runs
before the mailbox ring buffers are unmapped and freed, and it gives up
after 1ms and warns.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 49b8813a35643..e00d626f64384 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -176,10 +177,25 @@ static void fbnic_mbx_unmap_and_free_msg(struct fbnic_dev *fbd, int mbx_idx,
>  
>  static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
>  {
> +	u32 flush_done, val;
>  	int i;
>  
>  	fbnic_mbx_reset_desc_ring(fbd, mbx_idx);
>  
> +	/* Arming the flush only starts it. Wait for the outbound path to
> +	 * retire the requests it still holds before the buffers they may
> +	 * target are unmapped and freed. A lost link reads back as all ones,
> +	 * which ends the poll on the first pass.
> +	 */
> +	flush_done = mbx_idx == FBNIC_IPC_MBX_RX_IDX ?
> +		     FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE :
> +		     FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE;
> +	if (read_poll_timeout_atomic(fbnic_rd32, val, val & 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);
> +
>  	for (i = FBNIC_IPC_MBX_DESC_LEN; i--;)
>  		fbnic_mbx_unmap_and_free_msg(fbd, mbx_idx, i);
>  }

[Severity: Medium]
If read_poll_timeout_atomic() returns -ETIMEDOUT, does the original
DMA-after-free problem still happen?

On timeout the code only calls dev_warn(). It then falls through to the
same loop as before, and fbnic_mbx_unmap_and_free_msg() does this for
every descriptor:

    dma_unmap_single(fbd->dev, mbx->buf_info[desc_idx].addr,
                     PAGE_SIZE, direction);

    free_page((unsigned long)mbx->buf_info[desc_idx].msg);

As the comment says, a lost link reads back as all ones, so that case
exits on the first read. That leaves a timeout to mean a live device whose
outbound path has not drained, which is the wedged-device case named in
the commit message.

In that case the page can still be freed and reused, or its IOVA unmapped,
while a Tx read or an Rx write is still pending against it. Both paths
reach this branch:

  fbnic_fw_disable_mbx()->fbnic_mbx_clean()->fbnic_mbx_clean_desc_ring()

  fbnic_mbx_poll_tx_ready() clean_mbx error path->fbnic_mbx_clean()

The Rx ring has one more case. fbnic_mbx_init_desc_ring() later sets BME
and clears FLUSH and FLUSH_MODE, and nothing checks that the earlier AW
flush finished:

    wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AW_CFG,
         FBNIC_PUL_OB_TLP_HDR_AW_CFG_BME |
         ...

Could writes still parked in the pipeline then land on freed or reused DMA
addresses?

On timeout, would it be safer to skip the unmap and free and leak the
mailbox pages, instead of giving them back to the page allocator?

Also, the 1ms limit doesn't seem to be based on the PCIe completion
timeout, and that timeout is allowed to be longer. Is 1ms enough for
outstanding reads to complete?

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