* Re: [net-next,PATCH v5 3/3] net: phy: realtek: Add property to enable SSC
From: Aleksander Jan Bajkowski @ 2026-04-05 20:23 UTC (permalink / raw)
To: Marek Vasut, netdev
Cc: David S. Miller, Andrew Lunn, Conor Dooley, Eric Dumazet,
Florian Fainelli, Heiner Kallweit, Ivan Galkin, Jakub Kicinski,
Krzysztof Kozlowski, Michael Klein, Paolo Abeni, Rob Herring,
Russell King, Vladimir Oltean, devicetree
In-Reply-To: <20260326210704.58912-3-marek.vasut@mailbox.org>
Hi Marek,
On 26/03/2026 22:06, Marek Vasut wrote:
> Add support for spread spectrum clocking (SSC) on RTL8211F(D)(I)-CG,
> RTL8211FS(I)(-VS)-CG, RTL8211FG(I)(-VS)-CG PHYs. The implementation
> follows EMI improvement application note Rev. 1.2 for these PHYs.
>
> The current implementation enables SSC for both RXC and SYSCLK clock
> signals. Introduce DT properties 'realtek,clkout-ssc-enable',
> 'realtek,rxc-ssc-enable' and 'realtek,sysclk-ssc-enable' which control
> CLKOUT, RXC and SYSCLK SSC spread spectrum clocking enablement on these
> signals.
>
> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
> ---
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Aleksander Jan Bajkowski <olek2@wp.pl>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Ivan Galkin <ivan.galkin@axis.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Michael Klein <michael@fossekall.de>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Cc: devicetree@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
> V2: Split SSC clock control for each CLKOUT, RXC, SYSCLK signal
> V3: Update RTL8211FVD PHYCR2 comment to state this PHY has PHYCR2 register,
> but SSC configuration is not supported due to different layout.
> V4: - Perform all SSC configuration before disabling CLKOUT
> - Perform all SSC configuration in the same order as in the SSC appnote
> - Rebase on current next, retest using spectrum analyzer again
> V5: s@SCC@SSC@ typo
> ---
> drivers/net/phy/realtek/realtek_main.c | 131 +++++++++++++++++++++++++
> 1 file changed, 131 insertions(+)
>
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index 023e47ad605bd..0b5d35841fdd4 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -75,10 +75,18 @@
>
> #define RTL8211F_PHYCR2 0x19
> #define RTL8211F_CLKOUT_EN BIT(0)
> +#define RTL8211F_SYSCLK_SSC_EN BIT(3)
> #define RTL8211F_PHYCR2_PHY_EEE_ENABLE BIT(5)
> +#define RTL8211F_CLKOUT_SSC_EN BIT(7)
>
> #define RTL8211F_INSR 0x1d
>
> +/* RTL8211F SSC settings */
> +#define RTL8211F_SSC_PAGE 0xc44
> +#define RTL8211F_SSC_RXC 0x13
> +#define RTL8211F_SSC_SYSCLK 0x17
> +#define RTL8211F_SSC_CLKOUT 0x19
> +
> /* RTL8211F LED configuration */
> #define RTL8211F_LEDCR_PAGE 0xd04
> #define RTL8211F_LEDCR 0x10
> @@ -215,6 +223,9 @@ MODULE_LICENSE("GPL");
> struct rtl821x_priv {
> bool enable_aldps;
> bool disable_clk_out;
> + bool enable_clkout_ssc;
> + bool enable_rxc_ssc;
> + bool enable_sysclk_ssc;
> struct clk *clk;
> /* rtl8211f */
> u16 iner;
> @@ -278,6 +289,12 @@ static int rtl821x_probe(struct phy_device *phydev)
> "realtek,aldps-enable");
> priv->disable_clk_out = of_property_read_bool(dev->of_node,
> "realtek,clkout-disable");
> + priv->enable_clkout_ssc = of_property_read_bool(dev->of_node,
> + "realtek,clkout-ssc-enable");
> + priv->enable_rxc_ssc = of_property_read_bool(dev->of_node,
> + "realtek,rxc-ssc-enable");
> + priv->enable_sysclk_ssc = of_property_read_bool(dev->of_node,
> + "realtek,sysclk-ssc-enable");
>
> phydev->priv = priv;
>
> @@ -707,6 +724,108 @@ static int rtl8211f_config_phy_eee(struct phy_device *phydev)
> RTL8211F_PHYCR2_PHY_EEE_ENABLE, 0);
> }
>
> +static int rtl8211f_config_clkout_ssc(struct phy_device *phydev)
> +{
> + struct rtl821x_priv *priv = phydev->priv;
> + struct device *dev = &phydev->mdio.dev;
> + int ret;
> +
> + /* The value is preserved if the device tree property is absent */
> + if (!priv->enable_clkout_ssc)
> + return 0;
> +
> + /* RTL8211FVD has PHYCR2 register, but configuration of CLKOUT SSC
> + * is not currently supported by this driver due to different bit
> + * layout.
> + */
> + if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
> + return 0;
> +
> + /* Unnamed registers from EMI improvement parameters application note 1.2 */
> + ret = phy_write_paged(phydev, 0xd09, 0x10, 0xcf00);
> + if (ret < 0) {
> + dev_err(dev, "CLKOUT SSC initialization failed: %pe\n", ERR_PTR(ret));
> + return ret;
> + }
> +
> + ret = phy_write(phydev, RTL8211F_SSC_CLKOUT, 0x38c3);
Only registers 0x10–0x17 require paged operations. The remaining registers
are mapped directly into the PHY address space. This is mentioned in commit
650e55f224a575cdb18c984b95036109519502d1. Paged and direct access return
the same results. With this in mind, I believe that RTL8211F_SSC_CLKOUT is
an alias for RTL8211F_PHYCR2 and is described on page 45 of the
datasheet[1].
1. RTL8211F(I)-CG/RTL8211FD(I)-CG Datasheet
Best Regards,
Aleksander
^ permalink raw reply
* [PATCH net-next v4 0/3] net: bridge: add stp_mode attribute for STP mode selection
From: Andy Roulin @ 2026-04-05 20:52 UTC (permalink / raw)
To: netdev
Cc: bridge, Nikolay Aleksandrov, Ido Schimmel, Andrew Lunn,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Jonathan Corbet, Shuah Khan, Petr Machata,
Donald Hunter, Jonas Gorski, linux-doc, linux-kselftest,
linux-kernel, Andy Roulin
The bridge-stp usermode helper is currently restricted to the initial
network namespace, preventing userspace STP daemons like mstpd from
operating on bridges in other namespaces. Since commit ff62198553e4
("bridge: Only call /sbin/bridge-stp for the initial network
namespace"), bridges in non-init namespaces silently fall back to
kernel STP with no way to request userspace STP.
This series adds a new IFLA_BR_STP_MODE bridge attribute that allows
explicit per-bridge control over STP mode selection. Three modes are
supported:
- auto (default): existing behavior, try /sbin/bridge-stp in
init_net, fall back to kernel STP otherwise
- user: directly enable BR_USER_STP without invoking the helper,
works in any network namespace
- kernel: directly enable BR_KERNEL_STP without invoking the helper
The user and kernel modes bypass call_usermodehelper() entirely,
addressing the security concerns discussed at [1]. Userspace is
responsible for ensuring an STP daemon manages the bridge, rather
than relying on the kernel to invoke /sbin/bridge-stp.
Patch 1 adds the kernel support. The mode can only be changed while
STP is disabled and is processed before IFLA_BR_STP_STATE in
br_changelink() so both can be set atomically in a single netlink
message.
Patch 2 adds documentation for the new attribute in the bridge docs.
Patch 3 adds a selftest with 9 test cases. The test requires iproute2
with IFLA_BR_STP_MODE support and can be run with virtme-ng:
vng --run arch/x86/boot/bzImage --skip-modules \
--overlay-rwdir /sbin --overlay-rwdir /tmp --overlay-rwdir /bin \
--exec 'cp /path/to/iproute2-next/ip/ip /bin/ip && \
cd tools/testing/selftests/net && \
bash bridge_stp_mode.sh'
iproute2 support can be found here [2].
[1] https://lore.kernel.org/netdev/565B7F7D.80208@nod.at/
[2] https://github.com/aroulin/iproute2-next/tree/bridge-stp-mode
v4:
Patch #1:
* Use u8 for stp_mode struct field.
* Add stp_helper_active bool to track whether the
usermode helper was invoked during start.
* Allow mode change when STP is being disabled in
the same netlink message.
Patch #3:
* Add disable+mode-change simultaneous test.
v3:
Patch #1:
* Name enum br_stp_mode for YNL codegen.
* Add enum-name to rt-link.yaml spec.
v2:
Patch #1:
* Add rt-link.yaml netlink spec update.
* Allow idempotent stp_mode set while STP is active.
* Move stp_mode next to root_port to fill a struct
hole.
* Rephrase BR_STP_MODE_USER doc.
Patch #3:
* Fix shellcheck CI: add SC2329 suppression.
* Add idempotent stp_mode test.
Suggested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Andy Roulin <aroulin@nvidia.com>
Andy Roulin (3):
net: bridge: add stp_mode attribute for STP mode selection
docs: net: bridge: document stp_mode attribute
selftests: net: add bridge STP mode selection test
Documentation/netlink/specs/rt-link.yaml | 12 +
Documentation/networking/bridge.rst | 22 ++
include/uapi/linux/if_link.h | 39 +++
net/bridge/br_device.c | 1 +
net/bridge/br_netlink.c | 24 +-
net/bridge/br_private.h | 2 +
net/bridge/br_stp_if.c | 19 +-
tools/testing/selftests/net/Makefile | 1 +
.../testing/selftests/net/bridge_stp_mode.sh | 288 ++++++++++++++++++
9 files changed, 400 insertions(+), 8 deletions(-)
create mode 100755 tools/testing/selftests/net/bridge_stp_mode.sh
--
2.43.0
^ permalink raw reply
* [PATCH net-next v4 1/3] net: bridge: add stp_mode attribute for STP mode selection
From: Andy Roulin @ 2026-04-05 20:52 UTC (permalink / raw)
To: netdev
Cc: bridge, Nikolay Aleksandrov, Ido Schimmel, Andrew Lunn,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Jonathan Corbet, Shuah Khan, Petr Machata,
Donald Hunter, Jonas Gorski, linux-doc, linux-kselftest,
linux-kernel, Andy Roulin, Nikolay Aleksandrov
In-Reply-To: <20260405205224.3163000-1-aroulin@nvidia.com>
The bridge-stp usermode helper is currently restricted to the initial
network namespace, preventing userspace STP daemons (e.g. mstpd) from
operating on bridges in other network namespaces. Since commit
ff62198553e4 ("bridge: Only call /sbin/bridge-stp for the initial
network namespace"), bridges in non-init namespaces silently fall
back to kernel STP with no way to use userspace STP.
Add a new bridge attribute IFLA_BR_STP_MODE that allows explicit
per-bridge control over STP mode selection:
BR_STP_MODE_AUTO (default) - Existing behavior: invoke the
/sbin/bridge-stp helper in init_net only; fall back to kernel STP
if it fails or in non-init namespaces.
BR_STP_MODE_USER - Directly enable userspace STP (BR_USER_STP)
without invoking the helper. Works in any network namespace.
Userspace is responsible for ensuring an STP daemon manages the
bridge.
BR_STP_MODE_KERNEL - Directly enable kernel STP (BR_KERNEL_STP)
without invoking the helper.
The mode can only be changed while STP is disabled, or set to the
same value (-EBUSY otherwise). IFLA_BR_STP_MODE is processed before
IFLA_BR_STP_STATE in br_changelink(), so both can be set atomically
in a single netlink message. The mode can also be changed in the
same message that disables STP.
The stp_mode struct field is u8 since all possible values fit, while
NLA_U32 is used for the netlink attribute since it occupies the same
space in the netlink message as NLA_U8.
A new stp_helper_active boolean tracks whether the /sbin/bridge-stp
helper was invoked during br_stp_start(), so that br_stp_stop() only
calls the helper for stop when it was called for start. This avoids
calling the helper asymmetrically when stp_mode changes between
start and stop.
Suggested-by: Ido Schimmel <idosch@nvidia.com>
Assisted-by: Claude:claude-opus-4-6
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Andy Roulin <aroulin@nvidia.com>
---
Notes:
v2:
* Add rt-link.yaml netlink spec update.
* Allow idempotent stp_mode set while STP is active.
* Move stp_mode next to root_port to fill a struct hole.
* Rephrase BR_STP_MODE_USER doc.
v3:
* Name enum br_stp_mode for YNL codegen.
* Add enum-name to rt-link.yaml spec.
v4:
* Use u8 for stp_mode struct field.
* Add stp_helper_active bool to track whether the
usermode helper was invoked during start.
* Allow mode change when STP is being disabled in the
same netlink message.
Documentation/netlink/specs/rt-link.yaml | 12 ++++++++
include/uapi/linux/if_link.h | 39 ++++++++++++++++++++++++
net/bridge/br_device.c | 1 +
net/bridge/br_netlink.c | 24 ++++++++++++++-
net/bridge/br_private.h | 2 ++
net/bridge/br_stp_if.c | 19 +++++++-----
6 files changed, 89 insertions(+), 8 deletions(-)
diff --git a/Documentation/netlink/specs/rt-link.yaml b/Documentation/netlink/specs/rt-link.yaml
index df4b56beb8187..495836a569e8f 100644
--- a/Documentation/netlink/specs/rt-link.yaml
+++ b/Documentation/netlink/specs/rt-link.yaml
@@ -833,6 +833,14 @@ definitions:
entries:
- p2p
- mp
+ -
+ name: br-stp-mode
+ type: enum
+ enum-name: br-stp-mode
+ entries:
+ - auto
+ - user
+ - kernel
attribute-sets:
-
@@ -1543,6 +1551,10 @@ attribute-sets:
-
name: fdb-max-learned
type: u32
+ -
+ name: stp-mode
+ type: u32
+ enum: br-stp-mode
-
name: linkinfo-brport-attrs
name-prefix: ifla-brport-
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 83a96c56b8cad..f159be39a6e78 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -744,6 +744,11 @@ enum in6_addr_gen_mode {
* @IFLA_BR_FDB_MAX_LEARNED
* Set the number of max dynamically learned FDB entries for the current
* bridge.
+ *
+ * @IFLA_BR_STP_MODE
+ * Set the STP mode for the bridge, which controls how the bridge
+ * selects between userspace and kernel STP. The valid values are
+ * documented below in the ``BR_STP_MODE_*`` constants.
*/
enum {
IFLA_BR_UNSPEC,
@@ -796,11 +801,45 @@ enum {
IFLA_BR_MCAST_QUERIER_STATE,
IFLA_BR_FDB_N_LEARNED,
IFLA_BR_FDB_MAX_LEARNED,
+ IFLA_BR_STP_MODE,
__IFLA_BR_MAX,
};
#define IFLA_BR_MAX (__IFLA_BR_MAX - 1)
+/**
+ * DOC: Bridge STP mode values
+ *
+ * @BR_STP_MODE_AUTO
+ * Default. The kernel invokes the ``/sbin/bridge-stp`` helper to hand
+ * the bridge to a userspace STP daemon (e.g. mstpd). Only attempted in
+ * the initial network namespace; in other namespaces this falls back to
+ * kernel STP.
+ *
+ * @BR_STP_MODE_USER
+ * Directly enable userspace STP (``BR_USER_STP``) without invoking the
+ * ``/sbin/bridge-stp`` helper. Works in any network namespace.
+ * Userspace is responsible for ensuring an STP daemon manages the
+ * bridge.
+ *
+ * @BR_STP_MODE_KERNEL
+ * Directly enable kernel STP (``BR_KERNEL_STP``) without invoking the
+ * helper.
+ *
+ * The mode controls how the bridge selects between userspace and kernel
+ * STP when STP is enabled via ``IFLA_BR_STP_STATE``. It can only be
+ * changed while STP is disabled (``IFLA_BR_STP_STATE`` == 0), returns
+ * ``-EBUSY`` otherwise. The default value is ``BR_STP_MODE_AUTO``.
+ */
+enum br_stp_mode {
+ BR_STP_MODE_AUTO,
+ BR_STP_MODE_USER,
+ BR_STP_MODE_KERNEL,
+ __BR_STP_MODE_MAX
+};
+
+#define BR_STP_MODE_MAX (__BR_STP_MODE_MAX - 1)
+
struct ifla_bridge_id {
__u8 prio[2];
__u8 addr[6]; /* ETH_ALEN */
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index f7502e62dd357..a35ceae0a6f2c 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -518,6 +518,7 @@ void br_dev_setup(struct net_device *dev)
ether_addr_copy(br->group_addr, eth_stp_addr);
br->stp_enabled = BR_NO_STP;
+ br->stp_mode = BR_STP_MODE_AUTO;
br->group_fwd_mask = BR_GROUPFWD_DEFAULT;
br->group_fwd_mask_required = BR_GROUPFWD_DEFAULT;
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 0264730938f4b..6fd5386a1d646 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1270,6 +1270,9 @@ static const struct nla_policy br_policy[IFLA_BR_MAX + 1] = {
NLA_POLICY_EXACT_LEN(sizeof(struct br_boolopt_multi)),
[IFLA_BR_FDB_N_LEARNED] = { .type = NLA_REJECT },
[IFLA_BR_FDB_MAX_LEARNED] = { .type = NLA_U32 },
+ [IFLA_BR_STP_MODE] = NLA_POLICY_RANGE(NLA_U32,
+ BR_STP_MODE_AUTO,
+ BR_STP_MODE_MAX),
};
static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
@@ -1306,6 +1309,23 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
return err;
}
+ if (data[IFLA_BR_STP_MODE]) {
+ u32 mode = nla_get_u32(data[IFLA_BR_STP_MODE]);
+
+ if (mode != br->stp_mode) {
+ bool stp_off = br->stp_enabled == BR_NO_STP ||
+ (data[IFLA_BR_STP_STATE] &&
+ !nla_get_u32(data[IFLA_BR_STP_STATE]));
+
+ if (!stp_off) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Can't change STP mode while STP is enabled");
+ return -EBUSY;
+ }
+ }
+ br->stp_mode = mode;
+ }
+
if (data[IFLA_BR_STP_STATE]) {
u32 stp_enabled = nla_get_u32(data[IFLA_BR_STP_STATE]);
@@ -1634,6 +1654,7 @@ static size_t br_get_size(const struct net_device *brdev)
nla_total_size(sizeof(u8)) + /* IFLA_BR_NF_CALL_ARPTABLES */
#endif
nla_total_size(sizeof(struct br_boolopt_multi)) + /* IFLA_BR_MULTI_BOOLOPT */
+ nla_total_size(sizeof(u32)) + /* IFLA_BR_STP_MODE */
0;
}
@@ -1686,7 +1707,8 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
nla_put(skb, IFLA_BR_MULTI_BOOLOPT, sizeof(bm), &bm) ||
nla_put_u32(skb, IFLA_BR_FDB_N_LEARNED,
atomic_read(&br->fdb_n_learned)) ||
- nla_put_u32(skb, IFLA_BR_FDB_MAX_LEARNED, br->fdb_max_learned))
+ nla_put_u32(skb, IFLA_BR_FDB_MAX_LEARNED, br->fdb_max_learned) ||
+ nla_put_u32(skb, IFLA_BR_STP_MODE, br->stp_mode))
return -EMSGSIZE;
#ifdef CONFIG_BRIDGE_VLAN_FILTERING
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6dbca845e625d..361a9b84451ec 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -523,6 +523,8 @@ struct net_bridge {
unsigned char topology_change;
unsigned char topology_change_detected;
u16 root_port;
+ u8 stp_mode;
+ bool stp_helper_active;
unsigned long max_age;
unsigned long hello_time;
unsigned long forward_delay;
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index cc4b27ff1b088..28c1d3f7e22f6 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -149,7 +149,9 @@ static void br_stp_start(struct net_bridge *br)
{
int err = -ENOENT;
- if (net_eq(dev_net(br->dev), &init_net))
+ /* AUTO mode: try bridge-stp helper in init_net only */
+ if (br->stp_mode == BR_STP_MODE_AUTO &&
+ net_eq(dev_net(br->dev), &init_net))
err = br_stp_call_user(br, "start");
if (err && err != -ENOENT)
@@ -162,8 +164,9 @@ static void br_stp_start(struct net_bridge *br)
else if (br->bridge_forward_delay > BR_MAX_FORWARD_DELAY)
__br_set_forward_delay(br, BR_MAX_FORWARD_DELAY);
- if (!err) {
+ if (br->stp_mode == BR_STP_MODE_USER || !err) {
br->stp_enabled = BR_USER_STP;
+ br->stp_helper_active = !err;
br_debug(br, "userspace STP started\n");
} else {
br->stp_enabled = BR_KERNEL_STP;
@@ -180,12 +183,14 @@ static void br_stp_start(struct net_bridge *br)
static void br_stp_stop(struct net_bridge *br)
{
- int err;
-
if (br->stp_enabled == BR_USER_STP) {
- err = br_stp_call_user(br, "stop");
- if (err)
- br_err(br, "failed to stop userspace STP (%d)\n", err);
+ if (br->stp_helper_active) {
+ int err = br_stp_call_user(br, "stop");
+
+ if (err)
+ br_err(br, "failed to stop userspace STP (%d)\n", err);
+ br->stp_helper_active = false;
+ }
/* To start timers on any ports left in blocking */
spin_lock_bh(&br->lock);
--
2.43.0
^ permalink raw reply related
* [PATCH net-next v4 2/3] docs: net: bridge: document stp_mode attribute
From: Andy Roulin @ 2026-04-05 20:52 UTC (permalink / raw)
To: netdev
Cc: bridge, Nikolay Aleksandrov, Ido Schimmel, Andrew Lunn,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Jonathan Corbet, Shuah Khan, Petr Machata,
Donald Hunter, Jonas Gorski, linux-doc, linux-kselftest,
linux-kernel, Andy Roulin, Nikolay Aleksandrov
In-Reply-To: <20260405205224.3163000-1-aroulin@nvidia.com>
Add documentation for the IFLA_BR_STP_MODE bridge attribute in the
"User space STP helper" section of the bridge documentation. Reference
the BR_STP_MODE_* values via kernel-doc and describe the use case for
network namespace environments.
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Andy Roulin <aroulin@nvidia.com>
---
Documentation/networking/bridge.rst | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/Documentation/networking/bridge.rst b/Documentation/networking/bridge.rst
index ef8b73e157b26..c1e6ea52c9e59 100644
--- a/Documentation/networking/bridge.rst
+++ b/Documentation/networking/bridge.rst
@@ -148,6 +148,28 @@ called by the kernel when STP is enabled/disabled on a bridge
stp_state <0|1>``). The kernel enables user_stp mode if that command returns
0, or enables kernel_stp mode if that command returns any other value.
+STP mode selection
+------------------
+
+The ``IFLA_BR_STP_MODE`` bridge attribute allows explicit control over how
+STP operates when enabled, bypassing the ``/sbin/bridge-stp`` helper
+entirely for the ``user`` and ``kernel`` modes.
+
+.. kernel-doc:: include/uapi/linux/if_link.h
+ :doc: Bridge STP mode values
+
+The default mode is ``BR_STP_MODE_AUTO``, which preserves the traditional
+behavior of invoking the ``/sbin/bridge-stp`` helper. The ``user`` and
+``kernel`` modes are particularly useful in network namespace environments
+where the helper mechanism is not available, as ``call_usermodehelper()``
+is restricted to the initial network namespace.
+
+Example::
+
+ ip link set dev br0 type bridge stp_mode user stp_state 1
+
+The mode can only be changed while STP is disabled.
+
VLAN
====
--
2.43.0
^ permalink raw reply related
* [PATCH net-next v4 3/3] selftests: net: add bridge STP mode selection test
From: Andy Roulin @ 2026-04-05 20:52 UTC (permalink / raw)
To: netdev
Cc: bridge, Nikolay Aleksandrov, Ido Schimmel, Andrew Lunn,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Jonathan Corbet, Shuah Khan, Petr Machata,
Donald Hunter, Jonas Gorski, linux-doc, linux-kselftest,
linux-kernel, Andy Roulin, Nikolay Aleksandrov
In-Reply-To: <20260405205224.3163000-1-aroulin@nvidia.com>
Add a selftest for the IFLA_BR_STP_MODE bridge attribute that verifies:
1. stp_mode defaults to auto on new bridges
2. stp_mode can be toggled between user, kernel, and auto
3. Changing stp_mode while STP is active is rejected with -EBUSY
4. Re-setting the same stp_mode while STP is active succeeds
5. stp_mode user in a network namespace yields userspace STP (stp_state=2)
6. stp_mode kernel forces kernel STP (stp_state=1)
7. stp_mode auto in a netns preserves traditional fallback to kernel STP
8. stp_mode and stp_state can be set atomically in a single message
9. stp_mode persists across STP disable/enable cycles
Test 5 is the key use case: it demonstrates that userspace STP can now
be enabled in non-init network namespaces by setting stp_mode to user
before enabling STP.
Test 8 verifies the atomic usage pattern where both attributes are set
in a single netlink message, which is supported because br_changelink()
processes IFLA_BR_STP_MODE before IFLA_BR_STP_STATE.
The test gracefully skips if the installed iproute2 does not support
the stp_mode attribute.
Assisted-by: Claude:claude-opus-4-6
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Andy Roulin <aroulin@nvidia.com>
---
Notes:
v2:
* Fix shellcheck CI: add SC2329 suppression.
* Add idempotent stp_mode test.
v4:
* Add disable+mode-change simultaneous test.
tools/testing/selftests/net/Makefile | 1 +
.../testing/selftests/net/bridge_stp_mode.sh | 288 ++++++++++++++++++
2 files changed, 289 insertions(+)
create mode 100755 tools/testing/selftests/net/bridge_stp_mode.sh
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 6bced3ed798b0..053c7b83c76dd 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -15,6 +15,7 @@ TEST_PROGS := \
big_tcp.sh \
bind_bhash.sh \
bpf_offload.py \
+ bridge_stp_mode.sh \
bridge_vlan_dump.sh \
broadcast_ether_dst.sh \
broadcast_pmtu.sh \
diff --git a/tools/testing/selftests/net/bridge_stp_mode.sh b/tools/testing/selftests/net/bridge_stp_mode.sh
new file mode 100755
index 0000000000000..0c81fd029d794
--- /dev/null
+++ b/tools/testing/selftests/net/bridge_stp_mode.sh
@@ -0,0 +1,288 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# shellcheck disable=SC2034,SC2154,SC2317,SC2329
+#
+# Test for bridge STP mode selection (IFLA_BR_STP_MODE).
+#
+# Verifies that:
+# - stp_mode defaults to auto on new bridges
+# - stp_mode can be toggled between user, kernel, and auto
+# - stp_mode change is rejected while STP is active (-EBUSY)
+# - stp_mode user in a netns yields userspace STP (stp_state=2)
+# - stp_mode kernel forces kernel STP (stp_state=1)
+# - stp_mode auto preserves traditional fallback to kernel STP
+# - stp_mode and stp_state can be set atomically in one message
+# - stp_mode persists across STP disable/enable cycles
+
+source lib.sh
+
+require_command jq
+
+ALL_TESTS="
+ test_default_auto
+ test_set_modes
+ test_reject_change_while_stp_active
+ test_idempotent_mode_while_stp_active
+ test_user_mode_in_netns
+ test_kernel_mode
+ test_auto_mode
+ test_atomic_mode_and_state
+ test_mode_persistence
+"
+
+bridge_info_get()
+{
+ ip -n "$NS1" -d -j link show "$1" | \
+ jq -r ".[0].linkinfo.info_data.$2"
+}
+
+check_stp_mode()
+{
+ local br=$1; shift
+ local expected=$1; shift
+ local msg=$1; shift
+ local val
+
+ val=$(bridge_info_get "$br" stp_mode)
+ [ "$val" = "$expected" ]
+ check_err $? "$msg: expected $expected, got $val"
+}
+
+check_stp_state()
+{
+ local br=$1; shift
+ local expected=$1; shift
+ local msg=$1; shift
+ local val
+
+ val=$(bridge_info_get "$br" stp_state)
+ [ "$val" = "$expected" ]
+ check_err $? "$msg: expected $expected, got $val"
+}
+
+# Create a bridge in NS1, bring it up, and defer its deletion.
+bridge_create()
+{
+ ip -n "$NS1" link add "$1" type bridge
+ ip -n "$NS1" link set "$1" up
+ defer ip -n "$NS1" link del "$1"
+}
+
+setup_prepare()
+{
+ setup_ns NS1
+}
+
+cleanup()
+{
+ defer_scopes_cleanup
+ cleanup_all_ns
+}
+
+# Check that stp_mode defaults to auto when creating a bridge.
+test_default_auto()
+{
+ RET=0
+
+ ip -n "$NS1" link add br-test type bridge
+ defer ip -n "$NS1" link del br-test
+
+ check_stp_mode br-test auto "stp_mode default"
+
+ log_test "stp_mode defaults to auto"
+}
+
+# Test setting stp_mode to user, kernel, and back to auto.
+test_set_modes()
+{
+ RET=0
+
+ ip -n "$NS1" link add br-test type bridge
+ defer ip -n "$NS1" link del br-test
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode user
+ check_err $? "Failed to set stp_mode to user"
+ check_stp_mode br-test user "after set user"
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode kernel
+ check_err $? "Failed to set stp_mode to kernel"
+ check_stp_mode br-test kernel "after set kernel"
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode auto
+ check_err $? "Failed to set stp_mode to auto"
+ check_stp_mode br-test auto "after set auto"
+
+ log_test "stp_mode set user/kernel/auto"
+}
+
+# Verify that stp_mode cannot be changed while STP is active.
+test_reject_change_while_stp_active()
+{
+ RET=0
+
+ bridge_create br-test
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode kernel
+ check_err $? "Failed to set stp_mode to kernel"
+
+ ip -n "$NS1" link set dev br-test type bridge stp_state 1
+ check_err $? "Failed to enable STP"
+
+ # Changing stp_mode while STP is active should fail.
+ ip -n "$NS1" link set dev br-test type bridge stp_mode auto 2>/dev/null
+ check_fail $? "Changing stp_mode should fail while STP is active"
+
+ check_stp_mode br-test kernel "mode unchanged after rejected change"
+
+ # Disable STP, then change should succeed.
+ ip -n "$NS1" link set dev br-test type bridge stp_state 0
+ check_err $? "Failed to disable STP"
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode auto
+ check_err $? "Changing stp_mode should succeed after STP is disabled"
+
+ log_test "reject stp_mode change while STP is active"
+}
+
+# Verify that re-setting the same stp_mode while STP is active succeeds.
+test_idempotent_mode_while_stp_active()
+{
+ RET=0
+
+ bridge_create br-test
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode user stp_state 1
+ check_err $? "Failed to enable STP with user mode"
+
+ # Re-setting the same mode while STP is active should succeed.
+ ip -n "$NS1" link set dev br-test type bridge stp_mode user
+ check_err $? "Idempotent stp_mode set should succeed while STP is active"
+
+ check_stp_state br-test 2 "stp_state after idempotent set"
+
+ # Changing mode while disabling STP in the same message should succeed.
+ ip -n "$NS1" link set dev br-test type bridge stp_mode auto stp_state 0
+ check_err $? "Mode change with simultaneous STP disable should succeed"
+
+ check_stp_mode br-test auto "mode changed after disable+change"
+ check_stp_state br-test 0 "stp_state after disable+change"
+
+ log_test "idempotent and simultaneous mode change while STP active"
+}
+
+# Test that stp_mode user in a non-init netns yields userspace STP
+# (stp_state == 2). This is the key use case: userspace STP without
+# needing /sbin/bridge-stp or being in init_net.
+test_user_mode_in_netns()
+{
+ RET=0
+
+ bridge_create br-test
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode user
+ check_err $? "Failed to set stp_mode to user"
+
+ ip -n "$NS1" link set dev br-test type bridge stp_state 1
+ check_err $? "Failed to enable STP"
+
+ check_stp_state br-test 2 "stp_state with user mode"
+
+ log_test "stp_mode user in netns yields userspace STP"
+}
+
+# Test that stp_mode kernel forces kernel STP (stp_state == 1)
+# regardless of whether /sbin/bridge-stp exists.
+test_kernel_mode()
+{
+ RET=0
+
+ bridge_create br-test
+
+ ip -n "$NS1" link set dev br-test type bridge stp_mode kernel
+ check_err $? "Failed to set stp_mode to kernel"
+
+ ip -n "$NS1" link set dev br-test type bridge stp_state 1
+ check_err $? "Failed to enable STP"
+
+ check_stp_state br-test 1 "stp_state with kernel mode"
+
+ log_test "stp_mode kernel forces kernel STP"
+}
+
+# Test that stp_mode auto preserves traditional behavior: in a netns
+# (non-init_net), bridge-stp is not called and STP falls back to
+# kernel mode (stp_state == 1).
+test_auto_mode()
+{
+ RET=0
+
+ bridge_create br-test
+
+ # Auto mode is the default; enable STP in a netns.
+ ip -n "$NS1" link set dev br-test type bridge stp_state 1
+ check_err $? "Failed to enable STP"
+
+ # In a netns with auto mode, bridge-stp is skipped (init_net only),
+ # so STP should fall back to kernel mode (stp_state == 1).
+ check_stp_state br-test 1 "stp_state with auto mode in netns"
+
+ log_test "stp_mode auto preserves traditional behavior"
+}
+
+# Test that stp_mode and stp_state can be set in a single netlink
+# message. This is the intended atomic usage pattern.
+test_atomic_mode_and_state()
+{
+ RET=0
+
+ bridge_create br-test
+
+ # Set both stp_mode and stp_state in one command.
+ ip -n "$NS1" link set dev br-test type bridge stp_mode user stp_state 1
+ check_err $? "Failed to set stp_mode user and stp_state 1 atomically"
+
+ check_stp_state br-test 2 "stp_state after atomic set"
+
+ log_test "atomic stp_mode user + stp_state 1 in single message"
+}
+
+# Test that stp_mode persists across STP disable/enable cycles.
+test_mode_persistence()
+{
+ RET=0
+
+ bridge_create br-test
+
+ # Set user mode and enable STP.
+ ip -n "$NS1" link set dev br-test type bridge stp_mode user
+ ip -n "$NS1" link set dev br-test type bridge stp_state 1
+ check_err $? "Failed to enable STP with user mode"
+
+ # Disable STP.
+ ip -n "$NS1" link set dev br-test type bridge stp_state 0
+ check_err $? "Failed to disable STP"
+
+ # Verify mode is still user.
+ check_stp_mode br-test user "stp_mode after STP disable"
+
+ # Re-enable STP -- should use user mode again.
+ ip -n "$NS1" link set dev br-test type bridge stp_state 1
+ check_err $? "Failed to re-enable STP"
+
+ check_stp_state br-test 2 "stp_state after re-enable"
+
+ log_test "stp_mode persists across STP disable/enable cycles"
+}
+
+# Check iproute2 support before setting up resources.
+if ! ip link add type bridge help 2>&1 | grep -q "stp_mode"; then
+ echo "SKIP: iproute2 too old, missing stp_mode support"
+ exit "$ksft_skip"
+fi
+
+trap cleanup EXIT
+
+setup_prepare
+tests_run
+
+exit "$EXIT_STATUS"
--
2.43.0
^ permalink raw reply related
* Re: [PATCH net-next v2 8/9] net: dsa: lan9645x: add mdb management
From: Jens Emil Schulz Ostergaard @ 2026-04-05 21:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: UNGLinuxDriver, andrew, olteanv, davem, edumazet, pabeni, horms,
robh, krzk+dt, conor+dt, woojung.huh, linux, Steen.Hegelund,
daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260329195637.2789256-1-kuba@kernel.org>
On Sun, 2026-03-29 at 12:56 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> Subject: net: dsa: lan9645x: add mdb management
>
> Add support for dsa mdb callbacks. L2 multicast and IP multicast is
> handled differently. IP multicast stores the port group mask inline in
> the mac table. L2 multicast points to a PGID index, which encodes the
> port group mask.
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > new file mode 100644
> > index 000000000000..4cf92887a4eb
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > @@ -0,0 +1,391 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (C) 2026 Microchip Technology Inc.
> > + */
> > +
> > +#include "lan9645x_main.h"
>
> [ ... ]
>
> > +static void lan9645x_mdb_encode_mac(unsigned char *mac,
> > + struct lan9645x_mdb_entry *mdb_entry,
> > + enum macaccess_entry_type type)
> > +{
> > + ether_addr_copy(mac, mdb_entry->mac);
> > +
> > + /* The HW encodes the portmask in the high bits of the mac for ip
> > + * multicast entries, to save on the limited PGID resources.
> > + *
> > + * IPv4 Multicast DMAC: 0x01005Exxxxxx
> > + * IPv6 Multicast DMAC: 0x3333xxxxxxxx
> > + *
> > + * which gives us 24 or 16 bits to encode the portmask.
> > + */
> > + if (type == ENTRYTYPE_MACV4) {
> > + mac[0] = 0;
> > + mac[1] = mdb_entry->ports >> 8;
> > + mac[2] = mdb_entry->ports & 0xff;
> > + } else if (type == ENTRYTYPE_MACV6) {
> > + mac[0] = mdb_entry->ports >> 8;
> > + mac[1] = mdb_entry->ports & 0xff;
> > + }
> > +}
>
> Does this code need to mask out BIT(CPU_PORT) from mdb_entry->ports?
>
> For L2 multicast, the CPU port is explicitly stripped from the PGID portmask
> to avoid conflicting instructions, since CPU_PORT forwarding is handled by the
> cpu_copy flag. Here, the IP multicast portmask is encoded directly into the
> MAC address using the full mdb_entry->ports. Could this cause hardware to
> receive conflicting instructions (forwarding to port 9 and copying to the CPU
> extraction queue), leading to duplicate multicast packets for the CPU?
>
No this is fine.
> [ ... ]
>
> > +static struct lan9645x_pgid_entry *
> > +lan9645x_mdb_pgid_entry_create(struct lan9645x *lan9645x, u16 ports)
> > +{
> > + struct lan9645x_pgid_entry *pgid_entry = NULL;
> > + int index;
> > +
> > + for (index = PGID_GP_START; index < PGID_GP_END; index++) {
>
> Is it safe to start the search at PGID_GP_START (defined as CPU_PORT)?
>
> Hardware uses ANA_PGID(DEST_IDX) to forward unicast traffic, and MAC table
> entries for the CPU have DEST_IDX = 9. When the first L2 multicast group is
> added, it appears this loop will allocate PGID index 9, overwriting the
> unicast portmask for the CPU port. Could this cause all unicast traffic
> destined for the CPU to be flooded to the external physical ports in the
> multicast group instead?
>
No this is safe, PGID 9 is not used to forward anything to the CPU.
> > + bool used = false;
> > +
> > + list_for_each_entry(pgid_entry, &lan9645x->pgid_entries, list) {
> > + if (pgid_entry->index == index) {
> > + used = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!used)
> > + return lan9645x_pgid_entry_alloc(lan9645x, index,
> > + ports);
> > + }
> > +
> > + return ERR_PTR(-ENOSPC);
> > +}
>
> [ ... ]
>
> > +static int __lan9645x_mdb_add(struct lan9645x *lan9645x, int chip_port,
> > + const unsigned char addr[ETH_ALEN], u16 vid,
> > + enum macaccess_entry_type type)
> > +{
> > + struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> > + struct lan9645x_mdb_entry *mdb_entry;
> > + unsigned char mac[ETH_ALEN];
> > + int err, pgid_index;
> > + bool cpu_copy;
> > +
> > + mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> > + if (!mdb_entry) {
> > + mdb_entry = lan9645x_mdb_entry_alloc(lan9645x, addr, vid);
> > + if (IS_ERR(mdb_entry))
> > + return PTR_ERR(mdb_entry);
> > + }
> > +
> > + if (mdb_entry->ports & BIT(chip_port))
> > + return 0;
> > +
> > + mdb_entry->ports |= BIT(chip_port);
> > +
> > + /* Encode mac for IP mc */
> > + lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> > +
> > + /* Update PGID ptr for non-IP entries (L2 multicast) */
> > + old_pgid = mdb_entry->pgid;
> > + new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> > + if (IS_ERR(new_pgid)) {
> > + /* Out of PGIDs or mem. Continue forwarding to old port
> > + * group, or remove if fresh mdb_entry.
> > + */
> > + mdb_entry->ports &= ~BIT(chip_port);
> > + if (!mdb_entry->ports)
> > + lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> > +
> > + return PTR_ERR(new_pgid);
> > + }
> > + mdb_entry->pgid = new_pgid;
> > +
> > + cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> > + pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> > +
> > + /* Make sure to write on top of existing entry, so we do not disrupt
> > + * flowing traffic.
> > + */
> > + err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> > + mdb_entry->vid, type, cpu_copy);
> > + lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > + return err;
> > +}
>
> What happens if lan9645x_mact_learn_cpu_copy() fails and returns an error?
>
> The software MDB state (mdb_entry->ports and mdb_entry->pgid) is updated and
> the old PGID reference is dropped via lan9645x_pgid_entry_put() before
> verifying if the hardware MAC table update succeeds.
>
> If the hardware update fails, the software state isn't rolled back, leaving
> them out of sync. Furthermore, the dropped old_pgid reference could allow the
> index to be reallocated to a new multicast group while the hardware still
> points to the old PGID index, potentially causing silent traffic cross-talk.
>
I will change the mac table write error path, so hw/sw stay synced.
> > +
> > +static int __lan9645x_mdb_del(struct lan9645x *lan9645x, int chip_port,
> > + const unsigned char addr[ETH_ALEN], u16 vid,
> > + enum macaccess_entry_type type)
> > +{
> > + struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> > + struct lan9645x_mdb_entry *mdb_entry;
> > + unsigned char mac[ETH_ALEN];
> > + int err, pgid_index;
> > + bool cpu_copy;
> > +
> > + mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> > + if (!mdb_entry)
> > + return -ENOENT;
> > +
> > + if (!(mdb_entry->ports & BIT(chip_port)))
> > + return 0;
> > +
> > + mdb_entry->ports &= ~BIT(chip_port);
> > +
> > + /* Encode mac for IP mc */
> > + lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> > +
> > + /* Update PGID ptr for non-IP entries (L2 multicast) */
> > + old_pgid = mdb_entry->pgid;
> > + new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> > + if (IS_ERR(new_pgid)) {
> > + /* Continue forwarding to old port group. */
> > + mdb_entry->ports |= BIT(chip_port);
> > + return PTR_ERR(new_pgid);
> > + }
> > + mdb_entry->pgid = new_pgid;
> > +
> > + if (!mdb_entry->ports) {
> > + lan9645x_mact_forget(lan9645x, mac, mdb_entry->vid, type);
> > + lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > + lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> > + return 0;
> > + }
>
> Similar to the addition path above, if lan9645x_mact_forget() fails, its
> return value is ignored, and the software MDB entry and PGID are freed
> anyway. This could also lead to hardware/software state desynchronization.
>
> > +
> > + cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> > + pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> > +
> > + err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> > + mdb_entry->vid, type, cpu_copy);
> > + lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > + return err;
> > +}
^ permalink raw reply
* [PATCH net] selftests: net: bridge_vlan_mcast: wait for h1 before querier check
From: Daniel Golle @ 2026-04-05 21:29 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, Daniel Golle, Nikolay Aleksandrov,
netdev, linux-kselftest, linux-kernel
Cc: John Crispin, Alexander Sverdlin
The querier-interval test adds h1 (currently a slave of the VRF created
by simple_if_init) to a temporary bridge br1 acting as an outside IGMP
querier. The kernel VRF driver (drivers/net/vrf.c) calls cycle_netdev()
on every slave add and remove, toggling the interface admin-down then up.
Phylink takes the PHY down during the admin-down half of that cycle.
Since h1 and swp1 are cable-connected, swp1 also loses its link may need
several seconds to re-negotiate.
Use setup_wait_dev $h1 0 which waits for h1 to return to UP state, so the
test can rely on the link being back up at this point.
Fixes: 4d8610ee8bd77 ("selftests: net: bridge: add vlan mcast_querier_interval tests")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh b/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
index 72dfbeaf56b92..e8031f68200ad 100755
--- a/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_vlan_mcast.sh
@@ -414,6 +414,7 @@ vlmc_querier_intvl_test()
bridge vlan add vid 10 dev br1 self pvid untagged
ip link set dev $h1 master br1
ip link set dev br1 up
+ setup_wait_dev $h1 0
bridge vlan add vid 10 dev $h1 master
bridge vlan global set vid 10 dev br1 mcast_snooping 1 mcast_querier 1
sleep 2
--
2.53.0
^ permalink raw reply related
* Re: [net-next,PATCH v5 3/3] net: phy: realtek: Add property to enable SSC
From: Marek Vasut @ 2026-04-05 21:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, David S. Miller, Aleksander Jan Bajkowski, Andrew Lunn,
Conor Dooley, Eric Dumazet, Florian Fainelli, Heiner Kallweit,
Ivan Galkin, Krzysztof Kozlowski, Michael Klein, Paolo Abeni,
Rob Herring, Russell King, Vladimir Oltean, devicetree
In-Reply-To: <20260330185721.4b94ed30@kernel.org>
On 3/31/26 3:57 AM, Jakub Kicinski wrote:
> On Thu, 26 Mar 2026 22:06:35 +0100 Marek Vasut wrote:
>> +/* RTL8211F SSC settings */
>> +#define RTL8211F_SSC_PAGE 0xc44
>> +#define RTL8211F_SSC_RXC 0x13
>> +#define RTL8211F_SSC_SYSCLK 0x17
>> +#define RTL8211F_SSC_CLKOUT 0x19
>
>> + /* Unnamed registers from EMI improvement parameters application note 1.2 */
>> + ret = phy_write_paged(phydev, 0xd09, 0x10, 0xcf00);
>> + if (ret < 0) {
>> + dev_err(dev, "CLKOUT SSC initialization failed: %pe\n", ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + ret = phy_write(phydev, RTL8211F_SSC_CLKOUT, 0x38c3);
>> + if (ret < 0) {
>> + dev_err(dev, "CLKOUT SSC configuration failed: %pe\n", ERR_PTR(ret));
>> + return ret;
>> + }
>
> AI flags that this, did you mean to write to the SSC_PAGE here?
No, see commit 650e55f224a5 ("net: phy: realtek: simplify bogus paged
operations")
"
net: phy: realtek: simplify bogus paged operations
Only registers 0x10~0x17 are affected by the value in the page
selection register 0x1f. Hence there is no point in using paged
operations when accessing any other registers.
...
"
Register 0x19 is not affected by paged operations.
^ permalink raw reply
* Re: [net-next,PATCH v5 3/3] net: phy: realtek: Add property to enable SSC
From: Marek Vasut @ 2026-04-05 21:59 UTC (permalink / raw)
To: Aleksander Jan Bajkowski, netdev
Cc: David S. Miller, Andrew Lunn, Conor Dooley, Eric Dumazet,
Florian Fainelli, Heiner Kallweit, Ivan Galkin, Jakub Kicinski,
Krzysztof Kozlowski, Michael Klein, Paolo Abeni, Rob Herring,
Russell King, Vladimir Oltean, devicetree
In-Reply-To: <a03960a2-f313-4c12-98f2-d032ce4a45ad@wp.pl>
On 4/5/26 10:23 PM, Aleksander Jan Bajkowski wrote:
Hi,
>> +static int rtl8211f_config_clkout_ssc(struct phy_device *phydev)
>> +{
>> + struct rtl821x_priv *priv = phydev->priv;
>> + struct device *dev = &phydev->mdio.dev;
>> + int ret;
>> +
>> + /* The value is preserved if the device tree property is absent */
>> + if (!priv->enable_clkout_ssc)
>> + return 0;
>> +
>> + /* RTL8211FVD has PHYCR2 register, but configuration of CLKOUT SSC
>> + * is not currently supported by this driver due to different bit
>> + * layout.
>> + */
>> + if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
>> + return 0;
>> +
>> + /* Unnamed registers from EMI improvement parameters application
>> note 1.2 */
>> + ret = phy_write_paged(phydev, 0xd09, 0x10, 0xcf00);
>> + if (ret < 0) {
>> + dev_err(dev, "CLKOUT SSC initialization failed: %pe\n",
>> ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> + ret = phy_write(phydev, RTL8211F_SSC_CLKOUT, 0x38c3);
>
> Only registers 0x10–0x17 require paged operations. The remaining registers
> are mapped directly into the PHY address space. This is mentioned in commit
> 650e55f224a575cdb18c984b95036109519502d1. Paged and direct access return
> the same results. With this in mind, I believe that RTL8211F_SSC_CLKOUT is
> an alias for RTL8211F_PHYCR2 and is described on page 45 of the
> datasheet[1].
>
> 1. RTL8211F(I)-CG/RTL8211FD(I)-CG Datasheet
This is a good point indeed, I think I can simply set PHYCR2 bits
7,12,13 to enable the CLKOUT SSC ?
^ permalink raw reply
* [PATCH] net: txgbe: leave space for null terminators on property_entry
From: Fabio Baltieri @ 2026-04-05 22:20 UTC (permalink / raw)
To: Jiawen Wu, Mengyuan Lou, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman
Cc: Fabio Baltieri, netdev, linux-kernel
Lists of struct property_entry are supposed to be terminated with an
empty property, this driver currently seems to be allocating exactly the
amount of entry used.
Change the struct definition to leave an extra element for all
property_entry.
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---
Hi, bumped into this while studying the code, looks like the struct has
been allocated without space for terminators, guess the top ones just
end up using the bottom props as well but I'm surprised this does not
crash at some point. Build test only, don't have any hardware for this,
let me know if I'm missing something here.
Cheers,
Fabio
drivers/net/ethernet/wangxun/txgbe/txgbe_type.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 82433e9cb0e3..6b05f32b4a01 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -424,10 +424,10 @@ struct txgbe_nodes {
char i2c_name[32];
char sfp_name[32];
char phylink_name[32];
- struct property_entry gpio_props[1];
- struct property_entry i2c_props[3];
- struct property_entry sfp_props[8];
- struct property_entry phylink_props[2];
+ struct property_entry gpio_props[2];
+ struct property_entry i2c_props[4];
+ struct property_entry sfp_props[9];
+ struct property_entry phylink_props[3];
struct software_node_ref_args i2c_ref[1];
struct software_node_ref_args gpio0_ref[1];
struct software_node_ref_args gpio1_ref[1];
--
2.47.3
^ permalink raw reply related
* Re: [PATCH v4 0/9] driver core: Fix some race conditions
From: Doug Anderson @ 2026-04-05 22:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J . Wysocki, Danilo Krummrich, Alan Stern, Saravana Kannan,
Christoph Hellwig, Eric Dumazet, Johan Hovold, Leon Romanovsky,
Alexander Lobakin, Alexey Kardashevskiy, Robin Murphy,
Andrew Morton, Frank.Li, Jason Gunthorpe, alex, alexander.stein,
andre.przywara, andrew, andrew, andriy.shevchenko, aou, ardb,
bhelgaas, brgl, broonie, catalin.marinas, chleroy, davem, david,
devicetree, dmaengine, driver-core, gbatra, gregory.clement,
hkallweit1, iommu, jirislaby, joel, joro, kees, kevin.brodsky,
kuba, lenb, lgirdwood, linux-acpi, linux-arm-kernel, linux-aspeed,
linux-cxl, linux-kernel, linux-mips, linux-mm, linux-pci,
linux-riscv, linux-serial, linux-snps-arc, linux-usb, linux,
linuxppc-dev, m.szyprowski, maddy, mani, maz, miko.lenczewski,
mpe, netdev, npiggin, osalvador, oupton, pabeni, palmer,
peter.ujfalusi, peterz, pjw, robh, sebastian.hesselbarth, tglx,
tsbogend, vgupta, vkoul, will, willy, yangyicong, yeoreum.yun
In-Reply-To: <2026040539-sponge-publisher-2b42@gregkh>
Hi,
On Sat, Apr 4, 2026 at 10:28 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Apr 03, 2026 at 05:04:54PM -0700, Douglas Anderson wrote:
> > NOTE: one potentially "controversial" choice I made in some patches
> > was to always reserve a flag ID even if a flag is only used under
> > certain CONFIG_ settings. This is a change from how things were
> > before. Keeping the numbering consistent and allowing easy
> > compile-testing of both CONFIG settings seemed worth it, especially
> > since it won't take up any extra space until we've added a lot more
> > flags.
>
> Nah, this is fine, I don't see any problems with this as the original
> code kind of was doing the same thing with the "hole" in the structure
> if those options were not enabled.
>
> > I only marked the first patch as a "Fix" since it is the only one
> > fixing observed problems. Other patches could be considered fixes too
> > if folks want.
> >
> > I tested the first patch in the series backported to kernel 6.6 on the
> > Pixel phone that was experiencing the race. I added extra printouts to
> > make sure that the problem was hitting / addressed. The rest of the
> > patches are tested with allmodconfig with arm32, arm64, ppc, and
> > x86. I boot tested on an arm64 Chromebook running mainline.
>
> I'm guessing your tests passed? :)
Yup, all the tests that I've run have passed. I also threw in an
"allnoconfig" compile test just for good measure.
> Anyway, this looks great, unless there are any objections, other than
> the "needs to be undefined", which a follow-on patch can handle, I'll
> queue them up next week for 7.1-rc1.
Thanks. As per the other thread, I'm happy if you or Danilo want to
apply it, and I'm happy if you want to make minor fixups when
applying.
When I see the patches applied, I'll send a followup patch to address
the "needs to be undefined" comment, unless Danilo makes that change
himself when applying.
-Doug
^ permalink raw reply
* [net-next,PATCH v6 1/3] dt-bindings: net: realtek,rtl82xx: Keep property list sorted
From: Marek Vasut @ 2026-04-05 23:29 UTC (permalink / raw)
To: netdev
Cc: Marek Vasut, Rob Herring (Arm), David S. Miller,
Aleksander Jan Bajkowski, Andrew Lunn, Conor Dooley, Eric Dumazet,
Florian Fainelli, Heiner Kallweit, Ivan Galkin, Jakub Kicinski,
Krzysztof Kozlowski, Michael Klein, Paolo Abeni, Russell King,
Vladimir Oltean, devicetree
Sort the documented properties alphabetically, no functional change.
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Aleksander Jan Bajkowski <olek2@wp.pl>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Ivan Galkin <ivan.galkin@axis.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Michael Klein <michael@fossekall.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: devicetree@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: No change
V3: No change
V4: Add AB from Rob
V5: No change
V6: No change
---
.../devicetree/bindings/net/realtek,rtl82xx.yaml | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
index 2b5697bd7c5df..eafcc2f3e3d66 100644
--- a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
+++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
@@ -40,15 +40,15 @@ properties:
leds: true
- realtek,clkout-disable:
+ realtek,aldps-enable:
type: boolean
description:
- Disable CLKOUT clock, CLKOUT clock default is enabled after hardware reset.
+ Enable ALDPS mode, ALDPS mode default is disabled after hardware reset.
- realtek,aldps-enable:
+ realtek,clkout-disable:
type: boolean
description:
- Enable ALDPS mode, ALDPS mode default is disabled after hardware reset.
+ Disable CLKOUT clock, CLKOUT clock default is enabled after hardware reset.
wakeup-source:
type: boolean
--
2.53.0
^ permalink raw reply related
* [net-next,PATCH v6 2/3] dt-bindings: net: realtek,rtl82xx: Document realtek,*-ssc-enable property
From: Marek Vasut @ 2026-04-05 23:29 UTC (permalink / raw)
To: netdev
Cc: Marek Vasut, Krzysztof Kozlowski, David S. Miller,
Aleksander Jan Bajkowski, Andrew Lunn, Conor Dooley, Eric Dumazet,
Florian Fainelli, Heiner Kallweit, Ivan Galkin, Jakub Kicinski,
Krzysztof Kozlowski, Michael Klein, Paolo Abeni, Rob Herring,
Russell King, Vladimir Oltean, devicetree
In-Reply-To: <20260405233008.148974-1-marek.vasut@mailbox.org>
Document support for spread spectrum clocking (SSC) on RTL8211F(D)(I)-CG,
RTL8211FS(I)(-VS)-CG, RTL8211FG(I)(-VS)-CG PHYs. Introduce DT properties
'realtek,clkout-ssc-enable', 'realtek,rxc-ssc-enable' and
'realtek,sysclk-ssc-enable' which control CLKOUT, RXC and SYSCLK
SSC spread spectrum clocking enablement on these signals. These
clock are not exposed via the clock API, therefore assigned-clock-sscs
property does not apply.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Aleksander Jan Bajkowski <olek2@wp.pl>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Ivan Galkin <ivan.galkin@axis.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Michael Klein <michael@fossekall.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: devicetree@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: Split SSC clock control for each CLKOUT, RXC, SYSCLK signal
V3: - Add RB from krzk
- Update commit subject, use realtek,*-ssc-enable to be accurate
V4: No change
V5: No change
V6: No change
---
.../devicetree/bindings/net/realtek,rtl82xx.yaml | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
index eafcc2f3e3d66..45033c31a2d51 100644
--- a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
+++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
@@ -50,6 +50,21 @@ properties:
description:
Disable CLKOUT clock, CLKOUT clock default is enabled after hardware reset.
+ realtek,clkout-ssc-enable:
+ type: boolean
+ description:
+ Enable CLKOUT SSC mode, CLKOUT SSC mode default is disabled after hardware reset.
+
+ realtek,rxc-ssc-enable:
+ type: boolean
+ description:
+ Enable RXC SSC mode, RXC SSC mode default is disabled after hardware reset.
+
+ realtek,sysclk-ssc-enable:
+ type: boolean
+ description:
+ Enable SYSCLK SSC mode, SYSCLK SSC mode default is disabled after hardware reset.
+
wakeup-source:
type: boolean
description:
--
2.53.0
^ permalink raw reply related
* [net-next,PATCH v6 3/3] net: phy: realtek: Add property to enable SSC
From: Marek Vasut @ 2026-04-05 23:29 UTC (permalink / raw)
To: netdev
Cc: Marek Vasut, David S. Miller, Aleksander Jan Bajkowski,
Andrew Lunn, Conor Dooley, Eric Dumazet, Florian Fainelli,
Heiner Kallweit, Ivan Galkin, Jakub Kicinski, Krzysztof Kozlowski,
Michael Klein, Paolo Abeni, Rob Herring, Russell King,
Vladimir Oltean, devicetree
In-Reply-To: <20260405233008.148974-1-marek.vasut@mailbox.org>
Add support for spread spectrum clocking (SSC) on RTL8211F(D)(I)-CG,
RTL8211FS(I)(-VS)-CG, RTL8211FG(I)(-VS)-CG PHYs. The implementation
follows EMI improvement application note Rev. 1.2 for these PHYs.
The current implementation enables SSC for both RXC and SYSCLK clock
signals. Introduce DT properties 'realtek,clkout-ssc-enable',
'realtek,rxc-ssc-enable' and 'realtek,sysclk-ssc-enable' which control
CLKOUT, RXC and SYSCLK SSC spread spectrum clocking enablement on these
signals.
Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Aleksander Jan Bajkowski <olek2@wp.pl>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Ivan Galkin <ivan.galkin@axis.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Michael Klein <michael@fossekall.de>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: devicetree@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: Split SSC clock control for each CLKOUT, RXC, SYSCLK signal
V3: Update RTL8211FVD PHYCR2 comment to state this PHY has PHYCR2 register,
but SSC configuration is not supported due to different layout.
V4: - Perform all SSC configuration before disabling CLKOUT
- Perform all SSC configuration in the same order as in the SSC appnote
- Rebase on current next, retest using spectrum analyzer again
V5: s@SCC@SSC@ typo
V6: Drop RTL8211F_SSC_CLKOUT write in favor of setting PHYCR2 bit 7,12,13
---
drivers/net/phy/realtek/realtek_main.c | 127 +++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 023e47ad605bd..f31969de3ba99 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -75,10 +75,18 @@
#define RTL8211F_PHYCR2 0x19
#define RTL8211F_CLKOUT_EN BIT(0)
+#define RTL8211F_SYSCLK_SSC_EN BIT(3)
#define RTL8211F_PHYCR2_PHY_EEE_ENABLE BIT(5)
+#define RTL8211F_CLKOUT_SSC_EN BIT(7)
+#define RTL8211F_CLKOUT_SSC_CAP GENMASK(13, 12)
#define RTL8211F_INSR 0x1d
+/* RTL8211F SSC settings */
+#define RTL8211F_SSC_PAGE 0xc44
+#define RTL8211F_SSC_RXC 0x13
+#define RTL8211F_SSC_SYSCLK 0x17
+
/* RTL8211F LED configuration */
#define RTL8211F_LEDCR_PAGE 0xd04
#define RTL8211F_LEDCR 0x10
@@ -215,6 +223,9 @@ MODULE_LICENSE("GPL");
struct rtl821x_priv {
bool enable_aldps;
bool disable_clk_out;
+ bool enable_clkout_ssc;
+ bool enable_rxc_ssc;
+ bool enable_sysclk_ssc;
struct clk *clk;
/* rtl8211f */
u16 iner;
@@ -278,6 +289,12 @@ static int rtl821x_probe(struct phy_device *phydev)
"realtek,aldps-enable");
priv->disable_clk_out = of_property_read_bool(dev->of_node,
"realtek,clkout-disable");
+ priv->enable_clkout_ssc = of_property_read_bool(dev->of_node,
+ "realtek,clkout-ssc-enable");
+ priv->enable_rxc_ssc = of_property_read_bool(dev->of_node,
+ "realtek,rxc-ssc-enable");
+ priv->enable_sysclk_ssc = of_property_read_bool(dev->of_node,
+ "realtek,sysclk-ssc-enable");
phydev->priv = priv;
@@ -707,6 +724,104 @@ static int rtl8211f_config_phy_eee(struct phy_device *phydev)
RTL8211F_PHYCR2_PHY_EEE_ENABLE, 0);
}
+static int rtl8211f_config_clkout_ssc(struct phy_device *phydev)
+{
+ struct rtl821x_priv *priv = phydev->priv;
+ struct device *dev = &phydev->mdio.dev;
+ int ret;
+
+ /* The value is preserved if the device tree property is absent */
+ if (!priv->enable_clkout_ssc)
+ return 0;
+
+ /* RTL8211FVD has PHYCR2 register, but configuration of CLKOUT SSC
+ * is not currently supported by this driver due to different bit
+ * layout.
+ */
+ if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
+ return 0;
+
+ /* Unnamed registers from EMI improvement parameters application note 1.2 */
+ ret = phy_write_paged(phydev, 0xd09, 0x10, 0xcf00);
+ if (ret < 0) {
+ dev_err(dev, "CLKOUT SSC initialization failed: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ /* Enable CLKOUT SSC and CLKOUT SSC Capability using PHYCR2
+ * bits 7, 12, 13. This matches the register 25 write 0x38C3
+ * from the EMI improvement parameters application note 1.2
+ * section 2.3, without affecting unrelated bits.
+ */
+ ret = phy_set_bits(phydev, RTL8211F_PHYCR2,
+ RTL8211F_CLKOUT_SSC_CAP | RTL8211F_CLKOUT_SSC_EN);
+ if (ret < 0) {
+ dev_err(dev, "CLKOUT SSC enable failed: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ return 0;
+}
+
+static int rtl8211f_config_rxc_ssc(struct phy_device *phydev)
+{
+ struct rtl821x_priv *priv = phydev->priv;
+ struct device *dev = &phydev->mdio.dev;
+ int ret;
+
+ /* The value is preserved if the device tree property is absent */
+ if (!priv->enable_rxc_ssc)
+ return 0;
+
+ /* RTL8211FVD has PHYCR2 register, but configuration of RXC SSC
+ * is not currently supported by this driver due to different bit
+ * layout.
+ */
+ if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
+ return 0;
+
+ ret = phy_write_paged(phydev, RTL8211F_SSC_PAGE, RTL8211F_SSC_RXC, 0x5f00);
+ if (ret < 0) {
+ dev_err(dev, "RXC SSC configuration failed: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ return 0;
+}
+
+static int rtl8211f_config_sysclk_ssc(struct phy_device *phydev)
+{
+ struct rtl821x_priv *priv = phydev->priv;
+ struct device *dev = &phydev->mdio.dev;
+ int ret;
+
+ /* The value is preserved if the device tree property is absent */
+ if (!priv->enable_sysclk_ssc)
+ return 0;
+
+ /* RTL8211FVD has PHYCR2 register, but configuration of SYSCLK SSC
+ * is not currently supported by this driver due to different bit
+ * layout.
+ */
+ if (phydev->drv->phy_id == RTL_8211FVD_PHYID)
+ return 0;
+
+ ret = phy_write_paged(phydev, RTL8211F_SSC_PAGE, RTL8211F_SSC_SYSCLK, 0x4f00);
+ if (ret < 0) {
+ dev_err(dev, "SYSCLK SSC configuration failed: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ /* Enable SSC */
+ ret = phy_set_bits(phydev, RTL8211F_PHYCR2, RTL8211F_SYSCLK_SSC_EN);
+ if (ret < 0) {
+ dev_err(dev, "SYSCLK SSC enable failed: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ return 0;
+}
+
static int rtl8211f_config_init(struct phy_device *phydev)
{
struct device *dev = &phydev->mdio.dev;
@@ -723,6 +838,18 @@ static int rtl8211f_config_init(struct phy_device *phydev)
if (ret)
return ret;
+ ret = rtl8211f_config_rxc_ssc(phydev);
+ if (ret)
+ return ret;
+
+ ret = rtl8211f_config_sysclk_ssc(phydev);
+ if (ret)
+ return ret;
+
+ ret = rtl8211f_config_clkout_ssc(phydev);
+ if (ret)
+ return ret;
+
ret = rtl8211f_config_clk_out(phydev);
if (ret) {
dev_err(dev, "clkout configuration failed: %pe\n",
--
2.53.0
^ permalink raw reply related
* Re: [PATCH] net: mdio: realtek-rtl9300: use scoped device_for_each_child_node loop
From: Andrew Lunn @ 2026-04-05 23:30 UTC (permalink / raw)
To: Felix Gu
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Chris Packham, netdev, linux-kernel
In-Reply-To: <20260405-rtl9300-v1-1-08e4499cf944@gmail.com>
On Sun, Apr 05, 2026 at 02:51:52PM +0800, Felix Gu wrote:
> Switch to device_for_each_child_node_scoped() to auto-release fwnode
> references on early exit.
>
> Fixes: 24e31e474769 ("net: mdio: Add RTL9300 MDIO driver")
> Signed-off-by: Felix Gu <ustc.gu@gmail.com>
Hi Felix
Please take a read of:
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
You need to set the subject line correctly.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH] usb: rtl8150: avoid using uninitialized CSCR value
From: Andrew Lunn @ 2026-04-05 23:38 UTC (permalink / raw)
To: Petko Manolov
Cc: Simon Horman, Morduan Zang, Andrew Lunn, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-usb, netdev,
linux-kernel, syzbot+9db6c624635564ad813c
In-Reply-To: <20260405085212.GA8491@cabron.k.g>
> > - get_registers(dev, CSCR, 2, &tmp);
> > + if (get_registers(dev, CSCR, 2, &tmp) < 0)
> > + tmp = 0;
> > if (tmp & CSCR_LINK_STATUS)
> > netif_carrier_on(netdev);
> > else
>
> I was wondering if calling netif_carrier_off() is the right thing to do in case
> get_registers() fail.
>
> There are multiple get_registers() calls that don't check the error and if we do
> this in set_carrier() maybe we should do the same thing across the whole driver?
What does it actually mean if get_registers() fails? The device is
gone? Hot unplugged? If so, you are going to get a cascade of errors,
and then hopefully the USB core code removes the device?
Are there any legitimate reasons for get_registers() to fail if the
device is still plugged in?
It seems netif_carrier_off() is unnecessary?
Andrew
^ permalink raw reply
* Re: [PATCH net v2] tg3: Add PowerEdge R740xd to AER quirk list
From: Andrew Lunn @ 2026-04-05 23:49 UTC (permalink / raw)
To: Oskar Ray-Frayssinet
Cc: pavan.chebbi, mchan, andrew+netdev, davem, edumazet, kuba, pabeni,
horms, lszubowi, netdev, linux-kernel
In-Reply-To: <CAJioSGcBb3eRtgQXHWx+Bhw4M422dyAo-0yXO-XwU7h4hmwmEQ@mail.gmail.com>
On Sun, Apr 05, 2026 at 12:56:11PM +0200, Oskar Ray-Frayssinet wrote:
> On Sun, Apr 05, 2026 at 01:03:00AM +0200, Andrew Lunn wrote:
> > When you look at tg3_restart_aer_quirk_table[] they are all Dell.
> > Are there any Dell systems which actually work? Would it make sense
> > to mark all Dell systems as broken and enable the quirk?
>
> That's a good point. I don't know if there are Dell systems where
> the quirk would cause harm. Do you know if it's safe to apply the
> quirk to all Dell systems, or should we be cautious and keep the
> per-model approach?
You might want to ask on the PCI list.
As far as i see, with 30 seconds for searching and reading
documentation, AER, Advanced Error Reporting is an optional feature of
PCIe. Since it is optional, it seems like disabling it during shutdown
should not have any negative effects. Except for systems where it
actually works, and there are real errors during shutdown and
resume. It will be harder to debug such errors since you don't get the
advanced error report.
Andrew
^ permalink raw reply
* Re: [PATCH bpf v1 1/2] bpf: Fix SOCK_OPS_GET_SK same-register OOB read in sock_ops
From: Emil Tsalapatis @ 2026-04-05 23:49 UTC (permalink / raw)
To: Jiayuan Chen, bpf
Cc: Quan Sun, Yinhao Hu, Kaiyan Mei, Dongliang Mu, Martin KaFai Lau,
Daniel Borkmann, John Fastabend, Stanislav Fomichev,
Alexei Starovoitov, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, linux-kernel, netdev, linux-kselftest
In-Reply-To: <20260404141010.247536-1-jiayuan.chen@linux.dev>
On Sat Apr 4, 2026 at 10:09 AM EDT, Jiayuan Chen wrote:
> When a BPF sock_ops program reads ctx->sk with dst_reg == src_reg
> (e.g., r1 = *(u64 *)(r1 + offsetof(sk))), the SOCK_OPS_GET_SK() macro
> fails to zero the destination register in the is_fullsock == 0 path.
>
> The macro saves/restores a temporary register and checks is_fullsock.
> When is_fullsock == 0 (e.g., TCP_NEW_SYN_RECV state with a request_sock),
> it should set dst_reg = 0 (NULL) so the verifier's PTR_TO_SOCKET_OR_NULL
> type is correct at runtime. Instead, dst_reg retains the original ctx
> pointer, which passes subsequent NULL checks and can be used as a bogus
> socket pointer, leading to stack-out-of-bounds access in helpers like
> bpf_skc_to_tcp6_sock().
>
> Fix by:
> - Changing JMP_A(1) to JMP_A(2) in the fullsock path to skip the
> added instruction.
> - Adding BPF_MOV64_IMM(si->dst_reg, 0) after the temp register
> restore in the !fullsock path, placed after the restore because
> dst_reg == src_reg means we need src_reg intact to read ctx->temp.
>
> Fixes: 84f44df664e9 ("bpf: sock_ops sk access may stomp registers when dst_reg = src_reg")
> Reported-by: Quan Sun <2022090917019@std.uestc.edu.cn>
> Reported-by: Yinhao Hu <dddddd@hust.edu.cn>
> Reported-by: Kaiyan Mei <M202472210@hust.edu.cn>
> Reported-by: Dongliang Mu <dzm91@hust.edu.cn>
> Closes: https://lore.kernel.org/bpf/6fe1243e-149b-4d3b-99c7-fcc9e2f75787@std.uestc.edu.cn/T/#u
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
This patch only seems to fix the problem when dst_reg == src_reg.
Why is this not an issue when is_fullsock == 0, but dst_reg != src_reg?
In that case the dst_reg is unmodified by the whole macro but is still
marked as PTR_TO_SOCKET_OR_NULL. Isn't that a problem? Can you add
a test case for is_fullsock == 0 but dst_reg != src_reg in patch 2?
> ---
> Apologies for the Easter timing!
> ---
> net/core/filter.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 78b548158fb05..8fee00e6adef4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -10618,10 +10618,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
> si->dst_reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, sk));\
> if (si->dst_reg == si->src_reg) { \
> - *insn++ = BPF_JMP_A(1); \
> + *insn++ = BPF_JMP_A(2); \
> *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, \
> temp)); \
> + *insn++ = BPF_MOV64_IMM(si->dst_reg, 0); \
> } \
> } while (0)
>
^ permalink raw reply
* Re: [PATCH v3 net-next] net: use get_random_u{16,32,64}() where appropriate
From: Andrew Lunn @ 2026-04-05 23:51 UTC (permalink / raw)
To: David Carlier
Cc: Jakub Kicinski, David S . Miller, Eric Dumazet, Paolo Abeni,
Andrew Lunn, Simon Horman, Ilya Dryomov, Johannes Berg,
Matthieu Baerts, Mat Martineau, Geliang Tang, Aaron Conole,
Ilya Maximets, Marcelo Ricardo Leitner, Xin Long, Jon Maloy,
netdev, ceph-devel, linux-wireless, mptcp, dev, linux-sctp,
tipc-discussion, linux-kernel
In-Reply-To: <20260405154816.4774-1-devnexen@gmail.com>
On Sun, Apr 05, 2026 at 04:48:16PM +0100, David Carlier wrote:
> Use the typed random integer helpers instead of
> get_random_bytes() when filling a single integer variable.
> The helpers return the value directly, require no pointer
> or size argument, and better express intent.
>
> Skipped sites writing into __be16 fields (netdevsim) where
> a direct assignment would trigger sparse endianness warnings.
>
> Signed-off-by: David Carlier <devnexen@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH bpf v1 1/2] bpf: Fix SOCK_OPS_GET_SK same-register OOB read in sock_ops
From: Emil Tsalapatis @ 2026-04-05 23:54 UTC (permalink / raw)
To: Emil Tsalapatis, Jiayuan Chen, bpf
Cc: Quan Sun, Yinhao Hu, Kaiyan Mei, Dongliang Mu, Martin KaFai Lau,
Daniel Borkmann, John Fastabend, Stanislav Fomichev,
Alexei Starovoitov, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, linux-kernel, netdev, linux-kselftest
In-Reply-To: <DHLMGWNKBI4K.3LM87NOU4XB3O@etsalapatis.com>
On Sun Apr 5, 2026 at 7:49 PM EDT, Emil Tsalapatis wrote:
> On Sat Apr 4, 2026 at 10:09 AM EDT, Jiayuan Chen wrote:
>> When a BPF sock_ops program reads ctx->sk with dst_reg == src_reg
>> (e.g., r1 = *(u64 *)(r1 + offsetof(sk))), the SOCK_OPS_GET_SK() macro
>> fails to zero the destination register in the is_fullsock == 0 path.
>>
>> The macro saves/restores a temporary register and checks is_fullsock.
>> When is_fullsock == 0 (e.g., TCP_NEW_SYN_RECV state with a request_sock),
>> it should set dst_reg = 0 (NULL) so the verifier's PTR_TO_SOCKET_OR_NULL
>> type is correct at runtime. Instead, dst_reg retains the original ctx
>> pointer, which passes subsequent NULL checks and can be used as a bogus
>> socket pointer, leading to stack-out-of-bounds access in helpers like
>> bpf_skc_to_tcp6_sock().
>>
>> Fix by:
>> - Changing JMP_A(1) to JMP_A(2) in the fullsock path to skip the
>> added instruction.
>> - Adding BPF_MOV64_IMM(si->dst_reg, 0) after the temp register
>> restore in the !fullsock path, placed after the restore because
>> dst_reg == src_reg means we need src_reg intact to read ctx->temp.
>>
>> Fixes: 84f44df664e9 ("bpf: sock_ops sk access may stomp registers when dst_reg = src_reg")
>> Reported-by: Quan Sun <2022090917019@std.uestc.edu.cn>
>> Reported-by: Yinhao Hu <dddddd@hust.edu.cn>
>> Reported-by: Kaiyan Mei <M202472210@hust.edu.cn>
>> Reported-by: Dongliang Mu <dzm91@hust.edu.cn>
>> Closes: https://lore.kernel.org/bpf/6fe1243e-149b-4d3b-99c7-fcc9e2f75787@std.uestc.edu.cn/T/#u
>> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
>
> This patch only seems to fix the problem when dst_reg == src_reg.
> Why is this not an issue when is_fullsock == 0, but dst_reg != src_reg?
> In that case the dst_reg is unmodified by the whole macro but is still
> marked as PTR_TO_SOCKET_OR_NULL. Isn't that a problem? Can you add
> a test case for is_fullsock == 0 but dst_reg != src_reg in patch 2?
Sorry for the double post, but also check sashiko.dev:
SOSK_OPTS_GET_FIELD seems to have the same issue as the
SOCK_OPTS_GET_SK. Can you add the same fix to it?
>
>> ---
>> Apologies for the Easter timing!
>> ---
>> net/core/filter.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 78b548158fb05..8fee00e6adef4 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -10618,10 +10618,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
>> si->dst_reg, si->src_reg, \
>> offsetof(struct bpf_sock_ops_kern, sk));\
>> if (si->dst_reg == si->src_reg) { \
>> - *insn++ = BPF_JMP_A(1); \
>> + *insn++ = BPF_JMP_A(2); \
>> *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg, \
>> offsetof(struct bpf_sock_ops_kern, \
>> temp)); \
>> + *insn++ = BPF_MOV64_IMM(si->dst_reg, 0); \
>> } \
>> } while (0)
>>
^ permalink raw reply
* Re: [PATCH] rxrpc/proc: size address buffers for %pISpc output
From: Pengpeng Hou @ 2026-04-06 6:10 UTC (permalink / raw)
To: David Howells, Marc Dionne
Cc: Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
linux-afs, netdev, linux-kernel, pengpeng
In-Reply-To: <20260404190004.4-rxrpc-proc-pengpeng@iscas.ac.cn>
Hi,
Yes. My original changelog example was too loose, and your quick test is
right for a fully expanded plain IPv6 form such as
[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff]:65535
That form is only 47 visible characters, so it fits in the current
char[50] buffers.
The reason I still think the bug is real is the current %pISpc
implementation in lib/vsprintf.c.
For AF_INET6, %pISpc goes through ip6_addr_string_sa(), and the compressed
path uses ip6_compressed_string(). That helper switches to a dotted-quad
tail not only for v4mapped addresses, but also for ISATAP addresses:
useIPv4 = ipv6_addr_v4mapped(&in6) || ipv6_addr_is_isatap(&in6);
So a current-tree case such as
[ffff:ffff:ffff:ffff:0:5efe:255.255.255.255]:65535
is possible. That string is 50 visible characters, i.e. 51 bytes
including the trailing NUL, which does not fit in the existing char[50]
buffers used by the rxrpc procfs helpers.
So I agree the example in my changelog should be corrected, but I do not
think the underlying bug goes away. The claim should be framed around the
ISATAP case rather than the plain IPv6 or mapped-v4 examples I used
originally.
If that makes sense, I can resend with the changelog corrected to cite the
actual maximum case explicitly.
Thanks,
Pengpeng
^ permalink raw reply
* Re: [PATCH bpf v1 2/2] selftests/bpf: Add test for SOCK_OPS_GET_SK with same src/dst register
From: Emil Tsalapatis @ 2026-04-06 1:03 UTC (permalink / raw)
To: Jiayuan Chen, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song, Jiri Olsa, John Fastabend,
Stanislav Fomichev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Shuah Khan, linux-kernel, netdev,
linux-kselftest
In-Reply-To: <20260404141010.247536-2-jiayuan.chen@linux.dev>
On Sat Apr 4, 2026 at 10:09 AM EDT, Jiayuan Chen wrote:
> Add a selftest that verifies SOCK_OPS_GET_SK() correctly returns NULL
> when dst_reg == src_reg and is_fullsock == 0.
>
> The BPF program reads ctx->is_fullsock, then loads ctx->sk using the
> same register for source and destination. When is_fullsock == 0, sk
> must be NULL. The test triggers this path via a TCP handshake (which
> causes TCP_NEW_SYN_RECV state) and checks:
> - null_seen == 1: the is_fullsock==0 path was hit with correct NULL sk
> - bug_detected == 0: sk was never non-NULL when is_fullsock==0
Can you add a test for dst_reg != src_reg, too?
>
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
> .../bpf/prog_tests/sock_ops_get_sk.c | 62 +++++++++++++++++++
> .../selftests/bpf/progs/sock_ops_get_sk.c | 44 +++++++++++++
> 2 files changed, 106 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c
> create mode 100644 tools/testing/selftests/bpf/progs/sock_ops_get_sk.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c b/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c
> new file mode 100644
> index 0000000000000..9eaf97786c1d9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "cgroup_helpers.h"
> +#include "network_helpers.h"
> +#include "sock_ops_get_sk.skel.h"
> +
> +/*
> + * Test that reading ctx->sk with dst_reg == src_reg in a sock_ops program
> + * correctly returns NULL when is_fullsock == 0 (TCP_NEW_SYN_RECV state).
> + *
> + * The bug was in SOCK_OPS_GET_SK() macro which failed to zero the destination
> + * register when it was the same as the source register and the socket was not
> + * a full socket. This left the ctx pointer in the register, which then passed
> + * the NULL check and could be used as a socket pointer, causing OOB access.
The reader of this file will not have context on the original bug, and
the dst_reg == src_reg condition is more obvious in the BPF file. Can
you move it there?
> + */
> +void test_sock_ops_get_sk(void)
> +{
> + struct sock_ops_get_sk *skel;
> + int cgroup_fd, server_fd, client_fd;
> + int prog_fd, err;
> +
> + cgroup_fd = test__join_cgroup("/sock_ops_get_sk");
> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> + return;
> +
> + skel = sock_ops_get_sk__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel_open_load"))
> + goto close_cgroup;
> +
> + prog_fd = bpf_program__fd(skel->progs.sock_ops_get_sk_same_reg);
> + err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
> + if (!ASSERT_OK(err, "prog_attach"))
> + goto destroy_skel;
> +
> + server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
> + if (!ASSERT_GE(server_fd, 0, "start_server"))
> + goto detach;
> +
> + /* Trigger TCP handshake which causes TCP_NEW_SYN_RECV state where
> + * is_fullsock == 0. With the bug, ctx->sk loaded with same src/dst
> + * register would return a stale ctx pointer instead of NULL.
> + */
> + client_fd = connect_to_fd(server_fd, 0);
> + if (!ASSERT_GE(client_fd, 0, "connect_to_fd"))
> + goto close_server;
> +
> + close(client_fd);
> +
> + /* Verify that the is_fullsock == 0 path was hit and sk was NULL */
> + ASSERT_EQ(skel->bss->null_seen, 1, "null_seen");
> + ASSERT_EQ(skel->bss->bug_detected, 0, "bug_not_detected");
> +
> +close_server:
> + close(server_fd);
> +detach:
> + bpf_prog_detach(cgroup_fd, BPF_CGROUP_SOCK_OPS);
> +destroy_skel:
> + sock_ops_get_sk__destroy(skel);
> +close_cgroup:
> + close(cgroup_fd);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c b/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c
> new file mode 100644
> index 0000000000000..4a75614b21eb5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/sock_ops_get_sk.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +/*
> + * Test that SOCK_OPS_GET_SK() correctly returns NULL when dst_reg == src_reg
> + * and is_fullsock == 0 (e.g., during TCP_NEW_SYN_RECV). Before the fix, the
Again, writing "the fix" here is vague.
> + * macro failed to zero the destination register, leaving a stale ctx pointer
> + * that bypassed the NULL check.
> + */
> +
> +int bug_detected;
> +int null_seen;
> +
> +SEC("sockops")
> +__naked void sock_ops_get_sk_same_reg(void)
> +{
> + asm volatile (
> + "r7 = *(u32 *)(r1 + %[is_fullsock_off]);"
> + "r1 = *(u64 *)(r1 + %[sk_off]);"
> + "if r7 != 0 goto 2f;"
> + "if r1 == 0 goto 1f;"
> + "r1 = %[bug_detected] ll;"
> + "r2 = 1;"
> + "*(u32 *)(r1 + 0) = r2;"
> + "goto 2f;"
> + "1:"
> + "r1 = %[null_seen] ll;"
> + "r2 = 1;"
> + "*(u32 *)(r1 + 0) = r2;"
> + "2:"
> + "r0 = 1;"
> + "exit;"
> + :
> + : __imm_const(is_fullsock_off, offsetof(struct bpf_sock_ops, is_fullsock)),
> + __imm_const(sk_off, offsetof(struct bpf_sock_ops, sk)),
> + __imm_addr(bug_detected),
> + __imm_addr(null_seen)
> + : __clobber_all);
> +}
> +
> +char _license[] SEC("license") = "GPL";
^ permalink raw reply
* Re: [PATCH net-next v2 0/4] bpf-timestamp: convert to push-level granularity
From: Willem de Bruijn @ 2026-04-06 2:17 UTC (permalink / raw)
To: Jason Xing, davem, edumazet, kuba, pabeni, horms, willemb,
martin.lau
Cc: netdev, bpf, Jason Xing
In-Reply-To: <20260404150452.83904-1-kerneljasonxing@gmail.com>
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
>
> 1. Design of send-level granularity
> Originally, socket timestamping was designed to support tracing each
> sendmsg instead of per packet because application needs to issue
> multiple extra recvmsg() calls to get the skbs carrying the timestamp
> one by one if application chooses tag with different tags(SCHED/DRV/ACK).
> It's an obvious huge burden if the application expects to see a finer
> grained behavior.
> Another point I mentioned a bit in Netdev 0x19[1], supposing the amount of
> data that application tries to transfer at one time is split into 100
> smaller packets, recording the last skb's timestamps (SCHED/DRV/HARDWARE)
> is no longer meaningful because at the moment timestamping only records
> 1/100 packets. In this case, only the delta between when to send and when
> to ack matters.
>
> 2. Known missing tag issues in TCP
> A critically important thing is that we can miss tagging the last packet
> in a few conditions as the patch 3/4 explains. That means we lose track
> of the send syscall. Digging into more into how tcp_sendmsg_locked works,
> I found it's not feasible to successfully identify the last skb before
> push functions get called. With that said, if we want to make the feature
> better to cover all of these cases, we inevitably needs to place
> tcp_bpf_tx_timestamp() function before each push function.
>
> 3. Practice at Tencent
> In production, we have a version that applies the packet basis policy to
> do the exhaustive profiling of each flow for months in order to:
> 1) 100% make sure to capture the jitter event. No sampling.
> 2) observe the performance, find the bottleneck and improve it.
> We're still collecting data and investigating how it helps us in all the
> potential aspects before upstreaming. My personal perspective on this is
> to replace tcpdump eventually. It's worth mentioning tcpdump no longer
> satisfies our micro observation in modern data center.
>
> 4. The tendency toward finer-grained observability
> As we're aware that there are already many various bpf scripts trying to
> implement the fine grained monitor of the packets, it's an unstoppable
> tendency for the future observability. We're faced with so many latency
> reports (like jitter, perf degradation) on a daily basis. Getting the
> root cause of each report is exactly what we pursue.
> After we know which request causes the problem, if it belongs to kernel,
> we will dig into the packet behavior with more useful information
> included. This is the process of tracing down the jitter problem.
> Likewise, in BPF timestamping that mitigates the impact of calling extra
> syscalls, breaking the coarse granularity into smaller ones is a first
> good way to go. It shouldn't be the burden like before especially it's
> independent of application.
>
> 5. Details of the series
> Now it's time to convert BPF timestamping feature into push-level
> granularity by only recording the last skb in each push function, which
> is quite similar to how we previously treat each send syscall.
> Regarding each push function as a whole, we only care about
> the last skb from each push since the skb can be chunked into different
> smaller packets. BPF scripts like progs/net_timestamping.c has the
> ability to trace each tagged skb and calculate the latency:
> 1) delta between send and each tagged skb in tcp_sendmsg_locked()
> 2) delta between SCHED/DRV/ACK. Three timestamps are also correlated
> with the sendmsg time.
>
> In conclusion, push-level is more of a compromise approach which covers
> those corner cases and further enhances the capabilities (like a finer
> grained observation of jitter and performance issues).
# push-level design
It it significantly less intuitive than per-syscall, which is under
user control. Or even than per-packet. As a fix for missing timestamps
I understand these two extensions, even with the unintended side
effect of reporting many unnecessary extra skbs in the common case.
As a model to advocate for, less so.
Would it help if all skbs from the same sendmsg() can still be
identified as common from the same syscall? That allows the user
to discard all but the last one (if they wish)
# ABI changes
For SO_TIMESTAMPING we would not be able to make this change
unconditionally as the behavior change would break existing
application expectations.
That is why historically we have guarded new behabvior behind new
TS options flags.
The same may be true for BPF.
# SO_TIMESTAMPING and BPF timestamping differences
A related point is that this breaks the 1:1 relationship between
SO_TIMESTAMPING and BPF timestamping. As said before, I think that
is fine as BPF timestamping can be cheaper. But we should avoid the
two forking in incompatible ways. I suggest that BPF timestamping
becomes a superset of SO_TIMESTAMPING: it must have all features
of SO_TIMESTAMPING, and may offer more.
# Documentation and testing
Please also expand Documentation and include a test.
> [1]: Page 29 of the slides demonstrates the picture of skb-level granularity
> https://netdevconf.info/0x19/sessions/talk/the-future-of-so_timestamping.html
>
> ---
> V2
> Link: https://lore.kernel.org/all/20260402085831.36983-1-kerneljasonxing@gmail.com/
> 1. only handle BPF timestamping feature to cover those issues (Eric, Willem)
> 2. keep timestamping functions inline in send process (Eric)
>
>
> Jason Xing (4):
> tcp: separate BPF timestamping from tcp_tx_timestamp
> tcp: advance the tsflags check to save cycles
> bpf-timestamp: keep track of the skb when wait_for_space occurs
> bpf-timestamp: complete tracing the skb from each push in sendmsg
>
> include/net/tcp.h | 20 ++++++++++++++++++++
> net/ipv4/tcp.c | 23 +++++++++++++----------
> 2 files changed, 33 insertions(+), 10 deletions(-)
>
> --
> 2.41.3
>
^ permalink raw reply
* Re: [PATCH net-next v2 2/4] tcp: advance the tsflags check to save cycles
From: Willem de Bruijn @ 2026-04-06 2:23 UTC (permalink / raw)
To: Jason Xing, davem, edumazet, kuba, pabeni, horms, willemb,
martin.lau
Cc: netdev, bpf, Jason Xing, Yushan Zhou
In-Reply-To: <20260404150452.83904-3-kerneljasonxing@gmail.com>
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
>
> Check the tsflags first to see if the socket timestamping is enabled.
> If so, then try to fetch the last skb from either write queue or
> retransmission queue.
This message does not explain why this change is made.
^ permalink raw reply
* Re: [PATCH net-next v2 3/4] bpf-timestamp: keep track of the skb when wait_for_space occurs
From: Willem de Bruijn @ 2026-04-06 2:28 UTC (permalink / raw)
To: Jason Xing, davem, edumazet, kuba, pabeni, horms, willemb,
martin.lau
Cc: netdev, bpf, Jason Xing, Yushan Zhou
In-Reply-To: <20260404150452.83904-4-kerneljasonxing@gmail.com>
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
>
> The patch is the 1/2 part of push-level granularity feature.
>
> Tag the skb in tcp_sendmsg_locked() when wait_for_space occurs even
> though it might not carry the last byte of the sendmsg.
>
> Prior to the patch, BPF timestamping cannot cover this case:
> The following steps reproduce this:
> 1) skb A is the current last skb before entering wait_for_space process
> 2) tcp_push() pushes A without any tag
> 3) A is transmitted from TCP to driver without putting any skb carrying
> timestamps in the error queue, like SCHED, DRV/HARDWARE.
> 4) sk_stream_wait_memory() sleeps for a while and then returns with an
> error code. Note that the socket lock is released.
> 5) skb A finally gets acked and removed from the rtx queue.
> 6) continue with the rest of tcp_sendmsg_locked(): it will jump to(goto)
> 'do_error' label and then 'out' label.
> 7) at this moment, skb A turns out to be the last one in this send
> syscall, and miss the following tcp_bpf_tx_timestamp() opportunity
> before the final tcp_push()
> 8) BPF script fails to see any timestamps this time
>
> Signed-off-by: Yushan Zhou <katrinzhou@tencent.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> net/ipv4/tcp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index c603b90057f6..7d030a11d004 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1400,9 +1400,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> wait_for_space:
> set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> tcp_remove_empty_skb(sk);
> - if (copied)
> + if (copied) {
> + tcp_bpf_tx_timestamp(sk);
> tcp_push(sk, flags & ~MSG_MORE, mss_now,
> TCP_NAGLE_PUSH, size_goal);
Now the number of skbs that will be tracked will be unpredictable,
varying based on memory pressure.
That sounds hard to use to me. Especially if these extra pushes
cannot be identified as such.
Perhaps if all skbs from the same sendmsg call can be identified,
that would help explain pattern in data resulting from these
uncommon extra data points.
> + }
>
> err = sk_stream_wait_memory(sk, &timeo);
> if (err != 0)
> --
> 2.41.3
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox