Skip to content

Conversation

@opsiff
Copy link
Member

@opsiff opsiff commented Nov 26, 2025

Heiner Kallweit (23):
r8169: add support for the temperature sensor being available from
RTL8125B
r8169: remove original workaround for RTL8125 broken rx issue
r8169: enable SG/TSO on selected chip versions per default
r8169: implement additional ethtool stats ops
r8169: don't take RTNL lock in rtl_task()
r8169: replace custom flag with disable_work() et al
r8169: avoid duplicated messages if loading firmware fails and switch
to warn level
r8169: remove rtl_dash_loop_wait_high/low
r8169: enable EEE at 2.5G per default on RTL8125B
r8169: fix inconsistent indenting in rtl8169_get_eth_mac_stats
r8169: align RTL8125 EEE config with vendor driver
r8169: align RTL8125/RTL8126 PHY config with vendor driver
r8169: align RTL8126 EEE config with vendor driver
r8169: improve initialization of RSS registers on RTL8125/RTL8126
r8169: remove leftover locks after reverted change
r8169: improve __rtl8169_set_wol
r8169: improve rtl_set_d3_pll_down
r8169: align WAKE_PHY handling with r8125/r8126 vendor drivers
r8169: use helper r8169_mod_reg8_cond to simplify rtl_jumbo_config
net: phy: convert eee_broken_modes to a linkmode bitmap
net: phy: add phy_set_eee_broken
r8169: copy vendor driver 2.5G/5G EEE advertisement constraints
r8169: remove redundant hwmon support

drivers/net/ethernet/realtek/r8169_firmware.c | 6 +-
drivers/net/ethernet/realtek/r8169_main.c | 355 ++++++++----------
.../net/ethernet/realtek/r8169_phy_config.c | 28 +-
drivers/net/phy/micrel.c | 2 +-
drivers/net/phy/phy-c45.c | 12 +-
drivers/net/phy/phy-core.c | 21 +-
include/linux/phy.h | 19 +-
7 files changed, 210 insertions(+), 233 deletions(-)

Summary by Sourcery

Update the Realtek r8169 driver to align RTL8125/RTL8126 behavior and capabilities with current upstream and vendor drivers, including improved stats reporting, power management, EEE handling, and workqueue/task management.

New Features:

  • Expose additional ethtool statistics for MAC, pause, and control frames on supported RTL8125 devices.
  • Enable scatter-gather and TSO by default on newer supported chip versions, matching vendor driver behavior.

Bug Fixes:

  • Adjust wake-on-LAN configuration and WAKE_PHY handling to be consistent and compatible with RTL8125/RTL8126 vendor drivers.
  • Refine PHY EEE configuration for RTL8125/RTL8126, including 2.5G/5G advertisement constraints and broken-mode flags, to avoid interoperability issues.

Enhancements:

  • Simplify and harden configuration register access and jumbo frame configuration logic using common helpers.
  • Improve D3 PLL power-down handling across MAC versions to better match hardware capabilities and vendor settings.
  • Align RTL8125/RTL8126 PHY and EEE configuration flows with vendor drivers, including enabling extended tally counters and RSS/queue control register use.
  • Streamline firmware loading to use non-spammy warnings when firmware is missing or fails to load.
  • Rework driver workqueue/task scheduling to use disable_work/enable_work primitives instead of custom flags and RTNL locking, and to remove obsolete DASH wait helpers.

…L8125B

mainline inclusion
from mainline-v6.13-rc1
category: feature

This adds support for the temperature sensor being available from
RTL8125B. Register information was taken from r8125 vendor driver.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
(cherry picked from commit 1ffcc8d)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: other

Now that we have b9c7ac4 ("r8169: disable ALDPS per default for
RTL8125"), the first attempt to fix the issue shouldn't be needed
any longer. So let's effectively revert 621735f ("r8169: fix
rare issue with broken rx after link-down on RTL8125") and see
whether anybody complains.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 854d71c)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: performance

