On 2023-12-14 13:31, Michael Chan wrote:
> From: Andy Gospodarek <andrew.gospodarek@xxxxxxxxxxxx>
>
> Remove double-mapping of DMA buffers as it can prevent page pool entries
> from being freed. Mapping is managed by page pool infrastructure and
> was previously managed by the driver in __bnxt_alloc_rx_page before
> allowing the page pool infrastructure to manage it.
>
> Fixes: 578fcfd26e2a ("bnxt_en: Let the page pool manage the DMA mapping")
> Reviewed-by: Somnath Kotur <somnath.kotur@xxxxxxxxxxxx>
> Signed-off-by: Andy Gospodarek <andrew.gospodarek@xxxxxxxxxxxx>
> Signed-off-by: Michael Chan <michael.chan@xxxxxxxxxxxx>
Reviewed-by: David Wei <dw@xxxxxxxxxxx>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index 96f5ca778c67..8cb9a99154aa 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> @@ -59,7 +59,6 @@ struct bnxt_sw_tx_bd *bnxt_xmit_bd(struct bnxt *bp,
> for (i = 0; i < num_frags ; i++) {
> skb_frag_t *frag = &sinfo->frags[i];
> struct bnxt_sw_tx_bd *frag_tx_buf;
> - struct pci_dev *pdev = bp->pdev;
> dma_addr_t frag_mapping;
> int frag_len;
>
> @@ -73,16 +72,10 @@ struct bnxt_sw_tx_bd *bnxt_xmit_bd(struct bnxt *bp,
> txbd = &txr->tx_desc_ring[TX_RING(prod)][TX_IDX(prod)];
>
> frag_len = skb_frag_size(frag);
> - frag_mapping = skb_frag_dma_map(&pdev->dev, frag, 0,
> - frag_len, DMA_TO_DEVICE);
I checked that skb_frag_dma_map() calls dma_map_page() with page set to
skb_frag_page(frag) and offset set to skb_frag_off(frag) + offset where
offset is 0. This is thus equivalent to the line added below:
page_pool_get_dma_addr(skb_frag_page(frag)) + skb_frag_off(frag)
> -
> - if (unlikely(dma_mapping_error(&pdev->dev, frag_mapping)))
> - return NULL;
I checked that page_pool_get_dma_addr() cannot fail or return an invalid
mapping. The DMA mapping happens when bulk allocating the pp alloc cache
during __page_pool_alloc_pages_slow(). If DMA mapping fails during
page_pool_dma_map() then the page is not stored in the cache. Therefore
any pages allocated from the pp will have a valid DMA addr.
> -
> - dma_unmap_addr_set(frag_tx_buf, mapping, frag_mapping);
As discussed with Michael Chan, only XDP_TX will have multiple page
frags. Presumably only XDP_TX will have num_frags > 0 and enter this for
loop. Even though XDP_REDIRECT also calls bnxt_xmit_bd() from
__bnxt_xmit_xdp_redirect(), I assume xdp_buff_has_frags() returns false.
> -
> flags = frag_len << TX_BD_LEN_SHIFT;
> txbd->tx_bd_len_flags_type = cpu_to_le32(flags);
> + frag_mapping = page_pool_get_dma_addr(skb_frag_page(frag)) +
> + skb_frag_off(frag);
I trust that the page pool DMA mapping management is correct.
Both skb_frag_dma_map() and page_pool_dma_map() call into
dma_map_page_attrs(), but page_pool_dma_map() has flags
DMA_ATTR_SKIP_CPU_SYNC and DMA_ATTR_WEAK_ORDERING set whereas
skb_frag_dma_map() has no flags.
DMA_ATTR_WEAK_ORDERING is optional and ignored for platforms that do not
support it, therefore safe to use.
DMA_ATTR_SKIP_CPU_SYNC is used since presumably there is no sharing of
pages between multiple devices. IIRC there is a single page pool per Rx
queue/NAPI context.
> txbd->tx_bd_haddr = cpu_to_le64(frag_mapping);
>
> len = frag_len;
