* [PATCH net-next v13 1/7] r8169: add support for multi irqs
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
@ 2026-09-10 6:31 ` javen
2026-09-16 1:24 ` Jakub Kicinski
2026-09-10 6:31 ` [PATCH net-next v13 2/7] r8169: refactor RX path to prepare for multi-queue javen
` (5 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
RSS uses multi rx queues to receive packets, and each rx queue needs one
irq and napi. So this patch adds support for multi irqs and napi here.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
- remove some unused definitions, such as index, name in rtl8169_irq
- remove array imr and isr
- remove min_irq_nvecs and max_irq_nvecs, replaced with help function
get_min_irq_nvecs and get_max_irq_nvecs
- alloc irq by flags, instead of PCI_IRQ_ALL_TYPES
Changes in v3:
- add enum rtl_isr_version to replace macro definition
- remove struct rtl8169_napi, use napi_struct array instead and alloc
memory for this array dynamically
- remove struct rtl8169_irq
Changes in v4:
- change retval to ret in rtl8169_set_real_num_queue()
- reverse xmas tree in rtl8169_poll() and rtl8169_interrupt()
- remove tp->hw_supp_isr_ver
Changes in v5:
- rtl8169_request_irq(), when failed, only free irqs which are
allocated
- remove rss_support, simplied napi init, call r8169_init_napi()
directly
- remove rtl_isr_version, INTR_VEC_MAP_MASK, INTR_VEC_MAP_STATUS,
R8169_MAX_MSIX_VEC, rss_enable, recheck_desc_ownbit
- rtl_software_parameter_initialize() this function will be expanded in
next patch, so i want to remain it here.
Changes in v6:
- Fix netpoll crash
- Fix use-after-free during driver unload by registering a devm action
for netif_napi_del()
- remove tp->irq
Changes in v7:
- pass NAPI as arg to rtl_rx()
- use netif_set_real_num_queues to replace rtl8169_set_real_num_queues
- replace rtl_software_parameter_initialize with rtl_setup_rx_params
Changes in v8:
- no changes
Changes in v9:
- no changes
Changes in v10:
- no changes
Changes in v11:
- no changes
Changes in v12:
- no changes
Changes in v13:
- Fix sleep-in-atomic bug in netpoll by passing 0 to
rtl8169_interrupt()
- call netif_napi_set_irq() to properly associate NAPI with IRQ vector
- Fix print format for unsigned int tp->irq_nvecs from %d to %u
---
drivers/net/ethernet/realtek/r8169_main.c | 151 +++++++++++++++++-----
1 file changed, 122 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5415ff62a286..b362855c262f 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -761,7 +761,6 @@ struct rtl8169_private {
struct pci_dev *pci_dev;
struct net_device *dev;
struct phy_device *phydev;
- struct napi_struct napi;
enum mac_version mac_version;
enum rtl_dash_type dash_type;
enum rtl_sfp_mode sfp_mode;
@@ -774,10 +773,12 @@ struct rtl8169_private {
dma_addr_t RxPhyAddr;
struct page *Rx_databuff[NUM_RX_DESC]; /* Rx data buffers */
struct ring_info tx_skb[NUM_TX_DESC]; /* Tx data buffers */
+ struct napi_struct *rtl8169_napi;
+ unsigned int num_rx_rings;
u16 cp_cmd;
u16 tx_lpi_timer;
u32 irq_mask;
- int irq;
+ unsigned int irq_nvecs;
struct clk *clk;
int speed;
@@ -2769,6 +2770,11 @@ static void rtl_hw_reset(struct rtl8169_private *tp)
rtl_loop_wait_low(tp, &rtl_chipcmd_cond, 100, 100);
}
+static void rtl_setup_rx_params(struct rtl8169_private *tp)
+{
+ tp->num_rx_rings = 1;
+}
+
static void rtl_request_firmware(struct rtl8169_private *tp)
{
struct rtl_fw *rtl_fw;
@@ -4431,9 +4437,21 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
netdev_reset_queue(tp->dev);
}
+static void rtl8169_napi_disable(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->irq_nvecs; i++)
+ napi_disable(&tp->rtl8169_napi[i]);
+}
+
+static void rtl8169_napi_enable(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->irq_nvecs; i++)
+ napi_enable(&tp->rtl8169_napi[i]);
+}
+
static void rtl8169_cleanup(struct rtl8169_private *tp)
{
- napi_disable(&tp->napi);
+ rtl8169_napi_disable(tp);
/* Give a racing hard_start_xmit a few cycles to complete. */
synchronize_net();
@@ -4479,7 +4497,7 @@ static void rtl_reset_work(struct rtl8169_private *tp)
for (i = 0; i < NUM_RX_DESC; i++)
rtl8169_mark_to_asic(tp->RxDescArray + i);
- napi_enable(&tp->napi);
+ rtl8169_napi_enable(tp);
rtl_hw_start(tp);
}
@@ -4933,7 +4951,8 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
skb_checksum_none_assert(skb);
}
-static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget)
+static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
+ int budget, struct napi_struct *napi)
{
struct device *d = tp_to_dev(tp);
int count;
@@ -4985,7 +5004,7 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
goto release_descriptor;
}
- skb = napi_alloc_skb(&tp->napi, pkt_size);
+ skb = napi_alloc_skb(napi, pkt_size);
if (unlikely(!skb)) {
dev->stats.rx_dropped++;
goto release_descriptor;
@@ -5009,7 +5028,7 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
if (skb->pkt_type == PACKET_MULTICAST)
dev->stats.multicast++;
- napi_gro_receive(&tp->napi, skb);
+ napi_gro_receive(napi, skb);
dev_sw_netstats_rx_add(dev, pkt_size);
release_descriptor:
@@ -5021,8 +5040,12 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
{
- struct rtl8169_private *tp = dev_instance;
- u32 status = rtl_get_events(tp);
+ struct napi_struct *napi = dev_instance;
+ struct rtl8169_private *tp;
+ u32 status;
+
+ tp = netdev_priv(napi->dev);
+ status = rtl_get_events(tp);
if ((status & 0xffff) == 0xffff || !(status & tp->irq_mask))
return IRQ_NONE;
@@ -5043,13 +5066,43 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
}
rtl_irq_disable(tp);
- napi_schedule(&tp->napi);
+ napi_schedule(napi);
out:
rtl_ack_events(tp, status);
return IRQ_HANDLED;
}
+static void rtl8169_free_irq(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->irq_nvecs; i++) {
+ struct napi_struct *napi = &tp->rtl8169_napi[i];
+
+ pci_free_irq(tp->pci_dev, i, napi);
+ }
+}
+
+static int rtl8169_request_irq(struct rtl8169_private *tp)
+{
+ struct net_device *dev = tp->dev;
+ struct napi_struct *napi;
+ int i, rc;
+
+ for (i = 0; i < tp->irq_nvecs; i++) {
+ napi = &tp->rtl8169_napi[i];
+ rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
+ NULL, napi, "%s-%d", dev->name, i);
+ if (rc)
+ goto free_irq;
+ }
+ return 0;
+
+free_irq:
+ while (--i >= 0)
+ pci_free_irq(tp->pci_dev, i, &tp->rtl8169_napi[i]);
+ return rc;
+}
+
static void rtl_task(struct work_struct *work)
{
struct rtl8169_private *tp =
@@ -5084,13 +5137,13 @@ static void rtl_task(struct work_struct *work)
static int rtl8169_poll(struct napi_struct *napi, int budget)
{
- struct rtl8169_private *tp = container_of(napi, struct rtl8169_private, napi);
- struct net_device *dev = tp->dev;
- int work_done;
+ struct rtl8169_private *tp = netdev_priv(napi->dev);
+ struct net_device *dev = napi->dev;
+ int work_done = 0;
rtl_tx(dev, tp, budget);
- work_done = rtl_rx(dev, tp, budget);
+ work_done = rtl_rx(dev, tp, budget, napi);
if (work_done < budget && napi_complete_done(napi, work_done))
rtl_irq_enable(tp);
@@ -5175,7 +5228,7 @@ static void rtl8169_up(struct rtl8169_private *tp)
if (tp->phydev)
rtl8169_init_phy(tp);
- napi_enable(&tp->napi);
+ rtl8169_napi_enable(tp);
enable_work(&tp->wk.work);
rtl_reset_work(tp);
}
@@ -5192,7 +5245,7 @@ static int rtl8169_close(struct net_device *dev)
rtl8169_down(tp);
rtl8169_rx_clear(tp);
- free_irq(tp->irq, tp);
+ rtl8169_free_irq(tp);
phylink_disconnect_phy(tp->phylink);
@@ -5213,7 +5266,8 @@ static void rtl8169_netpoll(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
- rtl8169_interrupt(tp->irq, tp);
+ for (int i = 0; i < tp->irq_nvecs; i++)
+ rtl8169_interrupt(0, &tp->rtl8169_napi[i]);
}
#endif
@@ -5221,7 +5275,6 @@ static int rtl_open(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
struct pci_dev *pdev = tp->pci_dev;
- unsigned long irqflags;
int retval = -ENOMEM;
pm_runtime_get_sync(&pdev->dev);
@@ -5246,8 +5299,7 @@ static int rtl_open(struct net_device *dev)
rtl_request_firmware(tp);
- irqflags = pci_dev_msi_enabled(pdev) ? IRQF_NO_THREAD : IRQF_SHARED;
- retval = request_irq(tp->irq, rtl8169_interrupt, irqflags, dev->name, tp);
+ retval = rtl8169_request_irq(tp);
if (retval < 0)
goto err_release_fw_2;
@@ -5266,7 +5318,7 @@ static int rtl_open(struct net_device *dev)
return retval;
err_free_irq:
- free_irq(tp->irq, tp);
+ rtl8169_free_irq(tp);
err_release_fw_2:
rtl_release_firmware(tp);
rtl8169_rx_clear(tp);
@@ -5419,6 +5471,14 @@ static void rtl_shutdown(struct pci_dev *pdev)
pci_prepare_to_sleep(pdev);
}
+static void r8169_free_napi(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->irq_nvecs; i++)
+ netif_napi_del(&tp->rtl8169_napi[i]);
+
+ kfree(tp->rtl8169_napi);
+}
+
static void rtl_remove_one(struct pci_dev *pdev)
{
struct rtl8169_private *tp = pci_get_drvdata(pdev);
@@ -5432,6 +5492,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
r8169_remove_leds(tp->leds);
unregister_netdev(tp->dev);
+ r8169_free_napi(tp);
phylink_destroy(tp->phylink);
if (tp->dash_type != RTL_DASH_NONE)
@@ -5473,7 +5534,9 @@ static void rtl_set_irq_mask(struct rtl8169_private *tp)
static int rtl_alloc_irq(struct rtl8169_private *tp)
{
+ struct pci_dev *pdev = tp->pci_dev;
unsigned int flags;
+ int nvecs;
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_02 ... RTL_GIGA_MAC_VER_06:
@@ -5489,7 +5552,14 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
break;
}
- return pci_alloc_irq_vectors(tp->pci_dev, 1, 1, flags);
+ nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
+
+ if (nvecs < 0)
+ return nvecs;
+
+ tp->irq_nvecs = nvecs;
+
+ return 0;
}
static void rtl_read_mac_address(struct rtl8169_private *tp,
@@ -6005,6 +6075,15 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
return 0;
}
+static void r8169_init_napi(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->irq_nvecs; i++) {
+ netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
+ netif_napi_set_irq(&tp->rtl8169_napi[i],
+ pci_irq_vector(tp->pci_dev, i));
+ }
+}
+
static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_chip_info *chip;
@@ -6105,12 +6184,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
rtl_hw_reset(tp);
+ rtl_setup_rx_params(tp);
+
rc = rtl_alloc_irq(tp);
if (rc < 0)
return dev_err_probe(&pdev->dev, rc, "Can't allocate interrupt\n");
- tp->irq = pci_irq_vector(pdev, 0);
-
INIT_WORK(&tp->wk.work, rtl_task);
disable_work(&tp->wk.work);
@@ -6118,8 +6197,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
dev->ethtool_ops = &rtl8169_ethtool_ops;
- netif_napi_add(dev, &tp->napi, rtl8169_poll);
-
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM |
NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
@@ -6180,6 +6257,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (jumbo_max)
dev->max_mtu = jumbo_max;
+ rc = netif_set_real_num_queues(tp->dev, 1, tp->num_rx_rings);
+ if (rc < 0)
+ return dev_err_probe(&pdev->dev, rc, "set tx/rx num failure\n");
+
rtl_set_irq_mask(tp);
tp->counters = dmam_alloc_coherent (&pdev->dev, sizeof(*tp->counters),
@@ -6202,10 +6283,17 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
}
}
+ tp->rtl8169_napi = kcalloc(tp->irq_nvecs, sizeof(struct napi_struct),
+ GFP_KERNEL);
+ if (!tp->rtl8169_napi)
+ return -ENOMEM;
+
+ r8169_init_napi(tp);
+
rc = register_netdev(dev);
if (rc) {
phylink_destroy(tp->phylink);
- return rc;
+ goto err_free_napi;
}
if (IS_ENABLED(CONFIG_R8169_LEDS)) {
@@ -6215,8 +6303,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
tp->leds = rtl8168_init_leds(dev);
}
- netdev_info(dev, "%s, %pM, %sXID %x, IRQ %d\n",
- chip->name, dev->dev_addr, ext_xid_str, xid, tp->irq);
+ netdev_info(dev, "%s, %pM, %sXID %x, IRQ %d (%u total)\n",
+ chip->name, dev->dev_addr, ext_xid_str, xid,
+ pci_irq_vector(pdev, 0), tp->irq_nvecs);
if (jumbo_max)
netdev_info(dev, "jumbo features [frames: %d bytes, tx checksumming: %s]\n",
@@ -6236,6 +6325,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
pm_runtime_put_sync(&pdev->dev);
return 0;
+
+err_free_napi:
+ r8169_free_napi(tp);
+ return rc;
}
static struct pci_driver rtl8169_pci_driver = {
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v13 1/7] r8169: add support for multi irqs
2026-09-10 6:31 ` [PATCH net-next v13 1/7] r8169: add support for multi irqs javen
@ 2026-09-16 1:24 ` Jakub Kicinski
0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2026-09-16 1:24 UTC (permalink / raw)
To: javen_xu
Cc: Jakub Kicinski, hkallweit1, nic_swsd, andrew+netdev, davem,
edumazet, pabeni, horms, netdev, linux-kernel
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: add support for multi irqs
This converts the single struct napi_struct in struct rtl8169_private into a
dynamically allocated array indexed by IRQ vector, replaces the open-coded
request_irq()/free_irq() calls with per-vector pci_request_irq()/pci_free_irq()
helpers, and passes the NAPI instance through rtl_rx() and the interrupt
handler.
A couple of questions below.
> RSS uses multi rx queues to receive packets, and each rx queue needs one
> irq and napi. So this patch adds support for multi irqs and napi here.
[Severity: Low]
This isn't a bug, but does the changelog match what the patch does?
rtl_alloc_irq() still asks for exactly one vector:
nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
and the new rtl_setup_rx_params() hard-codes a single ring:
static void rtl_setup_rx_params(struct rtl8169_private *tp)
{
tp->num_rx_rings = 1;
}
So tp->irq_nvecs and tp->num_rx_rings are both always 1 here, every new
loop over irq_nvecs runs once, and netif_set_real_num_queues(tp->dev, 1, 1)
has no effect until the later patches in the series raise the vector and
queue counts. Would something like "prepare for multiple IRQs and NAPI
instances, no functional change" describe this patch more accurately?
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 5415ff62a2869..b362855c262f8 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
[ ... ]
> @@ -5043,13 +5066,43 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> }
>
> rtl_irq_disable(tp);
> - napi_schedule(&tp->napi);
> + napi_schedule(napi);
> out:
> rtl_ack_events(tp, status);
>
> return IRQ_HANDLED;
> }
>
> +static void rtl8169_free_irq(struct rtl8169_private *tp)
> +{
> + for (int i = 0; i < tp->irq_nvecs; i++) {
> + struct napi_struct *napi = &tp->rtl8169_napi[i];
> +
> + pci_free_irq(tp->pci_dev, i, napi);
> + }
> +}
> +
> +static int rtl8169_request_irq(struct rtl8169_private *tp)
> +{
> + struct net_device *dev = tp->dev;
> + struct napi_struct *napi;
> + int i, rc;
> +
> + for (i = 0; i < tp->irq_nvecs; i++) {
> + napi = &tp->rtl8169_napi[i];
> + rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
> + NULL, napi, "%s-%d", dev->name, i);
> + if (rc)
> + goto free_irq;
> + }
> + return 0;
[Severity: Medium]
Does switching to pci_request_irq() here drop IRQF_NO_THREAD for MSI and
MSI-X?
pci_request_irq() in drivers/pci/irq.c has no flags argument:
unsigned long irqflags = IRQF_SHARED;
if (!handler)
irqflags |= IRQF_ONESHOT;
Since rtl8169_interrupt is passed as the primary handler and thread_fn is
NULL, every vector ends up registered with plain IRQF_SHARED. On kernels
booted with threadirqs, and on PREEMPT_RT where force_irqthreads() is
always true, irq_setup_forced_threading() in kernel/irq/manage.c then does:
if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
return 0;
...
new->flags |= IRQF_ONESHOT;
so rtl8169_interrupt() is moved into ->thread_fn behind
irq_default_primary_handler() and the vector stays masked for the duration
of the handler.
That looks like it reverts commit 870f531e178e ("r8169: set IRQF_NO_THREAD
if MSI(X) is enabled"), which set IRQF_NO_THREAD precisely because MSI and
MSI-X are never shared. Was this behaviour change intended? If not,
request_threaded_irq() with per-vector flags would keep the previous
semantics.
> +
> +free_irq:
> + while (--i >= 0)
> + pci_free_irq(tp->pci_dev, i, &tp->rtl8169_napi[i]);
> + return rc;
> +}
> +
[ ... ]
> @@ -5221,7 +5275,6 @@ static int rtl_open(struct net_device *dev)
> {
> struct rtl8169_private *tp = netdev_priv(dev);
> struct pci_dev *pdev = tp->pci_dev;
> - unsigned long irqflags;
> int retval = -ENOMEM;
>
> pm_runtime_get_sync(&pdev->dev);
> @@ -5246,8 +5299,7 @@ static int rtl_open(struct net_device *dev)
>
> rtl_request_firmware(tp);
>
> - irqflags = pci_dev_msi_enabled(pdev) ? IRQF_NO_THREAD : IRQF_SHARED;
> - retval = request_irq(tp->irq, rtl8169_interrupt, irqflags, dev->name, tp);
> + retval = rtl8169_request_irq(tp);
> if (retval < 0)
> goto err_release_fw_2;
>
[ ... ]
> @@ -6202,10 +6283,17 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
> }
>
> + tp->rtl8169_napi = kcalloc(tp->irq_nvecs, sizeof(struct napi_struct),
> + GFP_KERNEL);
> + if (!tp->rtl8169_napi)
> + return -ENOMEM;
[Severity: Medium]
Does this new error exit leak the phylink instance?
rtl_init_phylink() stores an unmanaged phylink_create() result in
tp->phylink, and the neighbouring failure paths in rtl_init_one() all
release it explicitly, for example the r8169_mdio_register() failure:
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;
}
The netdev itself is devm-allocated, so devres unwinding on probe failure
frees the netdev but nothing frees the phylink object. Should this branch
use the same unwind, e.g. rc = -ENOMEM followed by a goto to a label that
calls phylink_destroy()?
This is still the case at the end of the series, where the same branch only
gains a kfree(tp->rx_ring).
> +
> + r8169_init_napi(tp);
> +
> rc = register_netdev(dev);
> if (rc) {
> phylink_destroy(tp->phylink);
> - return rc;
> + goto err_free_napi;
> }
>
[ ... ]
--
pw-bot: cr
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v13 2/7] r8169: refactor RX path to prepare for multi-queue
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
2026-09-10 6:31 ` [PATCH net-next v13 1/7] r8169: add support for multi irqs javen
@ 2026-09-10 6:31 ` javen
2026-09-16 1:24 ` Jakub Kicinski
2026-09-10 6:31 ` [PATCH net-next v13 3/7] r8169: add support for new interrupt mapping javen
` (4 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
This patch is a preparatory refactoring of the RX path. It introduces
struct rtl8169_rx_ring and turns the previously embedded RX state in
rtl8169_private into a per-queue array.
While the netdev allocation is changed to devm_alloc_etherdev_mqs()
with up to 8 RX queues, the actual number of active RX rings
(num_rx_rings) is currently kept at 1. The actual multi-queue operation
and RSS enablement will be introduced in subsequent patches.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
- sort some registers by its number
- remove some unused definitions, like RX_DESC_RING_TYPE_MAX
- change recheck_desc_ownbit type
- remove rdsar_reg in rx_ring struct
- opts1 are different in rx_desc and rx_desc_rss, move the judgement
to Patch 5/7
Changes in v3:
- remove ring->rx_desc_alloc_size, use constant instead
Changes in v4:
- change rdsar_reg type to unsigned int
- follow reverse xmas tree, in rtl_set_rx_tx_desc_registers(),
rtl8169_alloc_rx_data(), rtl8169_alloc_rx_desc(),
rtl8169_free_rx_desc()
- add comments on LED_CTRL, remove helper function
Changes in v5:
- modify rtl8169_init_ring(), do rx clear when failed
- add definition R8169_MAX_TX_QUEUES 1
Changes in v6:
- Restore the secondary Rx error filter when NETIF_F_RXFALL is enabled
in rtl_rx()
Changes in v7:
- remove code associated with recheck_desc_ownbit
Changes in v8:
- remove le64_to_cpu() for addr, rx get addr from rx_desc_phy_addr
Changes in v9:
- remove R8127_MAX_RX_QUEUES
- remvoe rx_desc_ring_type to the following patch
- Fix loop bound in init_ring_indexes
- Restore checksum API
Changes in v10:
- alloc rtl8169_rx_ring struct according to the num_rx_ring dynamically
Changes in v11:
- leak rx_ring array on driver removal
Changes in v12:
- no changes
Changes in v13:
- remove unused dirty_rx
---
drivers/net/ethernet/realtek/r8169_main.c | 243 +++++++++++++++++-----
1 file changed, 188 insertions(+), 55 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index b362855c262f..f80ad229ae5f 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))
#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
@@ -463,6 +473,7 @@ enum rtl8125_registers {
TxPoll_8125 = 0x90,
LEDSEL3 = 0x96,
MAC0_BKP = 0x19e0,
+ RDSAR_Q1_LOW = 0x4000,
RSS_CTRL_8125 = 0x4500,
Q_NUM_CTRL_8125 = 0x4800,
EEE_TXIDLE_TIMER_8125 = 0x6048,
@@ -750,6 +761,14 @@ enum rtl_dash_type {
RTL_DASH_25_BP,
};
+struct rtl8169_rx_ring {
+ u32 cur_rx;
+ struct RxDesc *rx_desc_array;
+ dma_addr_t rx_desc_phy_addr[NUM_RX_DESC];
+ dma_addr_t rx_phy_addr;
+ struct page *rx_databuff[NUM_RX_DESC];
+};
+
enum rtl_sfp_mode {
RTL_SFP_NONE,
RTL_SFP_8168_AF,
@@ -764,20 +783,18 @@ struct rtl8169_private {
enum mac_version mac_version;
enum rtl_dash_type dash_type;
enum rtl_sfp_mode sfp_mode;
- u32 cur_rx; /* Index into the Rx descriptor buffer of next Rx pkt. */
u32 cur_tx; /* Index into the Tx descriptor buffer of next Rx pkt. */
u32 dirty_tx;
struct TxDesc *TxDescArray; /* 256-aligned Tx descriptor ring */
- struct RxDesc *RxDescArray; /* 256-aligned Rx descriptor ring */
dma_addr_t TxPhyAddr;
- dma_addr_t RxPhyAddr;
- struct page *Rx_databuff[NUM_RX_DESC]; /* Rx data buffers */
struct ring_info tx_skb[NUM_TX_DESC]; /* Tx data buffers */
struct napi_struct *rtl8169_napi;
+ struct rtl8169_rx_ring *rx_ring;
unsigned int num_rx_rings;
u16 cp_cmd;
u16 tx_lpi_timer;
u32 irq_mask;
+ unsigned int hw_supp_num_rx_queues;
unsigned int irq_nvecs;
struct clk *clk;
int speed;
@@ -2718,9 +2735,25 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
}
}
+static void rtl8169_rx_desc_init(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->num_rx_rings; i++) {
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
+
+ memset(ring->rx_desc_array, 0x0, R8169_RX_RING_BYTES);
+ }
+}
+
static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)
{
- tp->dirty_tx = tp->cur_tx = tp->cur_rx = 0;
+ tp->dirty_tx = 0;
+ tp->cur_tx = 0;
+
+ for (int i = 0; i < tp->num_rx_rings; i++) {
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
+
+ ring->cur_rx = 0;
+ }
}
static void rtl_jumbo_config(struct rtl8169_private *tp)
@@ -2773,6 +2806,14 @@ static void rtl_hw_reset(struct rtl8169_private *tp)
static void rtl_setup_rx_params(struct rtl8169_private *tp)
{
tp->num_rx_rings = 1;
+ switch (tp->mac_version) {
+ case RTL_GIGA_MAC_VER_80:
+ tp->hw_supp_num_rx_queues = R8169_MAX_RX_QUEUES;
+ break;
+ default:
+ tp->hw_supp_num_rx_queues = R8169_DEFAULT_RX_QUEUES;
+ break;
+ }
}
static void rtl_request_firmware(struct rtl8169_private *tp)
@@ -2899,6 +2940,8 @@ static void rtl_set_rx_max_size(struct rtl8169_private *tp)
static void rtl_set_rx_tx_desc_registers(struct rtl8169_private *tp)
{
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[0];
+
/*
* Magic spell: some iop3xx ARM board needs the TxDescAddrHigh
* register to be written before TxDescAddrLow to work.
@@ -2906,8 +2949,18 @@ static void rtl_set_rx_tx_desc_registers(struct rtl8169_private *tp)
*/
RTL_W32(tp, TxDescStartAddrHigh, ((u64) tp->TxPhyAddr) >> 32);
RTL_W32(tp, TxDescStartAddrLow, ((u64) tp->TxPhyAddr) & DMA_BIT_MASK(32));
- RTL_W32(tp, RxDescAddrHigh, ((u64) tp->RxPhyAddr) >> 32);
- RTL_W32(tp, RxDescAddrLow, ((u64) tp->RxPhyAddr) & DMA_BIT_MASK(32));
+ RTL_W32(tp, RxDescAddrHigh, ((u64)ring->rx_phy_addr) >> 32);
+ RTL_W32(tp, RxDescAddrLow,
+ ((u64)ring->rx_phy_addr) & DMA_BIT_MASK(32));
+
+ for (int i = 1; i < tp->num_rx_rings; i++) {
+ unsigned int rdsar_reg = RDSAR_Q1_LOW + (i - 1) * 8;
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
+
+ RTL_W32(tp, rdsar_reg + 4, ((u64)ring->rx_phy_addr >> 32));
+ RTL_W32(tp, rdsar_reg,
+ ((u64)ring->rx_phy_addr) & DMA_BIT_MASK(32));
+ }
}
static void rtl8169_set_magic_reg(struct rtl8169_private *tp)
@@ -4330,8 +4383,9 @@ static void rtl8169_mark_to_asic(struct RxDesc *desc)
}
static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
- struct RxDesc *desc)
+ struct rtl8169_rx_ring *ring, unsigned int index)
{
+ struct RxDesc *desc = ring->rx_desc_array + index;
struct device *d = tp_to_dev(tp);
int node = dev_to_node(d);
dma_addr_t mapping;
@@ -4349,55 +4403,107 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
}
desc->addr = cpu_to_le64(mapping);
+ ring->rx_desc_phy_addr[index] = mapping;
rtl8169_mark_to_asic(desc);
return data;
}
-static void rtl8169_rx_clear(struct rtl8169_private *tp)
+static void rtl8169_rx_clear(struct rtl8169_private *tp,
+ struct rtl8169_rx_ring *ring)
{
int i;
- for (i = 0; i < NUM_RX_DESC && tp->Rx_databuff[i]; i++) {
+ for (i = 0; i < NUM_RX_DESC && ring->rx_databuff[i]; i++) {
dma_unmap_page(tp_to_dev(tp),
- le64_to_cpu(tp->RxDescArray[i].addr),
+ ring->rx_desc_phy_addr[i],
R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
- __free_pages(tp->Rx_databuff[i], get_order(R8169_RX_BUF_SIZE));
- tp->Rx_databuff[i] = NULL;
- tp->RxDescArray[i].addr = 0;
- tp->RxDescArray[i].opts1 = 0;
+ __free_pages(ring->rx_databuff[i], get_order(R8169_RX_BUF_SIZE));
+ ring->rx_databuff[i] = NULL;
+ ring->rx_desc_phy_addr[i] = 0;
+ ring->rx_desc_array[i].addr = 0;
+ ring->rx_desc_array[i].opts1 = 0;
}
}
-static int rtl8169_rx_fill(struct rtl8169_private *tp)
+static int rtl8169_rx_fill(struct rtl8169_private *tp, struct rtl8169_rx_ring *ring)
{
int i;
for (i = 0; i < NUM_RX_DESC; i++) {
struct page *data;
- data = rtl8169_alloc_rx_data(tp, tp->RxDescArray + i);
+ data = rtl8169_alloc_rx_data(tp, ring, i);
if (!data) {
- rtl8169_rx_clear(tp);
+ rtl8169_rx_clear(tp, ring);
return -ENOMEM;
}
- tp->Rx_databuff[i] = data;
+ ring->rx_databuff[i] = data;
}
/* mark as last descriptor in the ring */
- tp->RxDescArray[NUM_RX_DESC - 1].opts1 |= cpu_to_le32(RingEnd);
+ ring->rx_desc_array[NUM_RX_DESC - 1].opts1 |= cpu_to_le32(RingEnd);
return 0;
}
+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;
+}
+
+static void rtl8169_free_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];
+ if (ring->rx_desc_array) {
+ dma_free_coherent(&pdev->dev,
+ R8169_RX_RING_BYTES,
+ ring->rx_desc_array,
+ ring->rx_phy_addr);
+ ring->rx_desc_array = NULL;
+ }
+ }
+}
+
static int rtl8169_init_ring(struct rtl8169_private *tp)
{
+ int i, ret;
+
rtl8169_init_ring_indexes(tp);
+ rtl8169_rx_desc_init(tp);
memset(tp->tx_skb, 0, sizeof(tp->tx_skb));
- memset(tp->Rx_databuff, 0, sizeof(tp->Rx_databuff));
- return rtl8169_rx_fill(tp);
+ for (i = 0; i < tp->num_rx_rings; i++) {
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
+
+ memset(ring->rx_databuff, 0, sizeof(ring->rx_databuff));
+ ret = rtl8169_rx_fill(tp, ring);
+ if (ret < 0)
+ goto err_clear;
+ }
+ return 0;
+
+err_clear:
+ while (--i >= 0)
+ rtl8169_rx_clear(tp, &tp->rx_ring[i]);
+ return ret;
}
static void rtl8169_unmap_tx_skb(struct rtl8169_private *tp, unsigned int entry)
@@ -4486,16 +4592,23 @@ static void rtl8169_cleanup(struct rtl8169_private *tp)
rtl8169_init_ring_indexes(tp);
}
-static void rtl_reset_work(struct rtl8169_private *tp)
+static void rtl8169_rx_desc_reset(struct rtl8169_private *tp)
{
- int i;
+ for (int i = 0; i < tp->num_rx_rings; i++) {
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
+ for (int j = 0; j < NUM_RX_DESC; j++)
+ rtl8169_mark_to_asic(ring->rx_desc_array + j);
+ }
+}
+
+static void rtl_reset_work(struct rtl8169_private *tp)
+{
netif_stop_queue(tp->dev);
rtl8169_cleanup(tp);
- for (i = 0; i < NUM_RX_DESC; i++)
- rtl8169_mark_to_asic(tp->RxDescArray + i);
+ rtl8169_rx_desc_reset(tp);
rtl8169_napi_enable(tp);
rtl_hw_start(tp);
@@ -4941,7 +5054,8 @@ static inline int rtl8169_fragmented_frame(u32 status)
return (status & (FirstFrag | LastFrag)) != (FirstFrag | LastFrag);
}
-static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
+static inline void rtl8169_rx_csum(struct sk_buff *skb,
+ u32 opts1)
{
u32 status = opts1 & (RxProtoMask | RxCSFailMask);
@@ -4951,15 +5065,30 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
skb_checksum_none_assert(skb);
}
+static bool rtl8169_check_rx_desc_error(struct net_device *dev,
+ struct rtl8169_private *tp,
+ u32 status)
+{
+ if (unlikely(status & RxRES)) {
+ if (status & (RxRWT | RxRUNT))
+ dev->stats.rx_length_errors++;
+ if (status & RxCRC)
+ dev->stats.rx_crc_errors++;
+ return true;
+ }
+ return false;
+}
+
static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
- int budget, struct napi_struct *napi)
+ struct rtl8169_rx_ring *ring, int budget,
+ struct napi_struct *napi)
{
struct device *d = tp_to_dev(tp);
int count;
- for (count = 0; count < budget; count++, tp->cur_rx++) {
- unsigned int pkt_size, entry = tp->cur_rx % NUM_RX_DESC;
- struct RxDesc *desc = tp->RxDescArray + entry;
+ for (count = 0; count < budget; count++, ring->cur_rx++) {
+ unsigned int pkt_size, entry = ring->cur_rx % NUM_RX_DESC;
+ struct RxDesc *desc = ring->rx_desc_array + entry;
struct sk_buff *skb;
const void *rx_buf;
dma_addr_t addr;
@@ -4975,15 +5104,11 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
*/
dma_rmb();
- if (unlikely(status & RxRES)) {
+ if (rtl8169_check_rx_desc_error(dev, tp, status)) {
if (net_ratelimit())
netdev_warn(dev, "Rx ERROR. status = %08x\n",
status);
dev->stats.rx_errors++;
- if (status & (RxRWT | RxRUNT))
- dev->stats.rx_length_errors++;
- if (status & RxCRC)
- dev->stats.rx_crc_errors++;
if (!(dev->features & NETIF_F_RXALL))
goto release_descriptor;
@@ -5010,8 +5135,8 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
goto release_descriptor;
}
- addr = le64_to_cpu(desc->addr);
- rx_buf = page_address(tp->Rx_databuff[entry]);
+ addr = ring->rx_desc_phy_addr[entry];
+ rx_buf = page_address(ring->rx_databuff[entry]);
dma_sync_single_for_cpu(d, addr, pkt_size, DMA_FROM_DEVICE);
prefetch(rx_buf);
@@ -5143,7 +5268,8 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
rtl_tx(dev, tp, budget);
- work_done = rtl_rx(dev, tp, budget, napi);
+ /* rtl8169_poll() is used only when there is a single RX ring. */
+ work_done = rtl_rx(dev, tp, &tp->rx_ring[0], budget, napi);
if (work_done < budget && napi_complete_done(napi, work_done))
rtl_irq_enable(tp);
@@ -5243,18 +5369,17 @@ static int rtl8169_close(struct net_device *dev)
phylink_stop(tp->phylink);
netif_stop_queue(dev);
rtl8169_down(tp);
- rtl8169_rx_clear(tp);
+ for (int i = 0; i < tp->num_rx_rings; i++)
+ rtl8169_rx_clear(tp, &tp->rx_ring[i]);
rtl8169_free_irq(tp);
phylink_disconnect_phy(tp->phylink);
- dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
- tp->RxPhyAddr);
dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
tp->TxPhyAddr);
tp->TxDescArray = NULL;
- tp->RxDescArray = NULL;
+ rtl8169_free_rx_desc(tp);
pm_runtime_put_sync(&pdev->dev);
@@ -5288,10 +5413,8 @@ static int rtl_open(struct net_device *dev)
if (!tp->TxDescArray)
goto out;
- tp->RxDescArray = dma_alloc_coherent(&pdev->dev, R8169_RX_RING_BYTES,
- &tp->RxPhyAddr, GFP_KERNEL);
- if (!tp->RxDescArray)
- goto err_free_tx_0;
+ if (rtl8169_alloc_rx_desc(tp) < 0)
+ goto err_free_rx_1;
retval = rtl8169_init_ring(tp);
if (retval < 0)
@@ -5321,12 +5444,10 @@ static int rtl_open(struct net_device *dev)
rtl8169_free_irq(tp);
err_release_fw_2:
rtl_release_firmware(tp);
- rtl8169_rx_clear(tp);
+ for (int i = 0; i < tp->num_rx_rings; i++)
+ rtl8169_rx_clear(tp, &tp->rx_ring[i]);
err_free_rx_1:
- dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
- tp->RxPhyAddr);
- tp->RxDescArray = NULL;
-err_free_tx_0:
+ rtl8169_free_rx_desc(tp);
dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
tp->TxPhyAddr);
tp->TxDescArray = NULL;
@@ -5493,6 +5614,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
unregister_netdev(tp->dev);
r8169_free_napi(tp);
+ kfree(tp->rx_ring);
phylink_destroy(tp->phylink);
if (tp->dash_type != RTL_DASH_NONE)
@@ -6094,7 +6216,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
u32 txconfig;
u32 xid;
- dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
+ dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*tp),
+ R8169_MAX_TX_QUEUES,
+ R8169_MAX_RX_QUEUES);
+
if (!dev)
return -ENOMEM;
@@ -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;
+ }
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;
}
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v13 2/7] r8169: refactor RX path to prepare for multi-queue
2026-09-10 6:31 ` [PATCH net-next v13 2/7] r8169: refactor RX path to prepare for multi-queue javen
@ 2026-09-16 1:24 ` Jakub Kicinski
0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2026-09-16 1:24 UTC (permalink / raw)
To: javen_xu
Cc: Jakub Kicinski, hkallweit1, nic_swsd, andrew+netdev, davem,
edumazet, pabeni, horms, netdev, linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v13 3/7] r8169: add support for new interrupt mapping
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
2026-09-10 6:31 ` [PATCH net-next v13 1/7] r8169: add support for multi irqs javen
2026-09-10 6:31 ` [PATCH net-next v13 2/7] r8169: refactor RX path to prepare for multi-queue javen
@ 2026-09-10 6:31 ` javen
2026-09-16 1:24 ` Jakub Kicinski
2026-09-10 6:31 ` [PATCH net-next v13 4/7] r8169: enable " javen
` (3 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
To support RSS, the number of hardware interrupt bits should match the
interrupt of software. So we add support for new interrupt mapping here.
ISR_VEC_MAP_REG is the hardware register to indicate interrupt status.
IMR_SET_VEC_MAP_REG is interrupt mask which is set to enable irq.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
- no changes
Changes in v3:
- init index in napi_struct and get message_id from index
- move rtl8169_disable_hw_interrupt_msix directly before the call to
napi_schedule()
- change the condition in rtl8169_request_irq when RTL_VEC_MAP_ENABLE
enabled, use rtl8169_interrupt_msix
Changes in v4:
- remove flag tp->feature, replace tp->features & RTL_VEC_MAP_ENABLE
with tp->irq_nvecs > 1, they are equivalent.
- follow reverse xmas tree, in rtl8169_interrupt_msix(),
rtl8169_poll_msix_rx(), rtl8169_poll_msix_tx(),
rtl8169_poll_msix_other()
- use napi->index in rtl8169_poll_msix_other()
- add a comment to describe RTL8127 MSI-X vector layout
- simplify r8169_init_napi()
Changes in v5:
- replace magic number in rtl8169_poll_msix_tx()
Changes in v6:
- when irq_nvecs <= 1, use register IntrMask_8125, else using vec map
- fix irq sequence in rtl8169_interrupt_msix(), disable interrupts
before clean it
- remove dead code in rtl8169_poll_msix_tx()
Changes in v7:
- remove recheck_desc_ownbit
- change return value of rtl_tx
- remove message_id which only used once
Changes in v8:
- fix rtl8169_netpoll()
- remove tx_done
Changes in v9:
- change the way of getting message_id of napi
Changes in v10:
- no changes
Changes in v11:
- add comment on rtl8169_poll_msix_tx, only use 1 tx
- remove napi for other. Separate napi only for datapath, control path
like linkchg is handled in interrupt function, which will not call
napi any more.
Changes in v12:
- no changes
Changes in v13:
- only request IRQs and napi for active vectors(0~7 rx, 8 tx)
---
drivers/net/ethernet/realtek/r8169_main.c | 215 +++++++++++++++++++---
1 file changed, 189 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index f80ad229ae5f..361e83335423 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -477,8 +477,12 @@ enum rtl8125_registers {
RSS_CTRL_8125 = 0x4500,
Q_NUM_CTRL_8125 = 0x4800,
EEE_TXIDLE_TIMER_8125 = 0x6048,
+ IMR_CLEAR_VEC_MAP_REG = 0x0d00,
+ ISR_VEC_MAP_REG = 0x0d04,
+ IMR_SET_VEC_MAP_REG = 0x0d0c,
};
+#define MSIX_ID_VEC_MAP_LINKCHG 29
#define LEDSEL_MASK_8125 0x23f
#define RX_VLAN_INNER_8125 BIT(22)
@@ -609,6 +613,9 @@ enum rtl_register_content {
/* magic enable v2 */
MagicPacket_v2 = (1 << 16), /* Wake up when receives a Magic Packet */
+#define ISRIMR_LINKCHG BIT(29)
+#define ISRIMR_TOK_Q0 BIT(8)
+#define ISRIMR_ROK_Q0 BIT(0)
};
enum rtl_desc_bit {
@@ -1776,26 +1783,38 @@ static u32 rtl_get_events(struct rtl8169_private *tp)
static void rtl_ack_events(struct rtl8169_private *tp, u32 bits)
{
- if (rtl_is_8125(tp))
- RTL_W32(tp, IntrStatus_8125, bits);
- else
+ if (rtl_is_8125(tp)) {
+ if (tp->irq_nvecs > 1)
+ RTL_W32(tp, ISR_VEC_MAP_REG, bits);
+ else
+ RTL_W32(tp, IntrStatus_8125, bits);
+ } else {
RTL_W16(tp, IntrStatus, bits);
+ }
}
static void rtl_irq_disable(struct rtl8169_private *tp)
{
- if (rtl_is_8125(tp))
- RTL_W32(tp, IntrMask_8125, 0);
- else
+ if (rtl_is_8125(tp)) {
+ if (tp->irq_nvecs > 1)
+ RTL_W32(tp, IMR_CLEAR_VEC_MAP_REG, 0xffffffff);
+ else
+ RTL_W32(tp, IntrMask_8125, 0);
+ } else {
RTL_W16(tp, IntrMask, 0);
+ }
}
static void rtl_irq_enable(struct rtl8169_private *tp)
{
- if (rtl_is_8125(tp))
- RTL_W32(tp, IntrMask_8125, tp->irq_mask);
- else
+ if (rtl_is_8125(tp)) {
+ if (tp->irq_nvecs > 1)
+ RTL_W32(tp, IMR_SET_VEC_MAP_REG, tp->irq_mask);
+ else
+ RTL_W32(tp, IntrMask_8125, tp->irq_mask);
+ } else {
RTL_W16(tp, IntrMask, tp->irq_mask);
+ }
}
static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
@@ -4545,13 +4564,17 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
static void rtl8169_napi_disable(struct rtl8169_private *tp)
{
- for (int i = 0; i < tp->irq_nvecs; i++)
+ int napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
+
+ for (int i = 0; i < napi_num; i++)
napi_disable(&tp->rtl8169_napi[i]);
}
static void rtl8169_napi_enable(struct rtl8169_private *tp)
{
- for (int i = 0; i < tp->irq_nvecs; i++)
+ int napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
+
+ for (int i = 0; i < napi_num; i++)
napi_enable(&tp->rtl8169_napi[i]);
}
@@ -5198,13 +5221,66 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
return IRQ_HANDLED;
}
+static void rtl8169_free_one_irq(struct rtl8169_private *tp, int i)
+{
+ if (tp->irq_nvecs > 1) {
+ if (i <= R8169_MAX_RX_QUEUES)
+ pci_free_irq(tp->pci_dev, i, &tp->rtl8169_napi[i]);
+ else if (i == MSIX_ID_VEC_MAP_LINKCHG)
+ pci_free_irq(tp->pci_dev, i, tp);
+ } else {
+ pci_free_irq(tp->pci_dev, i, &tp->rtl8169_napi[i]);
+ }
+}
+
static void rtl8169_free_irq(struct rtl8169_private *tp)
{
- for (int i = 0; i < tp->irq_nvecs; i++) {
- struct napi_struct *napi = &tp->rtl8169_napi[i];
+ for (int i = 0; i < tp->irq_nvecs; i++)
+ rtl8169_free_one_irq(tp, i);
+}
- pci_free_irq(tp->pci_dev, i, napi);
- }
+static void rtl8169_disable_hw_interrupt_msix(struct rtl8169_private *tp,
+ int message_id)
+{
+ RTL_W32(tp, IMR_CLEAR_VEC_MAP_REG, BIT(message_id));
+}
+
+static void rtl8169_clear_hw_isr(struct rtl8169_private *tp, int message_id)
+{
+ RTL_W32(tp, ISR_VEC_MAP_REG, BIT(message_id));
+}
+
+static void rtl8169_enable_hw_interrupt_msix(struct rtl8169_private *tp,
+ int message_id)
+{
+ RTL_W32(tp, IMR_SET_VEC_MAP_REG, BIT(message_id));
+}
+
+static irqreturn_t rtl8169_interrupt_msix(int irq, void *dev_instance)
+{
+ struct napi_struct *napi = dev_instance;
+ struct net_device *dev = napi->dev;
+ struct rtl8169_private *tp;
+ int message_id;
+
+ tp = netdev_priv(dev);
+ message_id = napi - tp->rtl8169_napi;
+
+ rtl8169_disable_hw_interrupt_msix(tp, message_id);
+ rtl8169_clear_hw_isr(tp, message_id);
+
+ napi_schedule(napi);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t rtl8169_interrupt_other(int irq, void *dev_instance)
+{
+ struct rtl8169_private *tp = dev_instance;
+
+ rtl8169_clear_hw_isr(tp, MSIX_ID_VEC_MAP_LINKCHG);
+ phy_mac_interrupt(tp->phydev);
+ return IRQ_HANDLED;
}
static int rtl8169_request_irq(struct rtl8169_private *tp)
@@ -5215,8 +5291,27 @@ static int rtl8169_request_irq(struct rtl8169_private *tp)
for (i = 0; i < tp->irq_nvecs; i++) {
napi = &tp->rtl8169_napi[i];
- rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
- NULL, napi, "%s-%d", dev->name, i);
+ if (tp->irq_nvecs > 1) {
+ /* RX: 0~7, TX: 8*/
+ if (i <= R8169_MAX_RX_QUEUES)
+ rc = pci_request_irq(tp->pci_dev, i,
+ rtl8169_interrupt_msix,
+ NULL, napi, "%s-%d",
+ dev->name, i);
+ else if (i == MSIX_ID_VEC_MAP_LINKCHG)
+ rc = pci_request_irq(tp->pci_dev, i,
+ rtl8169_interrupt_other,
+ NULL, tp, "%s-%d",
+ dev->name, i);
+ else
+ continue;
+ } else {
+ rc = pci_request_irq(tp->pci_dev, i,
+ rtl8169_interrupt,
+ NULL, napi, "%s-%d",
+ dev->name, i);
+ }
+
if (rc)
goto free_irq;
}
@@ -5224,7 +5319,7 @@ static int rtl8169_request_irq(struct rtl8169_private *tp)
free_irq:
while (--i >= 0)
- pci_free_irq(tp->pci_dev, i, &tp->rtl8169_napi[i]);
+ rtl8169_free_one_irq(tp, i);
return rc;
}
@@ -5390,9 +5485,16 @@ static int rtl8169_close(struct net_device *dev)
static void rtl8169_netpoll(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
+ int napi_num;
- for (int i = 0; i < tp->irq_nvecs; i++)
- rtl8169_interrupt(0, &tp->rtl8169_napi[i]);
+ napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
+
+ for (int i = 0; i < napi_num; i++) {
+ if (tp->irq_nvecs > 1)
+ rtl8169_interrupt_msix(0, &tp->rtl8169_napi[i]);
+ else
+ rtl8169_interrupt(0, &tp->rtl8169_napi[i]);
+ }
}
#endif
@@ -5594,7 +5696,9 @@ static void rtl_shutdown(struct pci_dev *pdev)
static void r8169_free_napi(struct rtl8169_private *tp)
{
- for (int i = 0; i < tp->irq_nvecs; i++)
+ int napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
+
+ for (int i = 0; i < napi_num; i++)
netif_napi_del(&tp->rtl8169_napi[i]);
kfree(tp->rtl8169_napi);
@@ -5648,10 +5752,16 @@ static const struct net_device_ops rtl_netdev_ops = {
static void rtl_set_irq_mask(struct rtl8169_private *tp)
{
- tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
+ if (tp->irq_nvecs > 1) {
+ tp->irq_mask = ISRIMR_LINKCHG | ISRIMR_TOK_Q0;
+ for (int i = 0; i < tp->num_rx_rings; i++)
+ tp->irq_mask |= ISRIMR_ROK_Q0 << i;
+ } else {
+ tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
- if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
- tp->irq_mask |= SYSErr | RxFIFOOver;
+ if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
+ tp->irq_mask |= SYSErr | RxFIFOOver;
+ }
}
static int rtl_alloc_irq(struct rtl8169_private *tp)
@@ -6197,10 +6307,63 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
return 0;
}
+static int rtl8169_poll_msix_rx(struct napi_struct *napi, int budget)
+{
+ struct net_device *dev = napi->dev;
+ struct rtl8169_private *tp;
+ int work_done = 0;
+ int message_id;
+
+ tp = netdev_priv(dev);
+ message_id = napi - tp->rtl8169_napi;
+
+ if (message_id < tp->num_rx_rings)
+ work_done += rtl_rx(dev, tp, &tp->rx_ring[message_id],
+ budget, napi);
+
+ if (work_done < budget && napi_complete_done(napi, work_done))
+ rtl8169_enable_hw_interrupt_msix(tp, message_id);
+
+ return work_done;
+}
+
+static int rtl8169_poll_msix_tx(struct napi_struct *napi, int budget)
+{
+ struct net_device *dev = napi->dev;
+ struct rtl8169_private *tp;
+
+ tp = netdev_priv(dev);
+
+ /* Currently r8169 only supports a single Tx ring.
+ * Therefore, we don't need a per-ring Tx processing loop here.
+ */
+ rtl_tx(dev, tp, budget);
+
+ if (napi_complete_done(napi, 0))
+ rtl8169_enable_hw_interrupt_msix(tp, (int)(napi - tp->rtl8169_napi));
+
+ return 0;
+}
+
+/* RTL8127 MSI-X vector layout:
+ * Vectors 0 .. (RxQs - 1) : Rx Queues
+ * Vectors RxQs .. (RxQs + TxQs - 1) : Tx Queues
+ * NAPI is only allocated for data path
+ */
static void r8169_init_napi(struct rtl8169_private *tp)
{
- for (int i = 0; i < tp->irq_nvecs; i++) {
- netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
+ int napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
+
+ for (int i = 0; i < napi_num; i++) {
+ int (*poll_fn)(struct napi_struct *, int) = rtl8169_poll;
+
+ if (tp->irq_nvecs > 1) {
+ if (i < R8169_MAX_RX_QUEUES)
+ poll_fn = rtl8169_poll_msix_rx;
+ else
+ poll_fn = rtl8169_poll_msix_tx;
+ }
+ netif_napi_add(tp->dev, &tp->rtl8169_napi[i], poll_fn);
netif_napi_set_irq(&tp->rtl8169_napi[i],
pci_irq_vector(tp->pci_dev, i));
}
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v13 3/7] r8169: add support for new interrupt mapping
2026-09-10 6:31 ` [PATCH net-next v13 3/7] r8169: add support for new interrupt mapping javen
@ 2026-09-16 1:24 ` Jakub Kicinski
0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2026-09-16 1:24 UTC (permalink / raw)
To: javen_xu
Cc: Jakub Kicinski, hkallweit1, nic_swsd, andrew+netdev, davem,
edumazet, pabeni, horms, netdev, linux-kernel
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: add support for new interrupt mapping
This adds the vector-map interrupt registers (IMR_CLEAR_VEC_MAP_REG,
ISR_VEC_MAP_REG, IMR_SET_VEC_MAP_REG) and, when more than one IRQ vector is
present, switches to per-vector MSI-X handlers and separate Rx/Tx NAPI poll
functions. A dedicated handler is registered on vector 29 for link change
events.
A few questions below.
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index f80ad229ae5f8..361e833354233 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -477,8 +477,12 @@ enum rtl8125_registers {
> RSS_CTRL_8125 = 0x4500,
> Q_NUM_CTRL_8125 = 0x4800,
> EEE_TXIDLE_TIMER_8125 = 0x6048,
> + IMR_CLEAR_VEC_MAP_REG = 0x0d00,
> + ISR_VEC_MAP_REG = 0x0d04,
> + IMR_SET_VEC_MAP_REG = 0x0d0c,
> };
>
> +#define MSIX_ID_VEC_MAP_LINKCHG 29
> #define LEDSEL_MASK_8125 0x23f
>
> #define RX_VLAN_INNER_8125 BIT(22)
> @@ -609,6 +613,9 @@ enum rtl_register_content {
>
> /* magic enable v2 */
> MagicPacket_v2 = (1 << 16), /* Wake up when receives a Magic Packet */
> +#define ISRIMR_LINKCHG BIT(29)
> +#define ISRIMR_TOK_Q0 BIT(8)
> +#define ISRIMR_ROK_Q0 BIT(0)
> };
[ ... ]
> @@ -5198,13 +5221,66 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> return IRQ_HANDLED;
> }
>
[ ... ]
> +static irqreturn_t rtl8169_interrupt_msix(int irq, void *dev_instance)
> +{
> + struct napi_struct *napi = dev_instance;
> + struct net_device *dev = napi->dev;
> + struct rtl8169_private *tp;
> + int message_id;
> +
> + tp = netdev_priv(dev);
> + message_id = napi - tp->rtl8169_napi;
> +
> + rtl8169_disable_hw_interrupt_msix(tp, message_id);
> + rtl8169_clear_hw_isr(tp, message_id);
> +
> + napi_schedule(napi);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rtl8169_interrupt_other(int irq, void *dev_instance)
> +{
> + struct rtl8169_private *tp = dev_instance;
> +
> + rtl8169_clear_hw_isr(tp, MSIX_ID_VEC_MAP_LINKCHG);
> + phy_mac_interrupt(tp->phydev);
> + return IRQ_HANDLED;
> }
[Severity: Critical]
Can tp->phydev be NULL here?
The existing handler for the same LinkChg event, rtl8169_interrupt(), guards
the pointer and has an SFP fallback:
if (status & LinkChg) {
if (tp->phydev)
phy_mac_interrupt(tp->phydev);
else if (tp->sfp_mode)
phylink_mac_change(tp->phylink,
!!(RTL_R8(tp, PHYstatus) & LinkStatus));
}
tp->phydev is only assigned by r8169_mdio_register(), and rtl_init_one()
calls that only for the non-SFP case:
if (tp->sfp_mode == RTL_SFP_NONE) {
rc = r8169_mdio_register(tp);
...
}
For an RTL8127 in fiber mode rtl_get_sfp_mode() returns RTL_SFP_8127_ATF,
so tp->phydev stays NULL (rtl_open() likewise does
"if (tp->phydev) r8169_phy_connect(tp);").
phy_mac_interrupt() dereferences the pointer straight away:
drivers/net/phy/phy.c:phy_mac_interrupt() {
/* Trigger a state machine change */
phy_trigger_machine(phydev);
}
with phy_queue_state_machine() forming &phydev->state_queue. Since this runs
in hard IRQ context, wouldn't the resulting oops be fatal?
The vector is armed regardless of sfp_mode, because rtl_set_irq_mask() sets
ISRIMR_LINKCHG for every multi-vector configuration, and vector 29 is
registered whenever irq_nvecs reaches MSIX_ID_VEC_MAP_LINKCHG. On RTL8127
get_min_irq_nvecs()/get_max_irq_nvecs() request 30..32 vectors once
"r8169: enable new interrupt mapping" lands, so the trigger is just a fiber
link up/down, e.g. plugging the module or the remote port toggling.
Should this handler mirror the tp->phydev check and the phylink_mac_change()
fallback? As written the SFP link state also never reaches phylink even if
the dereference were guarded.
>
> static int rtl8169_request_irq(struct rtl8169_private *tp)
> @@ -5215,8 +5291,27 @@ static int rtl8169_request_irq(struct rtl8169_private *tp)
>
> for (i = 0; i < tp->irq_nvecs; i++) {
> napi = &tp->rtl8169_napi[i];
> - rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
> - NULL, napi, "%s-%d", dev->name, i);
> + if (tp->irq_nvecs > 1) {
> + /* RX: 0~7, TX: 8*/
> + if (i <= R8169_MAX_RX_QUEUES)
> + rc = pci_request_irq(tp->pci_dev, i,
> + rtl8169_interrupt_msix,
> + NULL, napi, "%s-%d",
> + dev->name, i);
> + else if (i == MSIX_ID_VEC_MAP_LINKCHG)
> + rc = pci_request_irq(tp->pci_dev, i,
> + rtl8169_interrupt_other,
> + NULL, tp, "%s-%d",
> + dev->name, i);
> + else
> + continue;
> + } else {
> + rc = pci_request_irq(tp->pci_dev, i,
> + rtl8169_interrupt,
> + NULL, napi, "%s-%d",
> + dev->name, i);
> + }
> +
> if (rc)
> goto free_irq;
> }
[ ... ]
> @@ -5648,10 +5752,16 @@ static const struct net_device_ops rtl_netdev_ops = {
>
> static void rtl_set_irq_mask(struct rtl8169_private *tp)
> {
> - tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
> + if (tp->irq_nvecs > 1) {
> + tp->irq_mask = ISRIMR_LINKCHG | ISRIMR_TOK_Q0;
> + for (int i = 0; i < tp->num_rx_rings; i++)
> + tp->irq_mask |= ISRIMR_ROK_Q0 << i;
> + } else {
> + tp->irq_mask = RxOK | RxErr | TxOK | TxErr | LinkChg;
>
> - if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
> - tp->irq_mask |= SYSErr | RxFIFOOver;
> + if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
> + tp->irq_mask |= SYSErr | RxFIFOOver;
> + }
> }
[ ... ]
> @@ -6197,10 +6307,63 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
> return 0;
> }
>
> +static int rtl8169_poll_msix_rx(struct napi_struct *napi, int budget)
> +{
> + struct net_device *dev = napi->dev;
> + struct rtl8169_private *tp;
> + int work_done = 0;
> + int message_id;
> +
> + tp = netdev_priv(dev);
> + message_id = napi - tp->rtl8169_napi;
> +
> + if (message_id < tp->num_rx_rings)
> + work_done += rtl_rx(dev, tp, &tp->rx_ring[message_id],
> + budget, napi);
> +
> + if (work_done < budget && napi_complete_done(napi, work_done))
> + rtl8169_enable_hw_interrupt_msix(tp, message_id);
> +
> + return work_done;
> +}
[ ... ]
> +/* RTL8127 MSI-X vector layout:
> + * Vectors 0 .. (RxQs - 1) : Rx Queues
> + * Vectors RxQs .. (RxQs + TxQs - 1) : Tx Queues
> + * NAPI is only allocated for data path
> + */
[Severity: Low]
This isn't a bug, but does this comment match the code below it?
The comment says the Tx vectors start at RxQs, while r8169_init_napi() keys
the Tx poll function on the fixed constant R8169_MAX_RX_QUEUES, and
rtl8169_request_irq() states the layout differently:
/* RX: 0~7, TX: 8*/
if (i <= R8169_MAX_RX_QUEUES)
The hardware defines added in this patch also point at a fixed boundary,
ISRIMR_ROK_Q0 = BIT(0) and ISRIMR_TOK_Q0 = BIT(8), i.e. Tx is bit 8 no
matter how many Rx rings are active. With tp->num_rx_rings typically 1, 2 or
4, the comment would place Tx on vector 1, 2 or 4, which is where the code
installs rtl8169_poll_msix_rx().
Should the comment also mention the third handler this patch registers, on
vector 29 (MSIX_ID_VEC_MAP_LINKCHG)?
> static void r8169_init_napi(struct rtl8169_private *tp)
> {
> - for (int i = 0; i < tp->irq_nvecs; i++) {
> - netif_napi_add(tp->dev, &tp->rtl8169_napi[i], rtl8169_poll);
> + int napi_num = min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1);
> +
> + for (int i = 0; i < napi_num; i++) {
> + int (*poll_fn)(struct napi_struct *, int) = rtl8169_poll;
> +
> + if (tp->irq_nvecs > 1) {
> + if (i < R8169_MAX_RX_QUEUES)
> + poll_fn = rtl8169_poll_msix_rx;
> + else
> + poll_fn = rtl8169_poll_msix_tx;
> + }
> + netif_napi_add(tp->dev, &tp->rtl8169_napi[i], poll_fn);
> netif_napi_set_irq(&tp->rtl8169_napi[i],
> pci_irq_vector(tp->pci_dev, i));
> }
[Severity: Low]
Should this loop bound be derived from tp->num_rx_rings instead of
R8169_MAX_RX_QUEUES?
With MSI-X this creates 9 NAPI instances (8 Rx + 1 Tx) and calls
netif_napi_set_irq() for each, while the number of Rx queues reported to the
core and the number of rings allocated both come from tp->num_rx_rings:
rc = netif_set_real_num_queues(tp->dev, 1, tp->num_rx_rings);
...
tp->rx_ring = kcalloc(tp->num_rx_rings, sizeof(struct rtl8169_rx_ring),
GFP_KERNEL);
tp->num_rx_rings is rounddown_pow_of_two(min(netif_get_num_default_rss_queues(),
hw_supp_num_rx_queues)), so it is typically 1, 2 or 4 on machines with fewer
than 16 CPUs. rtl8169_poll_msix_rx() appears to acknowledge the surplus
instances with its "if (message_id < tp->num_rx_rings)" guard, and still
re-arms those vectors through rtl8169_enable_hw_interrupt_msix().
rtl8169_request_irq() also requests handlers for those vectors and
rtl8169_netpoll() kicks their NAPIs. Is reporting more NAPI instances to the
netdev core than there are Rx queues intentional?
One more question, about code outside this diff.
[Severity: Medium]
Do the two new allocation failure paths in rtl_init_one() leak the phylink
instance created by rtl_init_phylink()? These call sites come from the
earlier patches in this series rather than from this commit:
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) {
kfree(tp->rx_ring);
return -ENOMEM;
}
Both return after rtl_init_phylink() has stored the object in tp->phylink,
without calling phylink_destroy(). The neighbouring error paths in the same
function do release it:
if (tp->sfp_mode == RTL_SFP_NONE) {
rc = r8169_mdio_register(tp);
if (rc) {
phylink_destroy(tp->phylink);
return rc;
}
}
...
rc = register_netdev(dev);
if (rc) {
phylink_destroy(tp->phylink);
goto err_free_napi;
}
With 30 to 32 napi_structs requested on RTL8127, is the napi kcalloc()
failure path worth wiring into the same cleanup?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v13 4/7] r8169: enable new interrupt mapping
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
` (2 preceding siblings ...)
2026-09-10 6:31 ` [PATCH net-next v13 3/7] r8169: add support for new interrupt mapping javen
@ 2026-09-10 6:31 ` javen
2026-09-10 6:31 ` [PATCH net-next v13 5/7] r8169: add support and enable rss javen
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
This patch enables new interrupt mapping for RTL8127 and add error pkts
counter per ring.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
- no changes
Changes in v3:
- no changes
Changes in v4:
- no changes
Changes in v5:
- no changes
Changes in v6:
- no changes
Changes in v7:
- no changes
Changes in v8:
- no changes
Changes in v9:
- no changes
Changes in v10:
- no changes
Changes in v11:
- add error pkts counter per ring
Changes in v12:
- drop unrelated change in rtl8169_init_ring()
Changes in v12:
- no changes
---
drivers/net/ethernet/realtek/r8169_main.c | 78 +++++++++++++++++++----
1 file changed, 67 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 361e83335423..6e7a73f6bbb0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -30,6 +30,7 @@
#include <linux/prefetch.h>
#include <linux/ipv6.h>
#include <linux/unaligned.h>
+#include <linux/u64_stats_sync.h>
#include <net/ip6_checksum.h>
#include <net/netdev_queues.h>
@@ -774,6 +775,15 @@ struct rtl8169_rx_ring {
dma_addr_t rx_desc_phy_addr[NUM_RX_DESC];
dma_addr_t rx_phy_addr;
struct page *rx_databuff[NUM_RX_DESC];
+
+ struct {
+ u64 rx_errors;
+ u64 rx_dropped;
+ u64 rx_length_errors;
+ u64 rx_crc_errors;
+ u64 multicast;
+ struct u64_stats_sync syncp;
+ } stats;
};
enum rtl_sfp_mode {
@@ -4090,6 +4100,15 @@ DECLARE_RTL_COND(rtl_mac_ocp_e00e_cond)
return r8168_mac_ocp_read(tp, 0xe00e) & BIT(13);
}
+static void rtl8169_hw_enable_vec_mapping(struct rtl8169_private *tp)
+{
+ u8 tmp;
+
+ tmp = RTL_R8(tp, INT_CFG0_8125);
+ tmp |= INT_CFG0_ENABLE_8125;
+ RTL_W8(tp, INT_CFG0_8125, tmp);
+}
+
static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
{
rtl_pcie_state_l2l3_disable(tp);
@@ -4098,6 +4117,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);
+
/* disable UPS */
r8168_mac_ocp_modify(tp, 0xd40a, 0x0010, 0x0000);
@@ -5088,15 +5110,16 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb,
skb_checksum_none_assert(skb);
}
-static bool rtl8169_check_rx_desc_error(struct net_device *dev,
- struct rtl8169_private *tp,
+static bool rtl8169_check_rx_desc_error(struct rtl8169_rx_ring *ring,
u32 status)
{
if (unlikely(status & RxRES)) {
+ u64_stats_update_begin(&ring->stats.syncp);
if (status & (RxRWT | RxRUNT))
- dev->stats.rx_length_errors++;
+ ring->stats.rx_length_errors++;
if (status & RxCRC)
- dev->stats.rx_crc_errors++;
+ ring->stats.rx_crc_errors++;
+ u64_stats_update_end(&ring->stats.syncp);
return true;
}
return false;
@@ -5127,11 +5150,13 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
*/
dma_rmb();
- if (rtl8169_check_rx_desc_error(dev, tp, status)) {
+ if (rtl8169_check_rx_desc_error(ring, status)) {
if (net_ratelimit())
netdev_warn(dev, "Rx ERROR. status = %08x\n",
status);
- dev->stats.rx_errors++;
+ u64_stats_update_begin(&ring->stats.syncp);
+ ring->stats.rx_errors++;
+ u64_stats_update_end(&ring->stats.syncp);
if (!(dev->features & NETIF_F_RXALL))
goto release_descriptor;
@@ -5147,14 +5172,18 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
* They are seen as a symptom of over-mtu sized frames.
*/
if (unlikely(rtl8169_fragmented_frame(status))) {
- dev->stats.rx_dropped++;
- dev->stats.rx_length_errors++;
+ u64_stats_update_begin(&ring->stats.syncp);
+ ring->stats.rx_dropped++;
+ ring->stats.rx_length_errors++;
+ u64_stats_update_end(&ring->stats.syncp);
goto release_descriptor;
}
skb = napi_alloc_skb(napi, pkt_size);
if (unlikely(!skb)) {
- dev->stats.rx_dropped++;
+ u64_stats_update_begin(&ring->stats.syncp);
+ ring->stats.rx_dropped++;
+ u64_stats_update_end(&ring->stats.syncp);
goto release_descriptor;
}
@@ -5173,8 +5202,11 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
rtl8169_rx_vlan_tag(desc, skb);
- if (skb->pkt_type == PACKET_MULTICAST)
- dev->stats.multicast++;
+ if (skb->pkt_type == PACKET_MULTICAST) {
+ u64_stats_update_begin(&ring->stats.syncp);
+ ring->stats.multicast++;
+ u64_stats_update_end(&ring->stats.syncp);
+ }
napi_gro_receive(napi, skb);
@@ -5568,6 +5600,27 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
netdev_stats_to_stats64(stats, &dev->stats);
dev_fetch_sw_netstats(stats, dev->tstats);
+ for (int i = 0; i < tp->num_rx_rings; i++) {
+ u64 errors, dropped, length_errors, crc_errors, multicast;
+ struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
+ unsigned int start;
+
+ do {
+ start = u64_stats_fetch_begin(&ring->stats.syncp);
+ errors = ring->stats.rx_errors;
+ dropped = ring->stats.rx_dropped;
+ length_errors = ring->stats.rx_length_errors;
+ crc_errors = ring->stats.rx_crc_errors;
+ multicast = ring->stats.multicast;
+ } while (u64_stats_fetch_retry(&ring->stats.syncp, start));
+
+ stats->rx_errors += errors;
+ stats->rx_dropped += dropped;
+ stats->rx_length_errors += length_errors;
+ stats->rx_crc_errors += crc_errors;
+ stats->multicast += multicast;
+ }
+
/*
* Fetch additional counter values missing in stats collected by driver
* from tally counters.
@@ -6576,6 +6629,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (!tp->rx_ring)
return -ENOMEM;
+ for (int i = 0; i < tp->num_rx_rings; i++)
+ u64_stats_init(&tp->rx_ring[i].stats.syncp);
+
tp->rtl8169_napi = kcalloc(tp->irq_nvecs, sizeof(struct napi_struct),
GFP_KERNEL);
if (!tp->rtl8169_napi) {
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH net-next v13 5/7] r8169: add support and enable rss
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
` (3 preceding siblings ...)
2026-09-10 6:31 ` [PATCH net-next v13 4/7] r8169: enable " javen
@ 2026-09-10 6:31 ` javen
2026-09-16 1:24 ` Jakub Kicinski
2026-09-10 6:31 ` [PATCH net-next v13 6/7] r8169: move struct ethtool_ops javen
2026-09-10 6:31 ` [PATCH net-next v13 7/7] r8169: add get_channel support for ethtool javen
6 siblings, 1 reply; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
This patch adds support and enable rss for RTL8127.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
- some changes moved from Patch 2/7
Changes in v3:
- add struct rtl8169_rss_data. Allocate it dynamically when needed.
- define rss_key as an u32 array
- replace some magic bit numbers in rtl8169_set_rss_hash_opt() and
rtl8125_set_rx_q_num()
- use union to combine different rx descriptor, refactor struct RxDesc
- remove dead code from rtl8169_double_check_rss_support()
Changes in v4:
- rename macro definition, e.g R8127_MAX_IRQ to R8127_MAX_NUM_IRQVEC
- change hw_supp_indir_tbl_entries type to unsigned int
- change init_rx_desc_type type to enum
- remove rtl_check_rss_support(), add helper function
rtl_hw_support_rss()
- remove hw_curr_isr_ver, use irq_nvecs to judge whether we should
enable vector interrupt mapping, use tp->num_rx_ring to judge whether
we should enable rss
- remove function rtl8169_double_check_rss_support(), use
rtl8169_set_rx_ring_num() to set num_rx_ring according to tp->irq_nvecs
Changes in v5:
- no changes
Changes in v6:
- change rss_queue_num type from u8 to unsigned int
- fix rx desc clear in rtl8169_rx_clear() for different desc type
- clamping num_rx_ring with rounddown_pow_of_two()
Changes in v7:
- remove unused macro
- change unfixed type in rtl8169_store_reta
Changes in v8:
- refill desc->addr when rx_desc reset
- rtl8169_set_channels fixed in patch 7/7
Changes in v9:
- remove rtl8169_set_desc_dma_addr, only set desc dma addr for
RX_DESC_TYPE_RSS desc
Changes in v10:
- Change rss_key to u8 array and write rss_key_reg as u32 values.
Use get_unaligned_le32() to keep behavior consistent on big-endian
and little-endian
Changes in v11:
- fix compilation error by adding block in switch default case
- fix concurrency bug on updating global dev->stats by using per-queue
stat
- fix packet drop logic to properlly handle fatal errors when rss is
enable
- use get_unaligned_le32() uniformly in rtl8169_store_reta()
- fix coding style issues
- add comment on pci_alloc_irq_vectors() call
Changes in v12:
- add support for UDP rss
Changes in v13:
- no changes
---
drivers/net/ethernet/realtek/r8169_main.c | 404 ++++++++++++++++++++--
1 file changed, 366 insertions(+), 38 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 6e7a73f6bbb0..ad4ed3ffbec1 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -88,6 +88,19 @@
#define R8169_MAX_RX_QUEUES 8
#define R8169_DEFAULT_RX_QUEUES 1
#define R8169_MAX_TX_QUEUES 1
+#define R8127_MAX_NUM_IRQVEC 32
+#define R8127_MIN_NUM_IRQVEC 30
+#define R8169_IRQ_DEFAULT 1
+#define RTL_RSS_KEY_SIZE 40
+#define RSS_CPU_NUM_MASK GENMASK(18, 16)
+#define RSS_HASH_MASK GENMASK(10, 8)
+#define RTL_MAX_INDIRECTION_TABLE_ENTRIES 128
+#define RXS_RSS_UDP BIT(27)
+#define RXS_RSS_IPV4 BIT(28)
+#define RXS_RSS_IPV6 BIT(29)
+#define RXS_RSS_TCP BIT(30)
+#define RXS_RSS_L3_TYPE_MASK (RXS_RSS_IPV4 | RXS_RSS_IPV6)
+#define RXS_RSS_L4_TYPE_MASK (RXS_RSS_TCP | RXS_RSS_UDP)
#define OCP_STD_PHY_BASE 0xa400
@@ -512,6 +525,9 @@ enum rtl_register_content {
RxRUNT = (1 << 20),
RxCRC = (1 << 19),
+ RXRUNT_RSS = (1 << 21),
+ RXCRC_RSS = (1 << 20),
+
/* ChipCmdBits */
StopReq = 0x80,
CmdReset = 0x10,
@@ -617,6 +633,22 @@ enum rtl_register_content {
#define ISRIMR_LINKCHG BIT(29)
#define ISRIMR_TOK_Q0 BIT(8)
#define ISRIMR_ROK_Q0 BIT(0)
+#define RTL_DESC_TYPE_CTRL 0xd8
+#define RSS_KEY_REG 0x4600
+#define RSS_INDIRECTION_TBL_REG 0x4700
+#define RSS_CTRL_TCP_IPV4_SUPP BIT(0)
+#define RTL_DESC_TYPE_RSS BIT(1)
+#define RSS_CTRL_IPV4_SUPP BIT(1)
+#define RSS_CTRL_TCP_IPV6_SUPP BIT(2)
+#define RSS_CTRL_IPV6_SUPP BIT(3)
+#define RSS_CTRL_IPV6_EXT_SUPP BIT(4)
+#define RSS_CTRL_TCP_IPV6_EXT_SUPP BIT(5)
+#define RSS_CTRL_UDP_IPV4_SUPP BIT(11)
+#define RSS_CTRL_UDP_IPV6_SUPP BIT(12)
+#define RX_RES_RSS BIT(22)
+#define RX_RUNT_RSS BIT(21)
+#define RX_CRC_RSS BIT(20)
+#define RTL_RX_Q_NUM_MASK GENMASK(4, 2)
};
enum rtl_desc_bit {
@@ -674,6 +706,11 @@ enum rtl_rx_desc_bit {
#define RxProtoIP (PID1 | PID0)
#define RxProtoMask RxProtoIP
+#define RX_UDPT_DESC_RSS BIT(19)
+#define RX_TCPT_DESC_RSS BIT(18)
+#define RX_UDPF_DESC_RSS BIT(16) /* UDP/IP checksum failed */
+#define RX_TCPF_DESC_RSS BIT(15) /* TCP/IP checksum failed */
+
IPFail = (1 << 16), /* IP checksum failed */
UDPFail = (1 << 15), /* UDP/IP checksum failed */
TCPFail = (1 << 14), /* TCP/IP checksum failed */
@@ -695,9 +732,27 @@ struct TxDesc {
};
struct RxDesc {
- __le32 opts1;
- __le32 opts2;
- __le64 addr;
+ union {
+ /* RX_DESC_TYPE_DEFAULT */
+ struct {
+ __le32 opts1;
+ __le32 opts2;
+ __le64 addr;
+ };
+
+ /* RX_DESC_TYPE_RSS */
+ struct {
+ union {
+ __le64 rss_addr;
+ struct {
+ __le32 rss_info;
+ __le32 rss_result;
+ } rss_dword;
+ };
+ __le32 rss_opts2;
+ __le32 rss_opts1;
+ };
+ };
};
struct ring_info {
@@ -769,6 +824,11 @@ enum rtl_dash_type {
RTL_DASH_25_BP,
};
+enum rx_desc_type {
+ RX_DESC_TYPE_DEFAULT,
+ RX_DESC_TYPE_RSS,
+};
+
struct rtl8169_rx_ring {
u32 cur_rx;
struct RxDesc *rx_desc_array;
@@ -792,6 +852,12 @@ enum rtl_sfp_mode {
RTL_SFP_8127_ATF,
};
+struct rtl8169_rss_data {
+ u8 rss_key[RTL_RSS_KEY_SIZE];
+ u8 rss_indir_tbl[RTL_MAX_INDIRECTION_TABLE_ENTRIES];
+ unsigned int hw_supp_indir_tbl_entries;
+};
+
struct rtl8169_private {
void __iomem *mmio_addr; /* memory map physical address */
struct pci_dev *pci_dev;
@@ -812,7 +878,9 @@ struct rtl8169_private {
u16 tx_lpi_timer;
u32 irq_mask;
unsigned int hw_supp_num_rx_queues;
+ struct rtl8169_rss_data *rss_data;
unsigned int irq_nvecs;
+ enum rx_desc_type init_rx_desc_type;
struct clk *clk;
int speed;
@@ -1727,6 +1795,11 @@ static enum rtl_sfp_mode rtl_get_sfp_mode(struct rtl8169_private *tp)
return RTL_SFP_NONE;
}
+static bool rtl_hw_support_rss(struct rtl8169_private *tp)
+{
+ return tp->mac_version == RTL_GIGA_MAC_VER_80;
+}
+
static enum rtl_dash_type rtl_get_dash_type(struct rtl8169_private *tp)
{
switch (tp->mac_version) {
@@ -2026,9 +2099,20 @@ static inline u32 rtl8169_tx_vlan_tag(struct sk_buff *skb)
TxVlanTag | swab16(skb_vlan_tag_get(skb)) : 0x00;
}
-static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
+static void rtl8169_rx_vlan_tag(struct rtl8169_private *tp,
+ struct RxDesc *desc,
+ struct sk_buff *skb)
{
- u32 opts2 = le32_to_cpu(desc->opts2);
+ u32 opts2;
+
+ switch (tp->init_rx_desc_type) {
+ case RX_DESC_TYPE_RSS:
+ opts2 = le32_to_cpu(desc->rss_opts2);
+ break;
+ default:
+ opts2 = le32_to_cpu(desc->opts2);
+ break;
+ }
if (opts2 & RxVlanTag)
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & 0xffff));
@@ -2832,17 +2916,27 @@ static void rtl_hw_reset(struct rtl8169_private *tp)
rtl_loop_wait_low(tp, &rtl_chipcmd_cond, 100, 100);
}
+static void rtl8169_init_rss(struct rtl8169_private *tp)
+{
+ for (int i = 0; i < tp->rss_data->hw_supp_indir_tbl_entries; i++)
+ tp->rss_data->rss_indir_tbl[i] = ethtool_rxfh_indir_default(i, tp->num_rx_rings);
+
+ netdev_rss_key_fill(tp->rss_data->rss_key, RTL_RSS_KEY_SIZE);
+}
+
static void rtl_setup_rx_params(struct rtl8169_private *tp)
{
tp->num_rx_rings = 1;
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_80:
tp->hw_supp_num_rx_queues = R8169_MAX_RX_QUEUES;
+ tp->rss_data->hw_supp_indir_tbl_entries = RTL_MAX_INDIRECTION_TABLE_ENTRIES;
break;
default:
tp->hw_supp_num_rx_queues = R8169_DEFAULT_RX_QUEUES;
break;
}
+ tp->init_rx_desc_type = RX_DESC_TYPE_DEFAULT;
}
static void rtl_request_firmware(struct rtl8169_private *tp)
@@ -2967,6 +3061,58 @@ static void rtl_set_rx_max_size(struct rtl8169_private *tp)
RTL_W16(tp, RxMaxSize, R8169_RX_BUF_SIZE + 1);
}
+static void rtl8169_store_rss_key(struct rtl8169_private *tp)
+{
+ u8 *rss_key = tp->rss_data->rss_key;
+ const u16 rss_key_reg = RSS_KEY_REG;
+
+ /* Write RSS key to HW */
+ for (int i = 0; i < RTL_RSS_KEY_SIZE; i += sizeof(u32))
+ RTL_W32(tp, rss_key_reg + i, get_unaligned_le32(rss_key + i));
+}
+
+static void rtl8169_store_reta(struct rtl8169_private *tp)
+{
+ u8 *indir_tbl = tp->rss_data->rss_indir_tbl;
+ unsigned int i;
+
+ /* Write redirection table to HW */
+ for (i = 0; i < tp->rss_data->hw_supp_indir_tbl_entries; i += 4) {
+ u32 reta = get_unaligned_le32(&indir_tbl[i]);
+
+ RTL_W32(tp, RSS_INDIRECTION_TBL_REG + i, reta);
+ }
+}
+
+static void rtl8169_set_rss_hash_opt(struct rtl8169_private *tp)
+{
+ u32 rss_ctrl;
+
+ rss_ctrl = FIELD_PREP(RSS_CPU_NUM_MASK, ilog2(tp->num_rx_rings));
+
+ /* Perform hash on these packet types */
+ rss_ctrl |= RSS_CTRL_TCP_IPV4_SUPP |
+ RSS_CTRL_IPV4_SUPP |
+ RSS_CTRL_IPV6_SUPP |
+ RSS_CTRL_IPV6_EXT_SUPP |
+ RSS_CTRL_TCP_IPV6_SUPP |
+ RSS_CTRL_TCP_IPV6_EXT_SUPP |
+ RSS_CTRL_UDP_IPV4_SUPP |
+ RSS_CTRL_UDP_IPV6_SUPP;
+
+ rss_ctrl |= FIELD_PREP(RSS_HASH_MASK,
+ ilog2(tp->rss_data->hw_supp_indir_tbl_entries));
+
+ RTL_W32(tp, RSS_CTRL_8125, rss_ctrl);
+}
+
+static void rtl_set_rss_config(struct rtl8169_private *tp)
+{
+ rtl8169_set_rss_hash_opt(tp);
+ rtl8169_store_reta(tp);
+ rtl8169_store_rss_key(tp);
+}
+
static void rtl_set_rx_tx_desc_registers(struct rtl8169_private *tp)
{
struct rtl8169_rx_ring *ring = &tp->rx_ring[0];
@@ -4100,6 +4246,18 @@ DECLARE_RTL_COND(rtl_mac_ocp_e00e_cond)
return r8168_mac_ocp_read(tp, 0xe00e) & BIT(13);
}
+static void rtl8125_set_rx_q_num(struct rtl8169_private *tp)
+{
+ u16 rx_q_num;
+ u16 q_ctrl;
+
+ rx_q_num = ilog2(tp->num_rx_rings);
+ q_ctrl = RTL_R16(tp, Q_NUM_CTRL_8125);
+ q_ctrl &= ~RTL_RX_Q_NUM_MASK;
+ q_ctrl |= FIELD_PREP(RTL_RX_Q_NUM_MASK, rx_q_num);
+ RTL_W16(tp, Q_NUM_CTRL_8125, q_ctrl);
+}
+
static void rtl8169_hw_enable_vec_mapping(struct rtl8169_private *tp)
{
u8 tmp;
@@ -4139,6 +4297,13 @@ static void rtl_hw_start_8125_common(struct rtl8169_private *tp)
tp->mac_version == RTL_GIGA_MAC_VER_80)
RTL_W8(tp, 0xD8, RTL_R8(tp, 0xD8) & ~0x02);
+ /* enable rx descriptor type v4 and set queue num for rss */
+ if (tp->num_rx_rings > 1) {
+ rtl8125_set_rx_q_num(tp);
+ RTL_W8(tp, RTL_DESC_TYPE_CTRL,
+ RTL_R8(tp, RTL_DESC_TYPE_CTRL) | RTL_DESC_TYPE_RSS);
+ }
+
if (tp->mac_version == RTL_GIGA_MAC_VER_80)
r8168_mac_ocp_modify(tp, 0xe614, 0x0f00, 0x0f00);
else if (tp->mac_version == RTL_GIGA_MAC_VER_70)
@@ -4375,6 +4540,12 @@ static void rtl_hw_start(struct rtl8169_private *tp)
rtl_hw_aspm_clkreq_enable(tp, true);
rtl_set_rx_max_size(tp);
rtl_set_rx_tx_desc_registers(tp);
+ if (rtl_is_8125(tp)) {
+ if (tp->num_rx_rings > 1)
+ rtl_set_rss_config(tp);
+ else
+ RTL_W32(tp, RSS_CTRL_8125, 0x00);
+ }
rtl_lock_config_regs(tp);
rtl_jumbo_config(tp);
@@ -4413,14 +4584,26 @@ static int rtl8169_change_mtu(struct net_device *dev, int new_mtu)
return 0;
}
-static void rtl8169_mark_to_asic(struct RxDesc *desc)
+static void rtl8169_mark_to_asic(struct rtl8169_private *tp, struct RxDesc *desc)
{
- u32 eor = le32_to_cpu(desc->opts1) & RingEnd;
+ u32 eor;
- desc->opts2 = 0;
- /* Force memory writes to complete before releasing descriptor */
- dma_wmb();
- WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE));
+ switch (tp->init_rx_desc_type) {
+ case RX_DESC_TYPE_RSS:
+ eor = le32_to_cpu(desc->rss_opts1) & RingEnd;
+ desc->rss_opts2 = cpu_to_le32(0);
+ /* Force memory writes to complete before releasing descriptor */
+ dma_wmb();
+ WRITE_ONCE(desc->rss_opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE));
+ break;
+ default:
+ eor = le32_to_cpu(desc->opts1) & RingEnd;
+ desc->opts2 = cpu_to_le32(0);
+ /* Force memory writes to complete before releasing descriptor */
+ dma_wmb();
+ WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE));
+ break;
+ }
}
static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
@@ -4443,9 +4626,12 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
return NULL;
}
- desc->addr = cpu_to_le64(mapping);
ring->rx_desc_phy_addr[index] = mapping;
- rtl8169_mark_to_asic(desc);
+ if (tp->init_rx_desc_type == RX_DESC_TYPE_RSS)
+ desc->rss_addr = cpu_to_le64(mapping);
+ else
+ desc->addr = cpu_to_le64(mapping);
+ rtl8169_mark_to_asic(tp, desc);
return data;
}
@@ -4462,8 +4648,25 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp,
__free_pages(ring->rx_databuff[i], get_order(R8169_RX_BUF_SIZE));
ring->rx_databuff[i] = NULL;
ring->rx_desc_phy_addr[i] = 0;
- ring->rx_desc_array[i].addr = 0;
- ring->rx_desc_array[i].opts1 = 0;
+ if (tp->init_rx_desc_type == RX_DESC_TYPE_RSS) {
+ ring->rx_desc_array[i].rss_addr = 0;
+ ring->rx_desc_array[i].rss_opts1 = 0;
+ } else {
+ ring->rx_desc_array[i].addr = 0;
+ ring->rx_desc_array[i].opts1 = 0;
+ }
+ }
+}
+
+static void rtl8169_mark_as_last_descriptor(struct rtl8169_private *tp, struct RxDesc *desc)
+{
+ switch (tp->init_rx_desc_type) {
+ case RX_DESC_TYPE_RSS:
+ desc->rss_opts1 |= cpu_to_le32(RingEnd);
+ break;
+ default:
+ desc->opts1 |= cpu_to_le32(RingEnd);
+ break;
}
}
@@ -4483,7 +4686,7 @@ static int rtl8169_rx_fill(struct rtl8169_private *tp, struct rtl8169_rx_ring *r
}
/* mark as last descriptor in the ring */
- ring->rx_desc_array[NUM_RX_DESC - 1].opts1 |= cpu_to_le32(RingEnd);
+ rtl8169_mark_as_last_descriptor(tp, &ring->rx_desc_array[NUM_RX_DESC - 1]);
return 0;
}
@@ -4642,8 +4845,13 @@ static void rtl8169_rx_desc_reset(struct rtl8169_private *tp)
for (int i = 0; i < tp->num_rx_rings; i++) {
struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
- for (int j = 0; j < NUM_RX_DESC; j++)
- rtl8169_mark_to_asic(ring->rx_desc_array + j);
+ for (int j = 0; j < NUM_RX_DESC; j++) {
+ dma_addr_t phy_addr = ring->rx_desc_phy_addr[j];
+
+ if (tp->init_rx_desc_type == RX_DESC_TYPE_RSS)
+ ring->rx_desc_array[j].rss_addr = cpu_to_le64(phy_addr);
+ rtl8169_mark_to_asic(tp, ring->rx_desc_array + j);
+ }
}
}
@@ -5099,28 +5307,91 @@ static inline int rtl8169_fragmented_frame(u32 status)
return (status & (FirstFrag | LastFrag)) != (FirstFrag | LastFrag);
}
-static inline void rtl8169_rx_csum(struct sk_buff *skb,
- u32 opts1)
+static void rtl8169_rx_hash(struct rtl8169_private *tp,
+ struct RxDesc *desc,
+ struct sk_buff *skb)
+{
+ u32 rss_header_info;
+ u32 hash_val;
+
+ if (!(tp->dev->features & NETIF_F_RXHASH))
+ return;
+
+ rss_header_info = le32_to_cpu(desc->rss_dword.rss_info);
+
+ if (!(rss_header_info & RXS_RSS_L3_TYPE_MASK))
+ return;
+
+ hash_val = le32_to_cpu(desc->rss_dword.rss_result);
+
+ skb_set_hash(skb, hash_val,
+ (RXS_RSS_L4_TYPE_MASK & rss_header_info) ?
+ PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
+}
+
+static void rtl8169_rx_csum(struct rtl8169_private *tp,
+ struct sk_buff *skb,
+ u32 opts1)
{
- u32 status = opts1 & (RxProtoMask | RxCSFailMask);
+ bool csum_ok = false;
+
+ switch (tp->init_rx_desc_type) {
+ case RX_DESC_TYPE_RSS:
+ if (((opts1 & RX_TCPT_DESC_RSS) && !(opts1 & RX_TCPF_DESC_RSS)) ||
+ ((opts1 & RX_UDPT_DESC_RSS) && !(opts1 & RX_UDPF_DESC_RSS)))
+ csum_ok = true;
+ break;
+ default: {
+ u32 status = opts1 & (RxProtoMask | RxCSFailMask);
+
+ if (status == RxProtoTCP || status == RxProtoUDP)
+ csum_ok = true;
+ break;
+ }
+ }
- if (status == RxProtoTCP || status == RxProtoUDP)
+ if (csum_ok)
skb->ip_summed = CHECKSUM_UNNECESSARY;
else
skb_checksum_none_assert(skb);
}
+static __le32 rtl8169_rx_desc_opts1(struct rtl8169_private *tp, struct RxDesc *desc)
+{
+ switch (tp->init_rx_desc_type) {
+ case RX_DESC_TYPE_RSS:
+ return READ_ONCE(desc->rss_opts1);
+ default:
+ return READ_ONCE(desc->opts1);
+ }
+}
+
static bool rtl8169_check_rx_desc_error(struct rtl8169_rx_ring *ring,
+ struct rtl8169_private *tp,
u32 status)
{
- if (unlikely(status & RxRES)) {
- u64_stats_update_begin(&ring->stats.syncp);
- if (status & (RxRWT | RxRUNT))
- ring->stats.rx_length_errors++;
- if (status & RxCRC)
- ring->stats.rx_crc_errors++;
- u64_stats_update_end(&ring->stats.syncp);
- return true;
+ switch (tp->init_rx_desc_type) {
+ case RX_DESC_TYPE_RSS:
+ if (unlikely(status & RX_RES_RSS)) {
+ u64_stats_update_begin(&ring->stats.syncp);
+ if (status & RX_RUNT_RSS)
+ ring->stats.rx_length_errors++;
+ if (status & RX_CRC_RSS)
+ ring->stats.rx_crc_errors++;
+ u64_stats_update_end(&ring->stats.syncp);
+ return true;
+ }
+ break;
+ default:
+ if (unlikely(status & RxRES)) {
+ u64_stats_update_begin(&ring->stats.syncp);
+ if (status & (RxRWT | RxRUNT))
+ ring->stats.rx_length_errors++;
+ if (status & RxCRC)
+ ring->stats.rx_crc_errors++;
+ u64_stats_update_end(&ring->stats.syncp);
+ return true;
+ }
}
return false;
}
@@ -5140,7 +5411,7 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
dma_addr_t addr;
u32 status;
- status = le32_to_cpu(READ_ONCE(desc->opts1));
+ status = le32_to_cpu(rtl8169_rx_desc_opts1(tp, desc));
if (status & DescOwn)
break;
@@ -5150,7 +5421,7 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
*/
dma_rmb();
- if (rtl8169_check_rx_desc_error(ring, status)) {
+ if (rtl8169_check_rx_desc_error(ring, tp, status)) {
if (net_ratelimit())
netdev_warn(dev, "Rx ERROR. status = %08x\n",
status);
@@ -5160,8 +5431,14 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
if (!(dev->features & NETIF_F_RXALL))
goto release_descriptor;
- else if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
- goto release_descriptor;
+
+ if (tp->init_rx_desc_type == RX_DESC_TYPE_DEFAULT) {
+ if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
+ goto release_descriptor;
+ } else {
+ if (!(status & (RXRUNT_RSS | RXCRC_RSS)))
+ goto release_descriptor;
+ }
}
pkt_size = status & GENMASK(13, 0);
@@ -5197,10 +5474,12 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
skb->len = pkt_size;
dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
- rtl8169_rx_csum(skb, status);
+ if (tp->num_rx_rings > 1)
+ rtl8169_rx_hash(tp, desc, skb);
+ rtl8169_rx_csum(tp, skb, status);
skb->protocol = eth_type_trans(skb, dev);
- rtl8169_rx_vlan_tag(desc, skb);
+ rtl8169_rx_vlan_tag(tp, desc, skb);
if (skb->pkt_type == PACKET_MULTICAST) {
u64_stats_update_begin(&ring->stats.syncp);
@@ -5212,7 +5491,9 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
dev_sw_netstats_rx_add(dev, pkt_size);
release_descriptor:
- rtl8169_mark_to_asic(desc);
+ if (tp->init_rx_desc_type == RX_DESC_TYPE_RSS)
+ desc->rss_addr = cpu_to_le64(ring->rx_desc_phy_addr[entry]);
+ rtl8169_mark_to_asic(tp, desc);
}
return count;
@@ -5817,6 +6098,32 @@ static void rtl_set_irq_mask(struct rtl8169_private *tp)
}
}
+static int get_max_irq_nvecs(struct rtl8169_private *tp)
+{
+ if (tp->mac_version == RTL_GIGA_MAC_VER_80)
+ return R8127_MAX_NUM_IRQVEC;
+ return R8169_IRQ_DEFAULT;
+}
+
+static int get_min_irq_nvecs(struct rtl8169_private *tp)
+{
+ if (tp->mac_version == RTL_GIGA_MAC_VER_80)
+ return R8127_MIN_NUM_IRQVEC;
+ return R8169_IRQ_DEFAULT;
+}
+
+static void rtl8169_set_rx_ring_num(struct rtl8169_private *tp)
+{
+ if (tp->irq_nvecs >= get_min_irq_nvecs(tp)) {
+ unsigned int rss_queue_num = netif_get_num_default_rss_queues();
+
+ tp->num_rx_rings = rounddown_pow_of_two(min(rss_queue_num,
+ tp->hw_supp_num_rx_queues));
+ if (tp->num_rx_rings >= 2)
+ tp->init_rx_desc_type = RX_DESC_TYPE_RSS;
+ }
+}
+
static int rtl_alloc_irq(struct rtl8169_private *tp)
{
struct pci_dev *pdev = tp->pci_dev;
@@ -5837,7 +6144,11 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
break;
}
- nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
+ nvecs = pci_alloc_irq_vectors(pdev, get_min_irq_nvecs(tp),
+ get_max_irq_nvecs(tp), flags);
+
+ if (nvecs < 0)
+ nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
if (nvecs < 0)
return nvecs;
@@ -6511,6 +6822,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
tp->dash_type = rtl_get_dash_type(tp);
tp->dash_enabled = rtl_dash_is_enabled(tp);
+ if (rtl_hw_support_rss(tp)) {
+ tp->rss_data = devm_kzalloc(&pdev->dev, sizeof(*tp->rss_data),
+ GFP_KERNEL);
+ if (!tp->rss_data)
+ return -ENOMEM;
+ }
+
tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
if (sizeof(dma_addr_t) > 4 && tp->mac_version >= RTL_GIGA_MAC_VER_18 &&
@@ -6531,6 +6849,11 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (rc < 0)
return dev_err_probe(&pdev->dev, rc, "Can't allocate interrupt\n");
+ rtl8169_set_rx_ring_num(tp);
+
+ if (rtl_hw_support_rss(tp))
+ rtl8169_init_rss(tp);
+
INIT_WORK(&tp->wk.work, rtl_task);
disable_work(&tp->wk.work);
@@ -6543,6 +6866,11 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+ if (rtl_hw_support_rss(tp) && tp->num_rx_rings > 1) {
+ dev->hw_features |= NETIF_F_RXHASH;
+ dev->features |= NETIF_F_RXHASH;
+ }
+
/*
* Pretend we are using VLANs; This bypasses a nasty bug where
* Interrupts stop flowing on high load on 8110SCd controllers.
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH net-next v13 5/7] r8169: add support and enable rss
2026-09-10 6:31 ` [PATCH net-next v13 5/7] r8169: add support and enable rss javen
@ 2026-09-16 1:24 ` Jakub Kicinski
0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2026-09-16 1:24 UTC (permalink / raw)
To: javen_xu
Cc: Jakub Kicinski, hkallweit1, nic_swsd, andrew+netdev, davem,
edumazet, pabeni, horms, netdev, linux-kernel
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: add support and enable rss
This adds RSS support for RTL8127: a new RSS descriptor layout selected
through tp->init_rx_desc_type, an RSS key and indirection table kept in
struct rtl8169_rss_data and programmed from rtl_set_rss_config(), and a
change of the RTL_GIGA_MAC_VER_80 interrupt request to a multi-vector
allocation so more than one RX ring can be used.
The commit message says:
This patch adds support and enable rss for RTL8127.
[Severity: Low]
Could the message describe the behaviour changes that come with it? A few
that are not mentioned:
rtl_alloc_irq() changes the RTL_GIGA_MAC_VER_80 request from
pci_alloc_irq_vectors(pdev, 1, 1, flags) to an all-or-nothing 30..32 vector
request with a fallback to exactly one vector, so a machine that can only
supply 29 vectors silently ends up with a single vector and no RSS.
struct RxDesc becomes a union with a second, differently ordered layout that
every RX accessor now switches on.
rtl_init_one() sets dev->features |= NETIF_F_RXHASH, so RX hashing is on by
default.
rtl8169_set_rss_hash_opt() enables UDP 4-tuple hashing unconditionally via
RSS_CTRL_UDP_IPV4_SUPP | RSS_CTRL_UDP_IPV6_SUPP.
rtl_rx() changes the NETIF_F_RXALL accept/drop decision for the RSS
descriptor format.
Also, "This patch adds support and enable rss" reads better in imperative
mood, e.g. "Add support for RSS and enable it on RTL8127".
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 6e7a73f6bbb07..ad4ed3ffbec18 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
[ ... ]
> @@ -512,6 +525,9 @@ enum rtl_register_content {
> RxRUNT = (1 << 20),
> RxCRC = (1 << 19),
>
> + RXRUNT_RSS = (1 << 21),
> + RXCRC_RSS = (1 << 20),
> +
> /* ChipCmdBits */
> StopReq = 0x80,
> CmdReset = 0x10,
[ ... ]
> @@ -617,6 +633,22 @@ enum rtl_register_content {
> #define ISRIMR_LINKCHG BIT(29)
> #define ISRIMR_TOK_Q0 BIT(8)
> #define ISRIMR_ROK_Q0 BIT(0)
> +#define RTL_DESC_TYPE_CTRL 0xd8
> +#define RSS_KEY_REG 0x4600
> +#define RSS_INDIRECTION_TBL_REG 0x4700
> +#define RSS_CTRL_TCP_IPV4_SUPP BIT(0)
> +#define RTL_DESC_TYPE_RSS BIT(1)
> +#define RSS_CTRL_IPV4_SUPP BIT(1)
> +#define RSS_CTRL_TCP_IPV6_SUPP BIT(2)
> +#define RSS_CTRL_IPV6_SUPP BIT(3)
> +#define RSS_CTRL_IPV6_EXT_SUPP BIT(4)
> +#define RSS_CTRL_TCP_IPV6_EXT_SUPP BIT(5)
> +#define RSS_CTRL_UDP_IPV4_SUPP BIT(11)
> +#define RSS_CTRL_UDP_IPV6_SUPP BIT(12)
> +#define RX_RES_RSS BIT(22)
> +#define RX_RUNT_RSS BIT(21)
> +#define RX_CRC_RSS BIT(20)
> +#define RTL_RX_Q_NUM_MASK GENMASK(4, 2)
> };
[Severity: Low]
This isn't a bug, but the same two descriptor error bits now have two names.
RXRUNT_RSS/RXCRC_RSS are added as enum values above and RX_RUNT_RSS/
RX_CRC_RSS are added as macros here, with identical values:
RXRUNT_RSS = (1 << 21), vs #define RX_RUNT_RSS BIT(21)
RXCRC_RSS = (1 << 20), vs #define RX_CRC_RSS BIT(20)
rtl8169_check_rx_desc_error() uses the macro spellings while rtl_rx() uses
the enum spellings, so a later correction to one copy would miss the other.
Could one set be dropped?
[ ... ]
> @@ -2967,6 +3061,58 @@ static void rtl_set_rx_max_size(struct rtl8169_private *tp)
> RTL_W16(tp, RxMaxSize, R8169_RX_BUF_SIZE + 1);
> }
>
> +static void rtl8169_store_rss_key(struct rtl8169_private *tp)
[ ... ]
> +static void rtl8169_set_rss_hash_opt(struct rtl8169_private *tp)
> +{
> + u32 rss_ctrl;
> +
> + rss_ctrl = FIELD_PREP(RSS_CPU_NUM_MASK, ilog2(tp->num_rx_rings));
> +
> + /* Perform hash on these packet types */
> + rss_ctrl |= RSS_CTRL_TCP_IPV4_SUPP |
> + RSS_CTRL_IPV4_SUPP |
> + RSS_CTRL_IPV6_SUPP |
> + RSS_CTRL_IPV6_EXT_SUPP |
> + RSS_CTRL_TCP_IPV6_SUPP |
> + RSS_CTRL_TCP_IPV6_EXT_SUPP |
> + RSS_CTRL_UDP_IPV4_SUPP |
> + RSS_CTRL_UDP_IPV6_SUPP;
> +
> + rss_ctrl |= FIELD_PREP(RSS_HASH_MASK,
> + ilog2(tp->rss_data->hw_supp_indir_tbl_entries));
> +
> + RTL_W32(tp, RSS_CTRL_8125, rss_ctrl);
> +}
> +
> +static void rtl_set_rss_config(struct rtl8169_private *tp)
> +{
> + rtl8169_set_rss_hash_opt(tp);
> + rtl8169_store_reta(tp);
> + rtl8169_store_rss_key(tp);
> +}
> +
[Severity: Low]
Is the ethtool RSS interface meant to be added later in the series? The
driver now generates and programs a key and an indirection table
(rtl8169_init_rss() fills rss_key via netdev_rss_key_fill() and
rss_indir_tbl via ethtool_rxfh_indir_default(), rtl_set_rss_config() writes
both to hardware), but rtl8169_ethtool_ops gains none of
.get_rxfh_key_size, .get_rxfh_indir_size, .get_rxfh, .set_rxfh, .get_rxnfc
or .set_rxnfc, at this commit or at the end of the series.
So ethtool -x, -X and -N return EOPNOTSUPP: the key cannot be read, the
redirection table cannot be changed, and the UDP 4-tuple hashing enabled
above by RSS_CTRL_UDP_IPV4_SUPP | RSS_CTRL_UDP_IPV6_SUPP cannot be turned
off.
[ ... ]
> @@ -5160,8 +5431,14 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
>
> if (!(dev->features & NETIF_F_RXALL))
> goto release_descriptor;
> - else if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
> - goto release_descriptor;
> +
> + if (tp->init_rx_desc_type == RX_DESC_TYPE_DEFAULT) {
> + if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
> + goto release_descriptor;
> + } else {
> + if (!(status & (RXRUNT_RSS | RXCRC_RSS)))
> + goto release_descriptor;
> + }
> }
>
> pkt_size = status & GENMASK(13, 0);
[Severity: Medium]
This isn't introduced by this patch, but since the accept/drop decision is
being reworked here and a second acceptance branch is added, can pkt_size
underflow for a salvaged error frame?
pkt_size = status & GENMASK(13, 0);
if (likely(!(dev->features & NETIF_F_RXFCS)))
pkt_size -= ETH_FCS_LEN;
With rx-all enabled, a RUNT/CRC descriptor reporting a length of 0..3 makes
the unsigned subtraction wrap to roughly 4 GiB. The following
napi_alloc_skb() does not fail in that case, because it wraps too:
net/core/skbuff.c:napi_alloc_skb() {
...
len += NET_SKB_PAD + NET_IP_ALIGN;
if (len <= SKB_WITH_OVERHEAD(SKB_SMALL_HEAD_CACHE_SIZE) || ...)
skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX | SKB_ALLOC_NAPI, NUMA_NO_NODE);
...
}
0xfffffffc + NET_SKB_PAD + NET_IP_ALIGN wraps to a small value, so a small
head is allocated and skb_copy_to_linear_data(skb, rx_buf, pkt_size) then
copies the un-wrapped length into it, followed by skb->tail += pkt_size and
skb->len = pkt_size. Would a minimum length check before the ETH_FCS_LEN
subtraction be worthwhile here?
[ ... ]
> @@ -5817,6 +6098,32 @@ static void rtl_set_irq_mask(struct rtl8169_private *tp)
> }
> }
>
> +static int get_max_irq_nvecs(struct rtl8169_private *tp)
> +{
> + if (tp->mac_version == RTL_GIGA_MAC_VER_80)
> + return R8127_MAX_NUM_IRQVEC;
> + return R8169_IRQ_DEFAULT;
> +}
> +
> +static int get_min_irq_nvecs(struct rtl8169_private *tp)
> +{
> + if (tp->mac_version == RTL_GIGA_MAC_VER_80)
> + return R8127_MIN_NUM_IRQVEC;
> + return R8169_IRQ_DEFAULT;
> +}
> +
> +static void rtl8169_set_rx_ring_num(struct rtl8169_private *tp)
> +{
> + if (tp->irq_nvecs >= get_min_irq_nvecs(tp)) {
> + unsigned int rss_queue_num = netif_get_num_default_rss_queues();
> +
> + tp->num_rx_rings = rounddown_pow_of_two(min(rss_queue_num,
> + tp->hw_supp_num_rx_queues));
> + if (tp->num_rx_rings >= 2)
> + tp->init_rx_desc_type = RX_DESC_TYPE_RSS;
> + }
> +}
> +
[Severity: Low]
Since tp->num_rx_rings can now be smaller than the number of RX vectors
that were allocated, does rtl8169_netpoll() leave the hardware mask out of
sync with tp->irq_mask?
rtl8169_netpoll() iterates min(tp->irq_nvecs, R8169_MAX_RX_QUEUES + 1)
entries and calls rtl8169_interrupt_msix() for each one. For a vector with
message_id >= tp->num_rx_rings, rtl8169_poll_msix_rx() does no ring work,
completes immediately and still calls rtl8169_enable_hw_interrupt_msix(),
setting that queue's bit in IMR_SET_VEC_MAP_REG, while rtl_set_irq_mask()
deliberately only sets ISRIMR_ROK_Q0 << i for i < tp->num_rx_rings.
Would bounding the loop by tp->num_rx_rings be more consistent?
> static int rtl_alloc_irq(struct rtl8169_private *tp)
> {
> struct pci_dev *pdev = tp->pci_dev;
> @@ -5837,7 +6144,11 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
> break;
> }
>
> - nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
> + nvecs = pci_alloc_irq_vectors(pdev, get_min_irq_nvecs(tp),
> + get_max_irq_nvecs(tp), flags);
> +
> + if (nvecs < 0)
> + nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
>
> if (nvecs < 0)
> return nvecs;
[Severity: High]
This is the first time tp->irq_nvecs can be greater than 1 for
RTL_GIGA_MAC_VER_80, which activates the MSI-X path in
rtl8169_request_irq(). Can that dereference a NULL tp->phydev on the SFP
variant?
rtl8169_request_irq() registers vector MSIX_ID_VEC_MAP_LINKCHG with
rtl8169_interrupt_other() when tp->irq_nvecs > 1, and rtl_set_irq_mask()
sets ISRIMR_LINKCHG so the vector is armed. The handler does:
rtl8169_clear_hw_isr(tp, MSIX_ID_VEC_MAP_LINKCHG);
phy_mac_interrupt(tp->phydev);
while the single-vector handler treats a NULL phydev as an expected
configuration:
rtl8169_interrupt() {
...
if (status & LinkChg) {
if (tp->phydev)
phy_mac_interrupt(tp->phydev);
else if (tp->sfp_mode)
phylink_mac_change(tp->phylink,
!!(RTL_R8(tp, PHYstatus) & LinkStatus));
}
...
}
On an RTL8127 where rtl_get_sfp_mode() returns RTL_SFP_8127_ATF,
rtl_init_one() skips r8169_mdio_register(), which is the only place
tp->phydev is assigned, so tp->phydev stays NULL and the first link change
would dereference NULL in hard IRQ context.
Related: even with a valid phydev, rtl8169_interrupt_other() never performs
the phylink_mac_change() notification, so would link state ever be reported
on SFP boards using the MSI-X path?
[ ... ]
> @@ -6531,6 +6849,11 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (rc < 0)
> return dev_err_probe(&pdev->dev, rc, "Can't allocate interrupt\n");
>
> + rtl8169_set_rx_ring_num(tp);
> +
> + if (rtl_hw_support_rss(tp))
> + rtl8169_init_rss(tp);
> +
[Severity: Medium]
While looking at rtl_init_one() in this series: do the two allocation
failure paths added further down leak the phylink instance created by
rtl_init_phylink()?
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) {
kfree(tp->rx_ring);
return -ENOMEM;
}
Both run after rtl_init_phylink() succeeded, and phylink_create() is not
devm managed. The neighbouring failure paths in the same function do clean
up, e.g.:
rc = register_netdev(dev);
if (rc) {
phylink_destroy(tp->phylink);
goto err_free_napi;
}
These two returns are still without phylink_destroy() at the end of the
series.
[ ... ]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next v13 6/7] r8169: move struct ethtool_ops
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
` (4 preceding siblings ...)
2026-09-10 6:31 ` [PATCH net-next v13 5/7] r8169: add support and enable rss javen
@ 2026-09-10 6:31 ` javen
2026-09-10 6:31 ` [PATCH net-next v13 7/7] r8169: add get_channel support for ethtool javen
6 siblings, 0 replies; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
The patch moves the rtl8169_ethtool_ops definition further down in
r8169_main.c so that subsequent additions of rtl8169_get_channels and
rtl8169_set_channels can be referenced from the ops struct without
needing forward declarations.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
- no changes
Changes in v3:
- no changes
Changes in v4:
- no changes
Changes in v5:
- no changes
Changes in v6:
- modify commit message
Changes in v7:
- no changes
Changes in v8:
- no changes
Changes in v9:
- no changes
Changes in v10:
- no changes
Changes in v11:
- no changes
Changes in v12:
- no changes
Changes in v13:
- no changes
---
drivers/net/ethernet/realtek/r8169_main.c | 56 +++++++++++------------
1 file changed, 28 insertions(+), 28 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ad4ed3ffbec1..a103e04e6fc0 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2645,34 +2645,6 @@ static int rtl8169_nway_reset(struct net_device *dev)
return phylink_ethtool_nway_reset(tp->phylink);
}
-static const struct ethtool_ops rtl8169_ethtool_ops = {
- .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
- ETHTOOL_COALESCE_MAX_FRAMES,
- .get_drvinfo = rtl8169_get_drvinfo,
- .get_regs_len = rtl8169_get_regs_len,
- .get_link = ethtool_op_get_link,
- .get_coalesce = rtl_get_coalesce,
- .set_coalesce = rtl_set_coalesce,
- .get_regs = rtl8169_get_regs,
- .get_wol = rtl8169_get_wol,
- .set_wol = rtl8169_set_wol,
- .get_strings = rtl8169_get_strings,
- .get_sset_count = rtl8169_get_sset_count,
- .get_ethtool_stats = rtl8169_get_ethtool_stats,
- .get_ts_info = ethtool_op_get_ts_info,
- .nway_reset = rtl8169_nway_reset,
- .get_eee = rtl8169_get_eee,
- .set_eee = rtl8169_set_eee,
- .get_link_ksettings = rtl8169_get_link_ksettings,
- .set_link_ksettings = rtl8169_set_link_ksettings,
- .get_ringparam = rtl8169_get_ringparam,
- .get_pause_stats = rtl8169_get_pause_stats,
- .get_pauseparam = rtl8169_get_pauseparam,
- .set_pauseparam = rtl8169_set_pauseparam,
- .get_eth_mac_stats = rtl8169_get_eth_mac_stats,
- .get_eth_ctrl_stats = rtl8169_get_eth_ctrl_stats,
-};
-
static const struct rtl_chip_info *rtl8169_get_chip_version(u32 xid, bool gmii)
{
/* Chips combining a 1Gbps MAC with a 100Mbps PHY */
@@ -6733,6 +6705,34 @@ static void r8169_init_napi(struct rtl8169_private *tp)
}
}
+static const struct ethtool_ops rtl8169_ethtool_ops = {
+ .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
+ ETHTOOL_COALESCE_MAX_FRAMES,
+ .get_drvinfo = rtl8169_get_drvinfo,
+ .get_regs_len = rtl8169_get_regs_len,
+ .get_link = ethtool_op_get_link,
+ .get_coalesce = rtl_get_coalesce,
+ .set_coalesce = rtl_set_coalesce,
+ .get_regs = rtl8169_get_regs,
+ .get_wol = rtl8169_get_wol,
+ .set_wol = rtl8169_set_wol,
+ .get_strings = rtl8169_get_strings,
+ .get_sset_count = rtl8169_get_sset_count,
+ .get_ethtool_stats = rtl8169_get_ethtool_stats,
+ .get_ts_info = ethtool_op_get_ts_info,
+ .nway_reset = rtl8169_nway_reset,
+ .get_eee = rtl8169_get_eee,
+ .set_eee = rtl8169_set_eee,
+ .get_link_ksettings = rtl8169_get_link_ksettings,
+ .set_link_ksettings = rtl8169_set_link_ksettings,
+ .get_ringparam = rtl8169_get_ringparam,
+ .get_pause_stats = rtl8169_get_pause_stats,
+ .get_pauseparam = rtl8169_get_pauseparam,
+ .set_pauseparam = rtl8169_set_pauseparam,
+ .get_eth_mac_stats = rtl8169_get_eth_mac_stats,
+ .get_eth_ctrl_stats = rtl8169_get_eth_ctrl_stats,
+};
+
static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_chip_info *chip;
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH net-next v13 7/7] r8169: add get_channel support for ethtool
2026-09-10 6:31 [PATCH net-next v13 0/7] r8169: add RSS support for RTL8127 javen
` (5 preceding siblings ...)
2026-09-10 6:31 ` [PATCH net-next v13 6/7] r8169: move struct ethtool_ops javen
@ 2026-09-10 6:31 ` javen
6 siblings, 0 replies; 12+ messages in thread
From: javen @ 2026-09-10 6:31 UTC (permalink / raw)
To: hkallweit1, nic_swsd, andrew+netdev, davem, edumazet, kuba,
pabeni, horms
Cc: netdev, linux-kernel, Javen Xu
From: Javen Xu <javen_xu@realsil.com.cn>
Add get_channel support for ethtool.
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v11:
- new file, no change
Changes in v12:
- no changes
Changes in v13:
- no changes
---
drivers/net/ethernet/realtek/r8169_main.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a103e04e6fc0..b6f7d212ed77 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -6705,6 +6705,22 @@ static void r8169_init_napi(struct rtl8169_private *tp)
}
}
+static void rtl8169_get_channels(struct net_device *dev,
+ struct ethtool_channels *ch)
+{
+ struct rtl8169_private *tp = netdev_priv(dev);
+
+ if (tp->irq_nvecs > 1) {
+ ch->max_rx = tp->hw_supp_num_rx_queues;
+ ch->max_tx = 1;
+ ch->rx_count = tp->num_rx_rings;
+ ch->tx_count = 1;
+ } else {
+ ch->max_combined = 1;
+ ch->combined_count = 1;
+ }
+}
+
static const struct ethtool_ops rtl8169_ethtool_ops = {
.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
ETHTOOL_COALESCE_MAX_FRAMES,
@@ -6723,6 +6739,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.nway_reset = rtl8169_nway_reset,
.get_eee = rtl8169_get_eee,
.set_eee = rtl8169_set_eee,
+ .get_channels = rtl8169_get_channels,
.get_link_ksettings = rtl8169_get_link_ksettings,
.set_link_ksettings = rtl8169_set_link_ksettings,
.get_ringparam = rtl8169_get_ringparam,
--
2.43.0
^ permalink raw reply [flat|nested] 12+ messages in thread