Due to problem reports in the past SG and TSO/TSO6 are disabled per
default. It's not fully clear which chip versions are affected, so we
may impact also users of unaffected chip versions, unless they know
how to use ethtool for enabling SG/TSO/TSO6.
Vendor drivers r8168/r8125 enable SG/TSO/TSO6 for selected chip
versions per default, I'd interpret this as confirmation that these
chip versions are unaffected. So let's do the same here.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
(cherry picked from commit b8bf384)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: feature

This adds support for ethtool standard statistics, and makes use of the
extended hardware statistics being available from RTl8125.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit e3fc513)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: performance

There's not really a benefit here in taking the RTNL lock. The task
handler does exception handling only, so we're in trouble anyway when
we come here, and there's no need to protect against e.g. a parallel
ethtool call.
A benefit of removing the RTNL lock here is that we now can
synchronously cancel the workqueue from a context holding the RTNL mutex.

Signed-off-by: Heiner Kallweit <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
(cherry picked from commit ac48430)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: feature

So far we use a custom flag to define when a task can be scheduled and
when not. Let's use the standard mechanism with disable_work() et al
instead.
Note that in rtl8169_close() we can remove the call to cancel_work()
because we now call disable_work_sync() in rtl8169_down() already.

Signed-off-by: Heiner Kallweit <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
(cherry picked from commit e201594)
Signed-off-by: Wentao Guan <[email protected]>
… to warn level

mainline inclusion
from mainline-v6.13-rc1
category: bugfix

In case of a problem with firmware loading we inform at the driver level,
in addition the firmware load code itself issues warnings. Therefore
switch to firmware_request_nowarn() to avoid duplicated error messages.
In addition switch to warn level because the firmware is optional and
typically just fixes compatibility issues.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Message-ID: <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
(cherry picked from commit 1c105ba)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: other

Remove rtl_dash_loop_wait_high/low to simplify the code.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Message-ID: <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
(cherry picked from commit d64113c)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.12-rc1
category: feature

Register a6d/12 is shadowing register MDIO_AN_EEE_ADV2. So this line
disables advertisement of EEE at 2.5G. Latest vendor driver r8125
doesn't do this (any longer?), so this mode seems to be safe.
EEE saves quite some energy, therefore enable this mode per default.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Message-ID: <[email protected]>
Signed-off-by: Andrew Lunn <[email protected]>
(cherry picked from commit c4e6409)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

This fixes an inconsistent indenting introduced with e3fc513
("r8169: implement additional ethtool stats ops").

Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit b8bd8c4)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

Align the EEE config for RTL8125A/RTL8125B with vendor driver r8125.
This should help to avoid compatibility issues.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit eb90f87)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

This aligns some parameters with vendor driver r8125/r8126 to avoid
compatibility issues. Note that for RTL8125B there's no functional
change, just the open-coded version of the function is replaced.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 4af2f60)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

Align the EEE config for RTL8126A with vendor driver r8126 to avoid
compatibility issues.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit a3d8520)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

Replace the register addresses with the names used in r8125/r8126
vendor driver, and consider that RSS_CTRL_8125 is a 32 bit register.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 2cd02f2)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: other

After e31a9fe ("Revert "r8169: disable ASPM during NAPI poll"")
these locks aren't needed any longer.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 83cb4b4)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: other

Add helper r8169_mod_reg8_cond() what allows to significantly simplify
__rtl8169_set_wol().

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit c507e96)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: other

Make use of new helper r8169_mod_reg8_cond() and move from a switch()
to an if() clause. Benefit is that we don't have to touch this piece of
code each time support for a new chip version is added.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 330dc22)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

Vendor drivers r8125/r8126 apply this additional magic setting when
enabling WAKE_PHY, so do the same here.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit e3e9e90)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: other

Use recently added helper r8169_mod_reg8_cond() to simplify jumbo
mode configuration.

Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 7a3bcd3)
Signed-off-by: Wentao Guan <[email protected]>
@sourcery-ai
Copy link

sourcery-ai bot commented Nov 26, 2025

Reviewer's Guide

Backports a series of upstream r8169 changes to better support RTL8125/8126, including updated PHY/EEE configuration and WOL behavior, new ethtool statistics, refined workqueue and power‑management handling, simplified configuration register access, alignment with vendor driver defaults (e.g., SG/TSO and EEE constraints), and quieter firmware load failure reporting.

Sequence diagram for new ethtool MAC and pause stats retrieval

sequenceDiagram
    actor User
    participant ethtool
    participant net_core
    participant rtl8169_driver as r8169_driver
    participant hw as NIC_hardware

    User->>ethtool: request stats (e.g. ethtool --statistics eth0)
    ethtool->>net_core: SIOCETHTOOL ioctl
    net_core->>rtl8169_driver: call rtl8169_ethtool_ops.get_pause_stats(dev, pause_stats)
    alt rtl8125_only
        rtl8169_driver->>rtl8169_driver: rtl8169_update_counters(tp)
        rtl8169_driver->>hw: DMA read tally counters
        hw-->>rtl8169_driver: updated rtl8169_counters
        rtl8169_driver->>ethtool: fill ethtool_pause_stats
    else non_rtl8125
        rtl8169_driver-->>ethtool: return without updating pause_stats
    end

    net_core->>rtl8169_driver: call rtl8169_ethtool_ops.get_eth_mac_stats(dev, mac_stats)
    rtl8169_driver->>rtl8169_driver: rtl8169_update_counters(tp)
    rtl8169_driver->>hw: DMA read tally counters
    hw-->>rtl8169_driver: updated rtl8169_counters
    rtl8169_driver->>rtl8169_driver: map counters to mac_stats fields
    alt rtl8125_only_fields
        rtl8169_driver->>rtl8169_driver: use extended 8125 counters
    else common_fields
        rtl8169_driver->>rtl8169_driver: use base counters
    end
    rtl8169_driver-->>net_core: filled mac_stats
    net_core-->>ethtool: return stats
    ethtool-->>User: display updated MAC and pause statistics
Loading

Sequence diagram for updated workqueue and link-change handling

sequenceDiagram
    participant PHY as phy_device
    participant phylink as phylink_handler
    participant r8169 as r8169_driver
    participant workq as system_workqueue

    PHY-->>phylink: link status change interrupt
    phylink->>r8169: r8169_phylink_handler(ndev)
    alt link_up
        r8169->>r8169: rtl_link_chg_patch(tp)
        r8169->>r8169: pm_request_resume(dev)
        r8169-->>phylink: return
    else link_down
        r8169->>r8169: pm_runtime_idle(dev)
        r8169-->>phylink: return
    end

    note over r8169: On TX timeout or reset conditions
    r8169->>r8169: rtl_schedule_task(tp, flag)
    r8169->>r8169: set_bit(flag, tp->wk.flags)
    r8169->>workq: schedule_work(&tp->wk.work)
    alt schedule_success
        workq-->>r8169: execute rtl_task(work)
        r8169->>r8169: check and clear wk.flags
        alt TX_timeout_flag
            r8169->>r8169: reset secondary PCI bus if needed
            r8169->>r8169: rtl_tx_timeout_work()
        else RESET_flags
            r8169->>r8169: rtl_reset_work(tp)
        end
        r8169-->>workq: return
    else already_queued
        workq-->>r8169: schedule_work returns false
        r8169->>r8169: clear_bit(flag, tp->wk.flags)
    end

    note over r8169: Interface down/up lifecycle
    r8169->>r8169: rtl8169_down(tp)
    r8169->>r8169: disable_work_sync(&tp->wk.work)
    r8169->>r8169: bitmap_zero(tp->wk.flags)

    r8169->>r8169: rtl8169_up(tp)
    r8169->>r8169: enable_work(&tp->wk.work)
    r8169->>r8169: rtl_reset_work(tp)
    r8169-->>PHY: phy_start(tp->phydev)
Loading

Class diagram for updated r8169 driver structures and helpers

classDiagram
    class rtl8169_private {
        +struct net_device* dev
        +struct work_wrapper wk
        +struct phy_device* phydev
        +struct rtl8169_counters* counters
        +bool dash_enabled
        +enum mac_version mac_version
        +u16 ocp_base
        +u8 supports_gmii
        +u8 aspm_manageable
        +u8 dash_enabled
        +unsigned long wk_flags[RTL_FLAG_MAX]
        +raw_spinlock_t mac_ocp_lock
        +struct mutex led_lock
        +int irq
        +void rtl_jumbo_config()
        +void rtl_set_d3_pll_down(bool enable)
        +void __rtl8169_set_wol(u32 wolopts)
        +void rtl8169_get_ringparam()
        +void rtl8169_get_pause_stats()
        +void rtl8169_get_pauseparam()
        +int  rtl8169_set_pauseparam()
        +void rtl8169_get_eth_mac_stats()
        +void rtl8169_get_eth_ctrl_stats()
        +void rtl_schedule_task(enum rtl_flag flag)
        +void rtl8169_down()
        +void rtl8169_up()
    }

    class work_wrapper {
        +struct work_struct work
        +unsigned long flags[RTL_FLAG_MAX]
    }

    class work_struct {
        +work_func_t func
        +void enable_work()
        +void disable_work()
    }

    class net_device {
        +netdev_features_t features
        +netdev_features_t hw_features
        +unsigned int mtu
    }

    class phy_device {
        +void phy_support_eee()
        +void phy_support_asym_pause()
        +void phy_set_eee_broken(u32 modes)
    }

    class rtl8169_counters {
        +__le64 tx_packets
        +__le64 rx_packets
        +__le32 tx_one_collision
        +__le32 tx_multi_collision
        +__le16 align_errors
        +__le32 align_errors32
        +__le64 tx_errors
        +__le64 rx_broadcast
        +__le32 rx_multicast
        +__le64 tx_octets
        +__le32 tx_late_collision
        +__le32 tx_aborted32
        +__le64 rx_octets
        +__le32 rx_mac_error
        +__le64 tx_multicast64
        +__le64 tx_broadcast64
        +__le64 rx_multicast64
        +__le32 rx_frame_too_long
        +__le32 tx_pause_on
        +__le32 rx_pause_on
        +__le32 rx_unknown_opcode
    }

    class rtl8169_ethtool_ops {
        +get_ringparam
        +get_pause_stats
        +get_pauseparam
        +set_pauseparam
        +get_eth_mac_stats
        +get_eth_ctrl_stats
    }

    class helper_functions {
        +void r8169_mod_reg8_cond(rtl8169_private* tp, int reg, u8 bits, bool cond)
        +void rtl_mod_config2(rtl8169_private* tp, u8 clear, u8 set)
        +void rtl_mod_config5(rtl8169_private* tp, u8 clear, u8 set)
    }

    rtl8169_private o-- work_wrapper : has
    work_wrapper o-- work_struct : wraps
    rtl8169_private --> net_device : dev
    rtl8169_private --> phy_device : phydev
    rtl8169_private --> rtl8169_counters : counters
    rtl8169_ethtool_ops ..> rtl8169_private : operates_on
    helper_functions ..> rtl8169_private : config_register_access
Loading

File-Level Changes

