Re: [PATCH net-next v13 2/7] r8169: refactor RX path to prepare for multi-queue

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8169: refactor RX path to prepare for multi-queue

This patch moves the RX state out of struct rtl8169_private into a new
struct rtl8169_rx_ring array, switches the netdev allocation to
devm_alloc_etherdev_mqs() with up to 8 RX queues, and keeps the active
ring count at 1.

No Link: tags were present in the commit header.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index b362855c262f8..f80ad229ae5f8 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -74,9 +74,19 @@
>  #define NUM_TX_DESC	256	/* Number of Tx descriptor registers */
>  #define NUM_RX_DESC	256	/* Number of Rx descriptor registers */
>  #define R8169_TX_RING_BYTES	(NUM_TX_DESC * sizeof(struct TxDesc))
> -#define R8169_RX_RING_BYTES	(NUM_RX_DESC * sizeof(struct RxDesc))
> +
> +/*
> + * Workaround for the hardware DMA prefetcher. The H/W might aggressively
> + * fetch one more descriptor even after hitting the RingEnd mark. We
> + * allocate this extra dummy space as padding to prevent out-of-bounds
> + * access and potential IOMMU faults.
> + */
> +#define R8169_RX_RING_BYTES	((NUM_RX_DESC + 1) * sizeof(struct RxDesc))

[Severity: Medium]
The commit message says this patch "is a preparatory refactoring of the
RX path", but this macro change is a behavioural change on every chip the
driver supports.  Could it be split into its own patch and described in
the changelog?

The new comment asserts that the MAC "might aggressively fetch one more
descriptor even after hitting the RingEnd mark", causing "out-of-bounds
access and potential IOMMU faults".  If that is accurate, every released
kernel has that DMA hazard on the RX ring today.  Is there an erratum or
vendor reference for this, and should the change carry a Fixes: tag so it
can be picked up for stable?

There is also a size effect worth mentioning in the changelog.  struct
RxDesc is 16 bytes and NUM_RX_DESC is 256, so the request goes from
exactly 4096 bytes to 4112 bytes:

	256 * 16 = 4096	/* order-0 */
	257 * 16 = 4112	/* order-1 */

Since dma_alloc_coherent() is page granular, doesn't this turn each RX
descriptor ring allocation in rtl8169_alloc_rx_desc() into an order-1
allocation, i.e. 8 KiB of coherent memory per ring instead of 4 KiB (and
64 KiB instead of 32 KiB once the series scales to 8 rings)?

>  #define R8169_TX_STOP_THRS	(MAX_SKB_FRAGS + 1)
>  #define R8169_TX_START_THRS	(2 * R8169_TX_STOP_THRS)
> +#define R8169_MAX_RX_QUEUES	8
> +#define R8169_DEFAULT_RX_QUEUES	1
> +#define R8169_MAX_TX_QUEUES	1
>  
>  #define OCP_STD_PHY_BASE	0xa400
>  

[ ... ]

> @@ -4349,55 +4403,107 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)

[ ... ]

> +static int rtl8169_alloc_rx_desc(struct rtl8169_private *tp)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	struct rtl8169_rx_ring *ring;
> +
> +	for (int i = 0; i < tp->num_rx_rings; i++) {
> +		ring = &tp->rx_ring[i];
> +		ring->rx_desc_array = dma_alloc_coherent(&pdev->dev,
> +							 R8169_RX_RING_BYTES,
> +							 &ring->rx_phy_addr,
> +							 GFP_KERNEL);
> +		if (!ring->rx_desc_array)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}

[ ... ]

> @@ -6283,10 +6408,17 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		}
>  	}
>  
> +	tp->rx_ring = kcalloc(tp->num_rx_rings, sizeof(struct rtl8169_rx_ring),
> +			      GFP_KERNEL);
> +	if (!tp->rx_ring)
> +		return -ENOMEM;
> +
>  	tp->rtl8169_napi = kcalloc(tp->irq_nvecs, sizeof(struct napi_struct),
>  				   GFP_KERNEL);
> -	if (!tp->rtl8169_napi)
> +	if (!tp->rtl8169_napi) {
> +		kfree(tp->rx_ring);
>  		return -ENOMEM;
> +	}

[Severity: Medium]
Do these two bare returns leak the phylink instance created by
rtl_init_phylink()?

By this point in rtl_init_one() phylink_create() has already succeeded,
and it is not a devm allocation.  Every other failure path in the same
window destroys it, for example the r8169_mdio_register() failure just
above:

	rc = r8169_mdio_register(tp);
	if (rc) {
		phylink_destroy(tp->phylink);
		return rc;
	}

and the register_netdev() failure just below:

	rc = register_netdev(dev);
	if (rc) {
		phylink_destroy(tp->phylink);
		goto err_free_napi;
	}

rtl_remove_one() is never called for a failed probe, so when the
tp->rx_ring kcalloc() fails under memory pressure the phylink state
machine and its internal state have no owner.  The tp->rtl8169_napi
failure path that this hunk touches has the same shape.

Would a goto to a label that does phylink_destroy() (plus
kfree(tp->rx_ring)) be preferable to adding more bare returns here?

This is still present at the end of the series, so no later patch in the
set appears to address it.

>  
>  	r8169_init_napi(tp);
>  

[ ... ]

> @@ -6328,6 +6460,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  err_free_napi:
>  	r8169_free_napi(tp);
> +	kfree(tp->rx_ring);
>  	return rc;
>  }