Re: [PATCH net-next v10 4/7] r8169: enable new interrupt mapping

[Jakub Kicinski <kuba@kernel.org>]

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: enable new interrupt mapping

This patch enables new interrupt mapping for the RTL8127 device. It sets up
vector mapping if the device is configured with multiple MSI-X vectors.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 6a7e9d875cf00..31f6d0c2e3d15 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -3945,6 +3954,9 @@ static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
>  	RTL_W32(tp, RSS_CTRL_8125, 0);
>  	RTL_W16(tp, Q_NUM_CTRL_8125, 0);
>  
> +	if (tp->irq_nvecs > 1)
> +		rtl8169_hw_enable_vec_mapping(tp);
> +

[Severity: Medium]
With multiple MSI-X vectors enabled here, will concurrent NAPI polling cause
data races on the shared dev->stats counters?

When multiple NAPI instances process packets on different CPUs, rtl_rx() and
rtl8169_check_rx_desc_error() directly increment shared counters without
synchronization:

drivers/net/ethernet/realtek/r8169_main.c:rtl8169_check_rx_desc_error() {
    ...
    dev->stats.rx_length_errors++;
    if (status & RxCRC)
        dev->stats.rx_crc_errors++;
    ...
}

Could this lead to lost increments or torn reads on these network statistics?

[Severity: Medium]
Also, regarding the multiple RX rings setup for these vectors, is there a
memory leak in the teardown path?

The rx_ring array is allocated dynamically in rtl_init_one() during probe,
but rtl_remove_one() doesn't seem to free it:

drivers/net/ethernet/realtek/r8169_main.c:rtl_remove_one() {
    ...
    unregister_netdev(tp->dev);
    r8169_free_napi(tp);
    if (tp->dash_type != RTL_DASH_NONE)
        rtl8168_driver_stop(tp);
    rtl_release_firmware(tp);
    /* restore original MAC address */
    rtl_rar_set(tp, tp->dev->perm_addr);
}

Should there be a call to kfree(tp->rx_ring) here when the driver unbinds?

>  	/* disable UPS */
>  	r8168_mac_ocp_modify(tp, 0xd40a, 0x0010, 0x0000);
>