Change Details Files
Refine config/WOL/jumbo handling via new helper, remove unnecessary locks, and adjust D3 PLL-down behavior across MAC versions.
  • Introduce r8169_mod_reg8_cond() to conditionally set/clear 8-bit registers and use it in WOL, jumbo frame, and power management configuration paths.
  • Remove cfg9346_usage_lock/config25_lock and associated reference counting, simplifying rtl_lock_config_regs()/rtl_unlock_config_regs() and Config2/Config5 modification.
  • Rework __rtl8169_set_wol() to use r8169_mod_reg8_cond(), split out MagicPacket handling per chip family, and align WAKE_PHY/PME signaling with vendor drivers.
  • Simplify rtl_jumbo_config() by replacing multiple per-chip jumbo enable/disable helpers with r8169_mod_reg8_cond() plus direct MaxTxPacketSize writes.
  • Update rtl_set_d3_pll_down() to cover most MAC versions in a range-based check while excluding a few specific versions, using r8169_mod_reg8_cond() to drive D3_NO_PLL_DOWN.
drivers/net/ethernet/realtek/r8169_main.c
Rework workqueue/task and link handling to avoid RTNL in worker, use disable_work/enable_work, and remove the RTL_FLAG_TASK_ENABLED flag.
  • Change rtl_schedule_task() to always set the task bit and call schedule_work(), clearing the bit if scheduling fails.
  • Remove taking the RTNL lock inside rtl_task(), dropping RTL_FLAG_TASK_ENABLED checks and early bailouts.
  • Call disable_work_sync() in rtl8169_down() and rtl_remove_one(), and use enable_work() in rtl8169_up(), instead of manual flagging and cancel_work/cancel_work_sync().
  • Adjust rtl8169_close() to rely on rtl8169_down() cleanup instead of an extra cancel_work(), and remove the link-down reset workaround for RTL8125 in r8169_phylink_handler().
drivers/net/ethernet/realtek/r8169_main.c
Add ethtool MAC/control/pause statistics support based on extended tally counters, and initialize tally hardware for 8125.
  • Implement rtl8169_get_pause_stats(), rtl8169_get_eth_mac_stats(), and rtl8169_get_eth_ctrl_stats() to read from tp->counters after rtl8169_update_counters().
  • Wire the new callbacks into rtl8169_ethtool_ops to expose pause, MAC, and control statistics via ethtool.
  • On RTL8125, enable extended tally counters in rtl_hw_start_8125() and use 32-bit RSS_CTRL_8125 and Q_NUM_CTRL_8125 register definitions when resetting RSS and queue-number control.
  • Fix an indentation issue in rtl8169_get_eth_mac_stats code region while restructuring the function.
drivers/net/ethernet/realtek/r8169_main.c
Align RTL8125/RTL8126 PHY and EEE configuration and EEE advertisement constraints with vendor drivers, including 2.5G/5G behavior.
  • Refactor EEE configuration with rtl8125_common_config_eee_phy() and rtl8125_config_eee_phy(), reusing rtl8168g_config_eee_phy() and sharing code between 8125/8126 variants.
  • Update rtl8125a_2_hw_phy_config(), rtl8125b_hw_phy_config(), rtl8125d_hw_phy_config(), and rtl8126a_hw_phy_config() to call the new helpers, enable GPHY 10M where needed, and ensure ALDPS is disabled consistently.
  • Enable EEE at 2.5G by default on RTL8125B and copy vendor driver constraints for 2.5G/5G EEE advertisement using phy_set_eee_broken() for specific modes.
  • Align RTL8125/8126 PHY configuration sequences (e.g., legacy force mode, specific paged register tweaks) with those in the vendor drivers for more consistent behavior.
drivers/net/ethernet/realtek/r8169_phy_config.c
drivers/net/ethernet/realtek/r8169_main.c
Adjust SG/TSO defaults and quiet firmware loading failures while avoiding duplicate error messages.
  • Change SG/TSO default enabling logic so that for MAC versions >= RTL_GIGA_MAC_VER_46 (except VER_61) dev->features are initialized from dev->hw_features, mimicking the vendor driver while still allowing opt-in on older versions.
  • Switch firmware loading from request_firmware() to firmware_request_nowarn(), and downgrade failure logging from dev_err() to dev_warn() to avoid noisy/duplicated reports when firmware is optional or expected to be absent.
drivers/net/ethernet/realtek/r8169_main.c
drivers/net/ethernet/realtek/r8169_firmware.c

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from opsiff. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

mainline inclusion
from mainline-v6.13-rc1
category: bugfix

eee_broken_modes has a eee_cap1 register layout currently. This doen't
allow to flag e.g. 2.5Gbps or 5Gbps BaseT EEE as broken. To overcome
this limitation switch eee_broken_modes to a linkmode bitmap.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 721aa69)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

Add an accessor for eee_broken_modes, so that drivers
don't have to deal with phylib internals.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit ed623fb)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13-rc1
category: bugfix

Vendor driver r8125 doesn't advertise 2.5G EEE on RTL8125A, and r8126
doesn't advertise 5G EEE. Likely there are compatibility issues,
therefore do the same in r8169.
With this change we don't have to disable 2.5G EEE advertisement in
rtl8125a_config_eee_phy() any longer.
We use new phylib accessor phy_set_eee_broken() to mark the respective
EEE modes as broken.

Signed-off-by: Heiner Kallweit <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit e340bff)
Signed-off-by: Wentao Guan <[email protected]>
mainline inclusion
from mainline-v6.13
category: other

The temperature sensor is actually part of the integrated PHY and available
also on the standalone versions of the PHY. Therefore hwmon support will
be added to the Realtek PHY driver and can be removed here.

Fixes: 1ffcc8d ("r8169: add support for the temperature sensor being available from RTL8125B")
Signed-off-by: Heiner Kallweit <[email protected]>
Reviewed-by: Jacob Keller <[email protected]>
Link: https://patch.msgid.link/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
(cherry picked from commit 1f691a1)
Signed-off-by: Wentao Guan <[email protected]>
@opsiff opsiff force-pushed the linux-6.12-2025-11-26-r8169-v6.13 branch from 378ce2e to 3d14fcc Compare November 26, 2025 10:41
@deepin-ci-robot
Copy link

deepin pr auto review

我来对这段代码进行详细的审查和分析:

  1. 代码质量和语法逻辑:
  • 代码整体结构清晰,遵循Linux内核编程规范
  • 函数命名规范,使用统一的命名风格
  • 代码注释充分,关键操作都有说明
  • 错误处理机制完善
  1. 主要改进点:

a) 锁机制优化:

  • 移除了不必要的自旋锁 config25_lockcfg9346_usage_lock
  • 简化了寄存器访问的同步机制,使用新的 r8169_mod_reg8_cond() 函数替代原有的锁保护操作
  • 这提高了代码的可维护性和性能

b) 错误处理改进:

  • dev_err() 改为 dev_warn(),因为固件加载失败不应被视为严重错误
  • 使用 firmware_request_nowarn() 替代 request_firmware(),减少不必要的警告信息

c) 性能优化:

  • 新增 r8169_mod_reg8_cond() 函数,只在值确实改变时才写入寄存器
  • 优化了工作队列的处理方式,使用 disable_work_sync()enable_work()
  • 移除了冗余的标志位检查

d) 功能增强:

  • 添加了新的统计信息收集功能,包括暂停帧统计和MAC统计
  • 增加了对新硬件版本的支持
  • 改进了EEE(Energy Efficient Ethernet)配置机制

e) 安全性改进:

  • 使用 linkmode_fill() 替代直接赋值,提高类型安全性
  • 改进了PHY配置的错误处理机制
  1. 建议改进:

a) 错误处理:

// 建议添加更详细的错误日志
if (rc < 0) {
    dev_warn(rtl_fw->dev, "Unable to load firmware %s (%d): %s\n",
             rtl_fw->fw_name, rc, strerror(-rc));
    goto out;
}

b) 性能优化:

// 建议在频繁访问的寄存器操作中使用缓存
static void r8169_mod_reg8_cond(struct rtl8169_private *tp, int reg,
                               u8 bits, bool cond)
{
    static u8 reg_cache[256]; // 添加寄存器缓存
    u8 val, old_val = reg_cache[reg];
    
    if (cond)
        val = old_val | bits;
    else
        val = old_val & ~bits;
        
    if (val != old_val) {
        RTL_W8(tp, reg, val);
        reg_cache[reg] = val;
    }
}

c) 安全性增强:

// 建议添加边界检查
static void r8169_mod_reg8_cond(struct rtl8169_private *tp, int reg,
                               u8 bits, bool cond)
{
    if (reg < 0 || reg > 255) {
        dev_warn(tp->dev, "Invalid register address: %d\n", reg);
        return;
    }
    // ... 其余代码
}
  1. 总体评价:

这是一个高质量的代码改进,主要优化了性能、简化了代码结构,并增强了功能。改进后的代码更易于维护,同时保持了良好的性能和安全性。建议的改进主要集中在错误处理、性能优化和安全性增强方面,这些改进可以进一步提高代码的健壮性和可维护性。

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request backports r8169 Ethernet driver updates from mainline Linux v6.13 to improve support for RTL8125 and RTL8126 chipsets, aligning behavior with vendor drivers. The changes primarily focus on refactoring PHY EEE configuration, improving workqueue management, simplifying register access patterns, and enabling additional hardware features.

Key Changes:

  • Converted PHY eee_broken_modes from u32 to linkmode bitmap with new phy_set_eee_broken() helper for better extensibility
  • Replaced custom task management flags with kernel's standard disable_work/enable_work primitives for cleaner workqueue lifecycle management
  • Enabled SG/TSO by default for chip versions >= RTL_GIGA_MAC_VER_46 (except VER_61), matching vendor driver behavior
  • Added ethtool statistics operations (get_pause_stats, get_eth_mac_stats, get_eth_ctrl_stats) exposing hardware counters for RTL8125 devices

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
include/linux/phy.h Added phy_set_eee_broken() helper and converted eee_broken_modes to linkmode bitmap
drivers/net/phy/phy-core.c Updated of_set_phy_eee_broken() to use linkmode operations instead of bitwise ops
drivers/net/phy/phy-c45.c Modified genphy_c45_write_eee_adv() to filter advertised EEE modes using linkmode operations
drivers/net/phy/micrel.c Updated Micrel driver to use linkmode_fill() for disabling all EEE modes
drivers/net/ethernet/realtek/r8169_phy_config.c Consolidated EEE PHY configuration functions and aligned RTL8125/RTL8126 PHY settings with vendor drivers
drivers/net/ethernet/realtek/r8169_firmware.c Changed firmware loading to use firmware_request_nowarn() and reduced error message severity to warning
drivers/net/ethernet/realtek/r8169_main.c Major refactoring: removed custom task management, added ethtool stats ops, simplified register access with r8169_mod_reg8_cond(), improved WoL handling, enabled SG/TSO by default on select chips, removed redundant spinlocks, and configured EEE broken modes for 2.5G/5G

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2456 to +2457
if (!schedule_work(&tp->wk.work))
clear_bit(flag, tp->wk.flags);
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential race condition: If schedule_work returns false (work already queued), clearing the flag can cause the flag to be lost. The already-queued work should process this flag, but it's being cleared here. Consider removing the clear_bit call, or protecting the entire set_bit + schedule_work sequence with a lock. The correct behavior is: if work is already queued, the flag should remain set so the work can process it when it runs.

Suggested change
if (!schedule_work(&tp->wk.work))
clear_bit(flag, tp->wk.flags);
schedule_work(&tp->wk.work);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants