* Re: [PATCH net-next 09/12] gpio: tc956x: add TC956x/QPS615 support
From: Bartosz Golaszewski @ 2026-05-04 12:46 UTC (permalink / raw)
To: Alex Elder
Cc: daniel, mohd.anwar, a0987203069, alexandre.torgue, ast,
boon.khai.ng, chenchuangyu, chenhuacai, daniel, hawk, hkallweit1,
inochiama, john.fastabend, julianbraha, livelycarpet87,
matthew.gerlach, mcoquelin.stm32, me, prabhakar.mahadev-lad.rj,
richardcochran, rohan.g.thomas, sdf, siyanteng, weishangjuan,
wens, netdev, bpf, linux-arm-msm, devicetree, linux-gpio,
linux-stm32, linux-arm-kernel, linux-kernel, andrew+netdev, davem,
edumazet, kuba, pabeni, maxime.chevallier, rmk+kernel, andersson,
konradybcio, robh, krzk+dt, conor+dt, linusw, brgl, arnd, gregkh
In-Reply-To: <20260501155421.3329862-10-elder@riscstar.com>
On Fri, 1 May 2026 17:54:17 +0200, Alex Elder <elder@riscstar.com> said:
> Toshiba TC956x is an Ethernet-AVB/TSN bridge and is essentially
> a small and highly-specialized SoC. TC956x includes a GPIO block that
> can be accessed, alongside several other peripherals, via two PCIe
> endpoint functions. The PCIe function driver creates an auxiliary
> device for the GPIO block, and that device gets bound to this auxiliary
> device driver.
>
> Co-developed-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> drivers/gpio/Kconfig | 11 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-tc956x.c | 209 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 221 insertions(+)
> create mode 100644 drivers/gpio/gpio-tc956x.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 020e51e30317a..746cedea7e91d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1646,6 +1646,17 @@ config GPIO_TC3589X
> This enables support for the GPIOs found on the TC3589X
> I/O Expander.
>
> +config GPIO_TC956X
> + tristate "Toshiba TC956X GPIO support"
> + depends on TOSHIBA_TC956X_PCI
> + default m if TOSHIBA_TC956X_PCI
> + help
> + This enables support for the GPIO controller embedded in the Toshiba
> + TC956X (and Qualcomm QPS615). This device connects to the host
> + via PCIe port, which is the upstream port on an internal PCIe
> + switch. On some platforms, a few of the GPIO lines are used to
> + manage external resets.
> +
> config GPIO_TIMBERDALE
> bool "Support for timberdale GPIO IP"
> depends on MFD_TIMBERDALE
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index b267598b517de..c3584e7cba9b4 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -178,6 +178,7 @@ obj-$(CONFIG_GPIO_SYSCON) += gpio-syscon.o
> obj-$(CONFIG_GPIO_TANGIER) += gpio-tangier.o
> obj-$(CONFIG_GPIO_TB10X) += gpio-tb10x.o
> obj-$(CONFIG_GPIO_TC3589X) += gpio-tc3589x.o
> +obj-$(CONFIG_GPIO_TC956X) += gpio-tc956x.o
> obj-$(CONFIG_GPIO_TEGRA186) += gpio-tegra186.o
> obj-$(CONFIG_GPIO_TEGRA) += gpio-tegra.o
> obj-$(CONFIG_GPIO_THUNDERX) += gpio-thunderx.o
> diff --git a/drivers/gpio/gpio-tc956x.c b/drivers/gpio/gpio-tc956x.c
> new file mode 100644
> index 0000000000000..12221d8f812d9
> --- /dev/null
> +++ b/drivers/gpio/gpio-tc956x.c
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved.
> + */
> +
> +/*
> + * The Toshiba TC956X implements a PCIe Gen 3 switch that connects an
> + * upstream x4 port to two downstream PCIe x2 ports. It incorporates
> + * an internal endpoint on a internal PCIe port that implements two
> + * Synopsys XGMAC Ethernet interfaces.
> + *
> + * 35 GPIOs are also implemented by an embedded GPIO controller. Three
> + * registers control the first 32 GPIOs (other than 20 and 21, which are
> + * reserved). Three other registers control GPIOs 32 through 36. GPIOs
> + * 22-24, 27-28, 31, and 34 are treated as "input only".
> + *
> + * There is a TC956X PCI power controller driver that accesses the
> + * direction and output value registers for GPIOs 2 and 3. These
> + * GPIOs control the reset signal for the two downstream PCIe ports.
> + * Their values will never change during operation of this driver, and
> + * this driver reserves these two GPIOS.
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/dev_printk.h>
This is implied by device.h which is guarnteed by platform_device.h. Please
drop it.
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define DRIVER_NAME "tc956x-gpio"
> +
> +#define TC956X_GPIO_COUNT 37 /* Number of GPIOs (20-21 reserved) */
> +
> +/* The GPIO offsets are relative to 0x1200 in TC956X SFR space */
> +#define GPIO_IN0_OFFSET 0x00 /* Input value (0-31) */
> +#define GPIO_EN0_OFFSET 0x08 /* 0: out; 1: in (0-31) */
> +#define GPIO_OUT0_OFFSET 0x10 /* Output value (0-31) */
> +
> +#define GPIO_IN1_OFFSET 0x04 /* Input value (32-36) */
> +#define GPIO_EN1_OFFSET 0x0c /* 0: out; 1: in (32-36) */
> +#define GPIO_OUT1_OFFSET 0x14 /* Output value (32-36) */
> +
> +/*
> + * struct tc956x_gpio - Information related to the embedded GPIO controller
> + * @chip: GPIO chip structure
> + * @regmap: MMIO register map for SFR GPIO region access
> + * @input_only: Bitmap indicating which GPIOs are input-only
> + */
> +struct tc956x_gpio {
> + struct gpio_chip chip;
> + struct regmap *regmap;
> + DECLARE_BITMAP(input_only, TC956X_GPIO_COUNT);
> +};
> +
> +static int tc956x_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> + struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> + u32 reg;
> + u32 val;
> +
> + if (test_bit(offset, gpio->input_only))
> + return GPIO_LINE_DIRECTION_IN;
> +
> + reg = offset < 32 ? GPIO_EN0_OFFSET : GPIO_EN1_OFFSET;
> +
> + regmap_read(gpio->regmap, reg, &val);
> + if (val & BIT(offset % 32))
> + return GPIO_LINE_DIRECTION_IN;
> +
> + return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int tc956x_gpio_direction_input(struct gpio_chip *gc,
> + unsigned int offset)
> +{
> + u32 reg = offset < 32 ? GPIO_EN0_OFFSET : GPIO_EN1_OFFSET;
> + struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> + u32 mask = BIT(offset % 32);
> +
> + return regmap_update_bits(gpio->regmap, reg, mask, mask);
> +}
> +
> +static int tc956x_gpio_direction_output(struct gpio_chip *gc,
> + unsigned int offset, int value)
> +{
> + struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> + u32 vreg;
> + u32 dreg;
> + u32 mask;
> +
> + if (test_bit(offset, gpio->input_only))
> + return -EINVAL;
> +
> + if (offset < 32) {
> + vreg = GPIO_OUT0_OFFSET;
> + dreg = GPIO_EN0_OFFSET;
> + } else {
> + vreg = GPIO_OUT1_OFFSET;
> + dreg = GPIO_EN1_OFFSET;
> + }
> + mask = BIT(offset % 32);
> +
> + /* Set output value first, then direction */
> + regmap_update_bits(gpio->regmap, vreg, mask, value ? mask : 0);
> +
> + return regmap_update_bits(gpio->regmap, dreg, mask, 0);
> +}
> +
> +static int tc956x_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> + u32 reg = offset < 32 ? GPIO_IN0_OFFSET : GPIO_IN1_OFFSET;
> + struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> + u32 val;
> +
> + regmap_read(gpio->regmap, reg, &val);
> +
> + return val & BIT(offset % 32) ? 1 : 0;
> +}
> +
> +static int tc956x_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
> +{
> + u32 reg = offset < 32 ? GPIO_OUT0_OFFSET : GPIO_OUT1_OFFSET;
> + struct tc956x_gpio *gpio = gpiochip_get_data(gc);
> + u32 mask = BIT(offset % 32);
> +
> + return regmap_update_bits(gpio->regmap, reg, mask, value ? mask : 0);
> +}
> +
> +static int tc956x_gpio_init_valid_mask(struct gpio_chip *gc,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + /*
> + * GPIOs 2 and 3 are used by the PCI power control driver, and
> + * we don't allow them to be used. GPIOs 20 and 21 are reserved
> + * (and not usable).
> + */
> + bitmap_fill(valid_mask, ngpios);
> + bitmap_clear(valid_mask, 2, 2);
> + bitmap_clear(valid_mask, 20, 2);
> +
> + return 0;
> +}
> +
> +static int tc956x_gpio_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct device *dev = &adev->dev;
> + struct tc956x_gpio *gpio;
> + struct gpio_chip *gc;
> +
> + if (!dev->platform_data)
> + return -EINVAL;
> +
> + gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
Add newline.
> + gpio->regmap = dev->platform_data;
It's not clear whether this is an mmio regmap or a slow-bus one that can fail.
In the code above you're checking the return values of regmap operations quite
inconsistently. Could you please verify if you need it and either always check
them or not at all?
> +
> + /* Mark GPIOs 22, 23, 24, 27, 28, 31, and 34 as input only */
> + bitmap_set(gpio->input_only, 22, 3);
> + bitmap_set(gpio->input_only, 27, 2);
> + set_bit(31, gpio->input_only);
> + set_bit(34, gpio->input_only);
> +
> + gc = &gpio->chip;
> +
> + gc->label = DRIVER_NAME;
> + gc->parent = dev->parent;
> +
> + gc->get_direction = tc956x_gpio_get_direction;
> + gc->direction_input = tc956x_gpio_direction_input;
> + gc->direction_output = tc956x_gpio_direction_output;
> + gc->get = tc956x_gpio_get;
> + gc->set = tc956x_gpio_set;
> + gc->init_valid_mask = tc956x_gpio_init_valid_mask;
> +
> + gc->base = -1;
> + gc->ngpio = TC956X_GPIO_COUNT;
> + gc->can_sleep = false;
This makes me think this is an MMIO regmap after all.
> +
> + dev_set_drvdata(dev, gpio);
There's no corresponding dev_get_drvdata().
> +
> + return devm_gpiochip_add_data(dev, gc, gpio);
> +}
> +
> +static const struct auxiliary_device_id tc956x_gpio_ids[] = {
> + { .name = "tc956x_pci.tc9564-gpio", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, tc956x_gpio_ids);
> +
> +static struct auxiliary_driver tc956x_gpio_driver = {
> + .name = DRIVER_NAME,
> + .probe = tc956x_gpio_probe,
> + .id_table = tc956x_gpio_ids,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> +};
> +module_auxiliary_driver(tc956x_gpio_driver);
> +
> +MODULE_DESCRIPTION("Toshiba TC956X PCIe GPIO Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("auxiliary:" DRIVER_NAME);
> --
> 2.51.0
>
>
There are a few minor issues but overall looks good!
Bart
^ permalink raw reply
* Re: [PATCH net-next] net: mtk_star_emac: use of_get_ethdev_address
From: Andrew Lunn @ 2026-05-04 12:43 UTC (permalink / raw)
To: Rosen Penev
Cc: netdev, Felix Fietkau, Lorenzo Bianconi, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Matthias Brugger, AngeloGioacchino Del Regno,
open list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support,
moderated list:ARM/Mediatek SoC support
In-Reply-To: <20260504031019.607682-1-rosenp@gmail.com>
On Sun, May 03, 2026 at 08:10:19PM -0700, Rosen Penev wrote:
> The platform_ variant calls arch_get_platform_mac_address which is only
> implemented under SPARC.
platform_get_ethdev_address() calls eth_platform_get_mac_address().
int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
{
unsigned char *addr;
int ret;
ret = of_get_mac_address(dev->of_node, mac_addr);
if (!ret)
return 0;
addr = arch_get_platform_mac_address();
if (!addr)
return -ENODEV;
ether_addr_copy(mac_addr, addr);
return 0;
}
> Switch to the of variant as of functions are used in the surrounding
> code and to get EPROBE_DEFER support in order to handle NVMEM MAC
> address specifications.
So there is a call to of_get_mac_address(). And it calls
arch_get_platform_mac_address() as well. If you only call
of_get_mac_address(), don't you break SPARC?
If EPROBE_DEFER is what you are trying to get, please change
eth_platform_get_mac_address() to actually return it, not break SPARC.
Andrew
---
pw-bot: cr
^ permalink raw reply
* Re: [PATCH 1/6] lib: include crc32.h conditionally on CONFIG_CRC32
From: David Laight @ 2026-05-04 12:43 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Yury Norov, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Yury Norov, Rasmus Villemoes, Andrew Lunn,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Morton, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev,
Ruan Jinjie, linux-kernel, linux-riscv, Linux-Arch, Netdev, bpf,
Nathan Chancellor
In-Reply-To: <ec4ed7f5-b1c8-49e4-b83d-e29c5414b9de@app.fastmail.com>
On Mon, 04 May 2026 10:03:10 +0200
"Arnd Bergmann" <arnd@arndb.de> wrote:
> On Thu, Apr 30, 2026, at 23:13, Yury Norov wrote:
> > Currently, bitreverse API is either declared based on
> > CONFIG_HAVE_ARCH_BITREVERSE, wired to arch implementation, or if the
> > arch has no bitreverse, based on generic implementation.
> >
> > So, regardless of CONFIG_BITREVERSE=n, the corresponding API is always
> > declared. If that happens, the functions become declared but not
> > implemented, which is an error.
>
> I'm not following that description. Why is it an error to declare
> a funtion that is not implemented? Isn't that how optional interfaces
> tend to work in general?
>
> > The only header requiring the crc32 and bitreverse prototypes is
> > include/linux/etherdevice.h. Thus, protect inclusion of corresponding
> > headers in the etherdevice with CONFIG_CRC32, together with the only
> > function depending on it.
> ...
> > #include <linux/if_ether.h>
> > #include <linux/netdevice.h>
> > #include <linux/random.h>
> > +#ifdef CONFIG_CRC32
> > #include <linux/crc32.h>
> > +#endif
> > #include <linux/unaligned.h>
> > #include <asm/bitsperlong.h>
>
> Don't add #ifdef blocks around headers. If the header cannot
> be included without side-effects, change the linux/crc32.h
> file instead of its users.
>
> It looks like the problem is the check for CONFIG_GENERIC_BITREVERSE
> in include/asm-generic/bitops/__bitrev.h, which ends up
> hinding the generic___bitrev32() helper without need.
>
> Simply removing the #ifdef there should avoid the build failure.
>
> > +#ifdef CONFIG_CRC32
> > /**
> > * eth_hw_addr_crc - Calculate CRC from netdev_hw_addr
> > * @ha: pointer to hardware address
> > @@ -291,6 +294,7 @@ static inline u32 eth_hw_addr_crc(struct netdev_hw_addr *ha)
> > {
> > return ether_crc(ETH_ALEN, ha->addr);
> > }
> > +#endif
>
> I see there are only user users of this function, neither of
> them are performance critical. So the other options would
> be to either open-code this function in the two callers
> and remove it entirely, or move it into net/ethernet/eth.c.
Or change to a #define so that only the users need to have the
required headers included.
But open-coding in the callers saves anyone trying to read the code
having to look at another file to see what is going on.
-- David
>
> Arnd
>
^ permalink raw reply
* Re: nl80211: SET_WIPHY_NETNS does not check caller's CAP_NET_ADMIN over the target netns
From: Xie Maoyi @ 2026-05-04 12:38 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
In-Reply-To: <316680e2dc0103774bf0cfb77f60341a85ef5b81.camel@sipsolutions.net>
On 5/4/26, Johannes Berg wrote:
> I guess that's more a question of convention than anything else?
>
> But I guess we should follow the netdev convention:
> ...
> which (also?) requires access in the target netns.
Thanks. I will send a patch that mirrors rtnl_get_net_ns_capable() in nl80211_wiphy_netns().
> This seems ... inconsequential? After all, moving a wireless device
> between namespaces doesn't really change the physical layout of the
> machine. Perhaps that'd give someone access to the SSID of some hidden
> network but that's not really a secret anyway since it's over the air.
>
> Maybe we should fix it for clarity and convention, but I don't see it's
> really an issue?
Understood that the impact is small on its own. I would still like to fold it in for the clarity and convention reason you mentioned. The fix in nl80211_prepare_wdev_dump() continuation is one net_eq() line. It brings that path in line with nl80211_dump_wiphy() at line 3437 and the scheduled scan dump at line 4420. Both already do the check on every iteration. Happy to drop it from the series if you prefer to leave it as is.
I will post a 2-patch series shortly. Both patches are already verified end to end on a KASAN VM (the EPERM PoC log was attached to the original report).
Best regards,
Maoyi
Nanyang Technological University
https://maoyixie.com/
________________________________
CONFIDENTIALITY: This email is intended solely for the person(s) named and may be confidential and/or privileged. If you are not the intended recipient, please delete it, notify us and do not copy, use, or disclose its contents.
Towards a sustainable earth: Print only when necessary. Thank you.
^ permalink raw reply
* [PATCH net-next v4 2/2] selftests: openvswitch: add pop_vlan test
From: Minxi Hou @ 2026-05-04 12:37 UTC (permalink / raw)
To: netdev
Cc: aconole, echaudro, i.maximets, davem, edumazet, kuba, pabeni,
horms, shuah, dev, linux-kselftest, Minxi Hou
In-Reply-To: <20260504123713.555461-1-houminxi@gmail.com>
Add test_pop_vlan() to verify OVS kernel datapath pop_vlan action
correctly strips 802.1Q VLAN tags from frames.
Test structure:
- Baseline: untagged forwarding validates basic connectivity.
- Negative: forward without pop_vlan, assert VLAN tag preserved.
- Positive: forward with pop_vlan, assert tag stripped and
untagged ICMP echo request arrives.
Add start_capture/stop_capture helpers using ovs_wait for
deterministic tcpdump readiness instead of ad-hoc sleep.
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
.../selftests/net/openvswitch/openvswitch.sh | 196 ++++++++++++++++++
1 file changed, 196 insertions(+)
diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh b/tools/testing/selftests/net/openvswitch/openvswitch.sh
index b327d3061ed5..95fb8f824b15 100755
--- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
+++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
@@ -27,6 +27,7 @@ tests="
upcall_interfaces ovs: test the upcall interfaces
tunnel_metadata ovs: test extraction of tunnel metadata
drop_reason drop: test drop reasons are emitted
+ pop_vlan vlan: POP_VLAN action strips tag
psample psample: Sampling packets with psample"
info() {
@@ -830,6 +831,201 @@ test_tunnel_metadata() {
return 0
}
+# Start tcpdump capture with deterministic readiness wait.
+# Usage: start_capture <netns> <iface> <pcap_path> <out_pid_var> <out_log_var>
+# $4 and $5 are variable NAMES — start_capture writes the tcpdump PID
+# and log path into those caller variables via nameref (bash 4.3+).
+# Contract: caller MUST call stop_capture with the returned PID and log
+# before returning from the function.
+start_capture() {
+ local ns="$1" iface="$2" pcap="$3"
+ local -n _out_pid="$4"
+ local -n _out_log="$5"
+ local log pid
+
+ command -v tcpdump >/dev/null 2>&1 || {
+ info "tcpdump missing"
+ return $ksft_skip
+ }
+
+ log=$(mktemp)
+ ip netns exec "$ns" tcpdump -nei "$iface" \
+ -w "$pcap" -U 2>"$log" &
+ pid=$!
+ ovs_wait grep -q "listening on" "$log" || {
+ kill $pid 2>/dev/null
+ wait $pid 2>/dev/null
+ rm -f "$log"
+ info "FAIL: tcpdump failed to start on $iface"
+ return 1
+ }
+ kill -0 $pid 2>/dev/null || {
+ wait $pid 2>/dev/null
+ rm -f "$log"
+ info "FAIL: tcpdump died after start on $iface"
+ return 1
+ }
+ # $pid/$log expand now (intentional — captures concrete values)
+ on_exit "kill $pid 2>/dev/null; rm -f $log"
+ _out_pid=$pid
+ _out_log=$log
+}
+
+# Stop capture and cleanup temp files.
+# Usage: stop_capture <pid> <log_path>
+stop_capture() {
+ kill "$1" 2>/dev/null
+ wait "$1" 2>/dev/null
+ rm -f "$2"
+}
+
+test_pop_vlan() {
+ modprobe -q openvswitch 2>/dev/null || true
+ [ -d /sys/module/openvswitch ] || return $ksft_skip
+ local ns_err
+ ns_err=$(mktemp)
+ if ! ip netns add __test_pop_vlan_netns__ 2>"$ns_err"; then
+ if grep -q "File exists" "$ns_err"; then
+ ip netns del __test_pop_vlan_netns__ 2>/dev/null
+ else
+ info "CONFIG_NET_NS missing or unavailable"
+ rm -f "$ns_err"
+ return $ksft_skip
+ fi
+ fi
+ ip netns del __test_pop_vlan_netns__ 2>/dev/null
+ rm -f "$ns_err"
+ modprobe -q 8021q 2>/dev/null || true
+ [ -d /sys/module/8021q ] || \
+ { info "CONFIG_VLAN_8021Q missing"; return $ksft_skip; }
+
+ local sbx="test_pop_vlan"
+ sbx_add "$sbx" || return $?
+ ovs_add_dp "$sbx" vlandp || return 1
+
+ # Validate basic connectivity before testing pop_vlan.
+ # --- baseline: untagged forwarding ---
+ ovs_add_netns_and_veths "$sbx" vlandp \
+ ns1 veth1 ns1veth 192.0.2.1/24 || return 1
+ ovs_add_netns_and_veths "$sbx" vlandp \
+ ns2 veth2 ns2veth 192.0.2.2/24 || return 1
+
+ # ARP + IPv4 bidirectional (all untagged)
+ ovs_add_flow "$sbx" vlandp \
+ 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1
+ ovs_add_flow "$sbx" vlandp \
+ 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1
+ ovs_add_flow "$sbx" vlandp \
+ 'in_port(1),eth(),eth_type(0x0800),ipv4()' '2' || return 1
+ ovs_add_flow "$sbx" vlandp \
+ 'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
+ ip netns exec ns1 ping -c 3 -W 2 192.0.2.2 || return 1
+
+ # --- POP_VLAN test ---
+ # ns1: VLAN sub-interface generates tagged frames
+ ip -n ns1 link add link ns1veth name ns1veth.10 \
+ type vlan id 10 || return 1
+ on_exit "ip -n ns1 link del ns1veth.10 2>/dev/null || true"
+ ip -n ns1 addr add 198.51.100.1/24 dev ns1veth.10 || return 1
+ ip -n ns1 link set ns1veth.10 up || return 1
+
+ # ns2: no VLAN sub-interface. POP delivers untagged frames to ns2veth
+ ip -n ns2 addr add 198.51.100.2/24 dev ns2veth || return 1
+ on_exit "ip -n ns2 addr del 198.51.100.2/24 dev ns2veth 2>/dev/null"
+
+ # veth: disable VLAN offload + GRO (force software tag processing)
+ if command -v ethtool >/dev/null 2>&1; then
+ ip netns exec ns1 ethtool -k ns1veth 2>/dev/null \
+ | grep -q vlan-offload && \
+ ip netns exec ns1 ethtool -K ns1veth \
+ rx-vlan-offload off tx-vlan-offload off \
+ gro off 2>/dev/null || true
+ ip netns exec ns2 ethtool -k ns2veth 2>/dev/null \
+ | grep -q vlan-offload && \
+ ip netns exec ns2 ethtool -K ns2veth \
+ rx-vlan-offload off tx-vlan-offload off \
+ gro off 2>/dev/null || true
+ fi
+
+ ovs_del_flows "$sbx" vlandp
+
+ # Static ARP avoids VLAN-tagged ARP complexity (ns2 has no VLAN
+ # sub-interface, so tagged ARP would be invisible to ns2).
+ local ns1veth10mac ns2mac
+ ns1veth10mac=$(ip -n ns1 link show ns1veth.10 | \
+ awk '/link\/ether/ {print $2}')
+ ns2mac=$(ip -n ns2 link show ns2veth | \
+ awk '/link\/ether/ {print $2}')
+ [ -n "$ns1veth10mac" ] && echo "$ns1veth10mac" | \
+ grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
+ [ -n "$ns2mac" ] && echo "$ns2mac" | \
+ grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1
+ ip -n ns1 neigh replace 198.51.100.2 lladdr "$ns2mac" \
+ dev ns1veth.10 nud permanent || return 1
+ ip -n ns2 neigh replace 198.51.100.1 lladdr "$ns1veth10mac" \
+ dev ns2veth nud permanent || return 1
+
+ # --- Negative check: fwd without pop_vlan, VLAN tag stays ---
+ local vlan_match='in_port(1),eth(),eth_type(0x8100),'
+ vlan_match+='vlan(vid=10),'
+ vlan_match+='encap(eth_type(0x0800),'
+ vlan_match+='ipv4(src=198.51.100.1,proto=1),icmp())'
+ ovs_add_flow "$sbx" vlandp "$vlan_match" '2' || return 1
+
+ local pcap_no_pop
+ pcap_no_pop=$(mktemp --suffix=.pcap)
+ on_exit "rm -f $pcap_no_pop"
+ local tpid tlog
+ start_capture ns2 ns2veth "$pcap_no_pop" tpid tlog || return $?
+
+ ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
+ >/dev/null 2>&1 || true
+ stop_capture "$tpid" "$tlog"
+
+ # assert: VLAN tag still present (no pop_vlan in action)
+ tcpdump -nr "$pcap_no_pop" 'vlan' 2>/dev/null | grep -q . || {
+ info "FAIL: negative check: no VLAN tag (expected tag present)"
+ return 1
+ }
+
+ ovs_del_flows "$sbx" vlandp
+
+ # --- Positive: pop_vlan strips tag ---
+ ovs_add_flow "$sbx" vlandp "$vlan_match" 'pop_vlan,2' || return 1
+ ovs_add_flow "$sbx" vlandp \
+ 'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1
+
+ local pcap
+ pcap=$(mktemp --suffix=.pcap)
+ on_exit "rm -f $pcap"
+ local tpid2 tlog2
+ start_capture ns2 ns2veth "$pcap" tpid2 tlog2 || return $?
+
+ # ns1veth.10 only accepts tagged frames;
+ # ns2 sends untagged reply → dropped by ns1
+ local ping_rc=0
+ ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \
+ >/dev/null 2>&1 || ping_rc=$?
+ stop_capture "$tpid2" "$tlog2"
+
+ # ping failure is expected (reply path asymmetric)
+ [ "$ping_rc" -ne 0 ] || {
+ info "FAIL: ping succeeded unexpectedly"
+ return 1
+ }
+
+ # assert: no VLAN tag (POP succeeded), untagged ICMP arrived
+ tcpdump -nr "$pcap" 'vlan' 2>/dev/null | grep -q . && {
+ info "FAIL: POP_VLAN: VLAN tag still present"; return 1
+ }
+ tcpdump -nr "$pcap" 'icmp and icmp[icmptype]=8' \
+ 2>/dev/null | grep -q . || {
+ info "FAIL: POP_VLAN: no untagged ICMP echo request"; return 1
+ }
+
+ return 0
+}
+
run_test() {
(
tname="$1"
--
2.53.0
^ permalink raw reply related
* [PATCH net-next v4 1/2] selftests: openvswitch: add vlan() and encap() flow string parsing
From: Minxi Hou @ 2026-05-04 12:37 UTC (permalink / raw)
To: netdev
Cc: aconole, echaudro, i.maximets, davem, edumazet, kuba, pabeni,
horms, shuah, dev, linux-kselftest, Minxi Hou
In-Reply-To: <20260504123713.555461-1-houminxi@gmail.com>
Add VLAN TCI formatting and parsing support to ovs-dpctl.py:
- Add _vlan_dpstr() to decompose TCI into vid/pcp/cfi fields,
with raw tci=0x%04x fallback when cfi=0 for round-trip safety.
- Add _parse_vlan_from_flowstr() boundary check for missing ')'.
- Add encap_ovskey subclass restricting nla_map to L2-L4 attributes
(slots 0-21) that appear inside 802.1Q ENCAP, with metadata
attributes set to "none".
- Check parse() return value for unrecognized trailing content.
- Support callable format functions in dpstr() output.
Signed-off-by: Minxi Hou <houminxi@gmail.com>
---
.../selftests/net/openvswitch/ovs-dpctl.py | 268 +++++++++++++++++-
1 file changed, 260 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
index 848f61fdcee0..285a325fe4d3 100644
--- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
+++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
@@ -901,11 +901,11 @@ class ovskey(nla):
nla_flags = NLA_F_NESTED
nla_map = (
("OVS_KEY_ATTR_UNSPEC", "none"),
- ("OVS_KEY_ATTR_ENCAP", "none"),
+ ("OVS_KEY_ATTR_ENCAP", "encap_ovskey"),
("OVS_KEY_ATTR_PRIORITY", "uint32"),
("OVS_KEY_ATTR_IN_PORT", "uint32"),
("OVS_KEY_ATTR_ETHERNET", "ethaddr"),
- ("OVS_KEY_ATTR_VLAN", "uint16"),
+ ("OVS_KEY_ATTR_VLAN", "be16"),
("OVS_KEY_ATTR_ETHERTYPE", "be16"),
("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"),
("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"),
@@ -1636,6 +1636,205 @@ class ovskey(nla):
class ovs_key_mpls(nla):
fields = (("lse", ">I"),)
+ # 802.1Q CFI (Canonical Format Indicator) bit, always set for Ethernet
+ _VLAN_CFI_MASK = 0x1000
+ _MAX_ENCAP_DEPTH = 4
+ _encap_depth = 0 # single-threaded usage assumed
+
+ @staticmethod
+ def _vlan_dpstr(tci):
+ """Format VLAN TCI as vid=X,pcp=Y,cfi=Z or tci=0xNNNN.
+
+ When cfi=1 (standard Ethernet VLAN), outputs decomposed
+ vid/pcp/cfi fields. When cfi=0 (truncated VLAN header),
+ falls back to raw tci=0x%04x to ensure round-trip
+ correctness: the parser auto-adds cfi=1 for vid/pcp
+ format, so cfi=0 would be lost on re-parse."""
+ vid = tci & 0x0FFF
+ pcp = (tci >> 13) & 0x7
+ cfi = (tci >> 12) & 0x1
+ if cfi:
+ return "vid=%d,pcp=%d,cfi=%d" % (vid, pcp, cfi)
+ return "tci=0x%04x" % tci
+
+ @staticmethod
+ def _parse_vlan_from_flowstr(flowstr):
+ """Parse vlan(tci=X) or vlan(vid=X[,pcp=Y,cfi=Z]) from flowstr.
+
+ Returns (remaining_flowstr, key_tci, mask_tci).
+ TCI values use standard bit layout (VID bits 0-11,
+ CFI bit 12, PCP bits 13-15); byte order conversion to
+ big-endian happens in pyroute2 be16 NLA serialization.
+ The mask covers only the fields the caller specified:
+ vid -> 0x0FFF, pcp -> 0xE000, cfi -> 0x1000, tci -> 0xFFFF.
+
+ The tci= key sets the raw TCI bitfield (no CFI validation) to allow
+ non-Ethernet use cases. Use cfi=1 for standard Ethernet VLAN matching.
+ """
+ tci = 0
+ mask = 0
+ has_tci = False
+ has_vid = has_pcp = has_cfi = False
+ _tci_mix_err = "vlan(): 'tci' cannot be mixed " \
+ "with 'vid'/'pcp'/'cfi'"
+ first = True
+ while True:
+ flowstr = flowstr.lstrip()
+ if not flowstr:
+ raise ValueError("vlan(): missing ')'")
+ if flowstr[0] == ')':
+ break
+ if not first:
+ flowstr = flowstr[1:] # skip ','
+ if not flowstr:
+ raise ValueError("vlan(): missing ')' after trailing comma")
+ flowstr = flowstr.lstrip()
+ if flowstr and flowstr[0] == ')':
+ break
+ if flowstr and flowstr[0] == ',':
+ raise ValueError(
+ "vlan(): empty or extra comma in field list")
+ first = False
+
+ eq = flowstr.find('=')
+ if eq == -1:
+ raise ValueError(
+ "vlan(): expected key=value, got '%s'" % flowstr)
+ key = flowstr[:eq].strip()
+ flowstr = flowstr[eq + 1:]
+
+ end = flowstr.find(',')
+ end2 = flowstr.find(')')
+ if end == -1 and end2 == -1:
+ raise ValueError("vlan(): missing ')'")
+ if end == -1 or (end2 != -1 and end2 < end):
+ end = end2
+ val = flowstr[:end].strip()
+ flowstr = flowstr[end:]
+
+ if not val:
+ raise ValueError("vlan(): empty value for key '%s'" % key)
+ try:
+ v = int(val, 16) if val.startswith(('0x', '0X')) else int(val)
+ except ValueError as exc:
+ raise ValueError(
+ "vlan(): invalid value '%s' for key '%s'"
+ % (val, key)) from exc
+
+ if key == 'tci':
+ if has_tci:
+ raise ValueError("vlan(): duplicate 'tci'")
+ if has_vid or has_pcp or has_cfi:
+ raise ValueError(_tci_mix_err)
+ if v > 0xFFFF or v < 0:
+ raise ValueError("vlan(): tci=0x%x out of range" % v)
+ tci = v
+ mask = 0xFFFF
+ has_tci = True
+ elif key == 'vid':
+ if has_tci:
+ raise ValueError(_tci_mix_err)
+ if has_vid:
+ raise ValueError("vlan(): duplicate 'vid'")
+ if v < 0 or v > 0xFFF:
+ raise ValueError("vlan(): vid=%d out of range (0-4095)" % v)
+ tci |= v
+ mask |= 0x0FFF
+ has_vid = True
+ elif key == 'pcp':
+ if has_tci:
+ raise ValueError(_tci_mix_err)
+ if has_pcp:
+ raise ValueError("vlan(): duplicate 'pcp'")
+ if v < 0 or v > 7:
+ raise ValueError("vlan(): pcp=%d out of range (0-7)" % v)
+ tci |= (v & 0x7) << 13
+ mask |= 0xE000
+ has_pcp = True
+ elif key == 'cfi':
+ if has_tci:
+ raise ValueError(_tci_mix_err)
+ if has_cfi:
+ raise ValueError("vlan(): duplicate 'cfi'")
+ if v != 1:
+ raise ValueError("vlan(): cfi must be 1 for Ethernet")
+ tci |= ovskey._VLAN_CFI_MASK
+ mask |= ovskey._VLAN_CFI_MASK
+ has_cfi = True
+ else:
+ raise ValueError("vlan(): unknown key '%s'" % key)
+
+ flowstr = flowstr[1:] # skip ')'
+ # Catch immediate '))' (user error). A ')' after ',' is consumed
+ # by parse()'s strspn(flowstr, "), ") inter-field separator stripping.
+ if flowstr.lstrip().startswith(')'):
+ raise ValueError("vlan(): unmatched ')'")
+ # parse() strips trailing ',', ')', ' ' as inter-field separators,
+ # so we do not need to call strspn here.
+
+ if mask == 0:
+ raise ValueError("vlan(): no fields specified, "
+ "use vlan(vid=X[,pcp=Y,cfi=Z]) or vlan(tci=X)")
+ if not has_tci:
+ tci |= ovskey._VLAN_CFI_MASK
+ mask |= ovskey._VLAN_CFI_MASK
+ return flowstr, tci, mask
+
+ @staticmethod
+ def _parse_encap_from_flowstr(flowstr):
+ """Parse encap(inner_flow) from flowstr.
+
+ Returns (remaining_flowstr, inner_key_dict, inner_mask_dict)
+ where each dict has an 'attrs' key for recursive NLA encoding.
+ Parenthesis-depth tracking handles nested encap() calls but not
+ quoted strings containing literal parentheses.
+ """
+ if ovskey._encap_depth >= ovskey._MAX_ENCAP_DEPTH:
+ raise ValueError("encap(): max nesting depth %d exceeded" %
+ ovskey._MAX_ENCAP_DEPTH)
+ try:
+ ovskey._encap_depth += 1
+ depth = 1
+ end = -1
+ for i, c in enumerate(flowstr):
+ if c == '(':
+ depth += 1
+ elif c == ')':
+ depth -= 1
+ if depth < 0:
+ raise ValueError(
+ "encap(): unmatched ')' at position %d" % i)
+ if depth == 0:
+ end = i
+ break
+
+ if end == -1:
+ if depth > 1:
+ raise ValueError("encap(): missing ')' at end")
+ raise ValueError("encap(): missing closing ')'")
+
+ inner_str = flowstr[:end].strip()
+ if not inner_str:
+ raise ValueError("encap(): empty inner flow")
+
+ flowstr = flowstr[end + 1:]
+ if flowstr.lstrip().startswith(')'):
+ raise ValueError("encap(): unmatched ')' after encap()")
+ # parse() strips trailing ',', ')', ' ' as inter-field separators,
+ # so we do not need to call strspn here.
+
+ inner_key = encap_ovskey()
+ inner_mask = encap_ovskey()
+ remaining = inner_key.parse(inner_str, inner_mask)
+ if remaining and re.search(r'[^\s,)]', remaining):
+ raise ValueError(
+ "encap(): unrecognized trailing "
+ "content '%s'" % remaining.strip())
+
+ return flowstr, inner_key, inner_mask
+ finally:
+ ovskey._encap_depth -= 1
+
def parse(self, flowstr, mask=None):
for field in (
("OVS_KEY_ATTR_PRIORITY", "skb_priority", intparse),
@@ -1657,6 +1856,16 @@ class ovskey(nla):
"eth_type",
lambda x: intparse(x, "0xffff"),
),
+ (
+ "OVS_KEY_ATTR_VLAN",
+ "vlan",
+ ovskey._parse_vlan_from_flowstr,
+ ),
+ (
+ "OVS_KEY_ATTR_ENCAP",
+ "encap",
+ ovskey._parse_encap_from_flowstr,
+ ),
(
"OVS_KEY_ATTR_IPV4",
"ipv4",
@@ -1794,6 +2003,9 @@ class ovskey(nla):
True,
),
("OVS_KEY_ATTR_ETHERNET", None, None, False, False),
+ ("OVS_KEY_ATTR_VLAN", "vlan", ovskey._vlan_dpstr,
+ lambda x: False, True),
+ ("OVS_KEY_ATTR_ENCAP", None, None, False, False),
(
"OVS_KEY_ATTR_ETHERTYPE",
"eth_type",
@@ -1821,22 +2033,61 @@ class ovskey(nla):
v = self.get_attr(field[0])
if v is not None:
m = None if mask is None else mask.get_attr(field[0])
+ fmt = field[2] # str format or callable
if field[4] is False:
print_str += v.dpstr(m, more)
print_str += ","
else:
if m is None or field[3](m):
- print_str += field[1] + "("
- print_str += field[2] % v
- print_str += "),"
+ val = fmt(v) if callable(fmt) else fmt % v
+ print_str += field[1] + "(" + val + "),"
elif more or m != 0:
- print_str += field[1] + "("
- print_str += (field[2] % v) + "/" + (field[2] % m)
- print_str += "),"
+ if callable(fmt):
+ val = fmt(v) + "/" + fmt(m)
+ else:
+ val = (fmt % v) + "/" + (fmt % m)
+ print_str += field[1] + "(" + val + "),"
return print_str
+class encap_ovskey(ovskey):
+ """Inner flow key attributes valid inside 802.1Q ENCAP.
+
+ Only L2-L4 key attributes (slots 0-21) appear inside ENCAP.
+ Metadata-only attributes (SKB_MARK, DP_HASH, RECIRC_ID, etc.)
+ are set to "none" — they never appear inside ENCAP per
+ ovs_nla_put_vlan() in net/openvswitch/flow_netlink.c.
+
+ nla_map indexes must match OVS_KEY_ATTR_* enum values in
+ include/uapi/linux/openvswitch.h.
+ """
+ nla_map = (
+ ("OVS_KEY_ATTR_UNSPEC", "none"), # 0
+ ("OVS_KEY_ATTR_ENCAP", "none"), # 1 — placeholder, no recursion
+ ("OVS_KEY_ATTR_PRIORITY", "none"), # 2 — skb metadata, not in ENCAP
+ ("OVS_KEY_ATTR_IN_PORT", "none"), # 3 — skb metadata, not in ENCAP
+ ("OVS_KEY_ATTR_ETHERNET", "ethaddr"), # 4
+ ("OVS_KEY_ATTR_VLAN", "be16"), # 5
+ ("OVS_KEY_ATTR_ETHERTYPE", "be16"), # 6
+ ("OVS_KEY_ATTR_IPV4", "ovs_key_ipv4"), # 7
+ ("OVS_KEY_ATTR_IPV6", "ovs_key_ipv6"), # 8
+ ("OVS_KEY_ATTR_TCP", "ovs_key_tcp"), # 9
+ ("OVS_KEY_ATTR_UDP", "ovs_key_udp"), # 10
+ ("OVS_KEY_ATTR_ICMP", "ovs_key_icmp"), # 11
+ ("OVS_KEY_ATTR_ICMPV6", "ovs_key_icmpv6"), # 12
+ ("OVS_KEY_ATTR_ARP", "ovs_key_arp"), # 13
+ ("OVS_KEY_ATTR_ND", "ovs_key_nd"), # 14
+ ("OVS_KEY_ATTR_SKB_MARK", "none"), # 15 — metadata, not in ENCAP
+ ("OVS_KEY_ATTR_TUNNEL", "none"), # 16 — tunnel metadata, not in ENCAP
+ ("OVS_KEY_ATTR_SCTP", "ovs_key_sctp"), # 17
+ ("OVS_KEY_ATTR_TCP_FLAGS", "be16"), # 18
+ ("OVS_KEY_ATTR_DP_HASH", "none"), # 19 — metadata, not in ENCAP
+ ("OVS_KEY_ATTR_RECIRC_ID", "none"), # 20 — metadata, not in ENCAP
+ ("OVS_KEY_ATTR_MPLS", "array(ovs_key_mpls)"), # 21
+ )
+
+
class OvsPacket(GenericNetlinkSocket):
OVS_PACKET_CMD_MISS = 1 # Flow table miss
OVS_PACKET_CMD_ACTION = 2 # USERSPACE action
@@ -2576,6 +2827,7 @@ def print_ovsdp_full(dp_lookup_rep, ifindex, ndb=NDB(), vpl=OvsVport()):
def main(argv):
+ nlmsg_atoms.encap_ovskey = encap_ovskey
nlmsg_atoms.ovskey = ovskey
nlmsg_atoms.ovsactions = ovsactions
--
2.53.0
^ permalink raw reply related
* [PATCH net-next v4 0/2] selftests: openvswitch: add pop_vlan test
From: Minxi Hou @ 2026-05-04 12:37 UTC (permalink / raw)
To: netdev
Cc: aconole, echaudro, i.maximets, davem, edumazet, kuba, pabeni,
horms, shuah, dev, linux-kselftest, Minxi Hou
Add test_pop_vlan() to verify OVS kernel datapath pop_vlan action
correctly strips 802.1Q VLAN tags from frames.
Patch 1 extends ovs-dpctl.py with vlan(vid=X,pcp=Y,cfi=Z) formatting
and parsing, plus an encap_ovskey subclass for safe ENCAP NLA decoding.
Patch 2 adds the selftest with baseline, negative, and positive checks.
Tested with vng on x86_64, all OVS selftests pass (including new
test_pop_vlan).
v4:
- fix all checkpatch line-length warnings in new code
- fix pylint W0707: use explicit exception chaining (from exc)
v3: https://lore.kernel.org/netdev/20260503120946.51869-1-houminxi@gmail.com/
- encap_ovskey: MPLS type "ovs_key_mpls" -> "array(ovs_key_mpls)"
- encap_ovskey: PRIORITY/IN_PORT set to "none" (metadata, not in ENCAP)
- _vlan_dpstr: cfi=0 falls back to tci=0x%04x for round-trip safety
- encap parse(): check return value for unrecognized trailing content
- vlan parser: boundary check + raise-from for exception chaining
- start_capture: || return $? to propagate ksft_skip correctly
- on_exit: moved after resource creation, not before
- ping success: changed from NOTE to FAIL + return 1
- VLAN interface creation: added || return 1 error propagation
- netns probe: distinguish EEXIST from missing CONFIG_NET_NS
- sbx_add: || return $ksft_skip -> || return $? (match sibling tests)
v2: https://lore.kernel.org/netdev/20260501133924.3100680-1-houminxi@gmail.com/
Minxi Hou (2):
selftests: openvswitch: add vlan() and encap() flow string parsing
selftests: openvswitch: add pop_vlan test
.../selftests/net/openvswitch/openvswitch.sh | 196 +++++++++++++
.../selftests/net/openvswitch/ovs-dpctl.py | 268 +++++++++++++++++-
2 files changed, 456 insertions(+), 8 deletions(-)
--
2.53.0
^ permalink raw reply
* Re: [PATCH v2] net: lan966x: avoid unregistering netdev on register failure
From: Andrew Lunn @ 2026-05-04 12:33 UTC (permalink / raw)
To: Myeonghun Pak
Cc: Horatiu Vultur, UNGLinuxDriver, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Ijae Kim, netdev,
linux-kernel
In-Reply-To: <20260502050741.76945-1-mhun512@gmail.com>
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -756,7 +756,7 @@ static void lan966x_cleanup_ports(struct lan966x *lan966x)
> unregister_netdev(port->dev);
>
> lan966x_xdp_port_deinit(port);
> - if (lan966x->fdma && lan966x->fdma_ndev == port->dev)
> + if (lan966x->fdma && port->dev && lan966x->fdma_ndev == port->dev)
> lan966x_fdma_netdev_deinit(lan966x, port->dev);
>
> if (port->phylink) {
Maybe this is better?
port = lan966x->ports[p];
if (!port || port->dev)
continue;
unregister_netdev(port->dev);
Andrew
^ permalink raw reply
* [PATCH 3/3 net-next v3] selftests: net: add test for IPv4 devconf netlink notifications
From: Fernando Fernandez Mancera @ 2026-05-04 12:31 UTC (permalink / raw)
To: netdev
Cc: linux-kselftest, horms, pabeni, kuba, edumazet, davem, idosch,
dsahern, Fernando Fernandez Mancera
In-Reply-To: <20260504123143.6284-1-fmancera@suse.de>
Introduce a new test, `ipv4_devconf_notify`, to verify that the kernel
sends the appropriate netlink notifications when IPv4 devconf parameters
are modified.
Since YNL currently has a bug where it declares an array of u32 values
instead of the nested attributes expected by the kernel for devconf set
operations, a temporary hack (`patched_add_attr`) is included to
pack the netlink attributes correctly.
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v3: added this patch to the series as requested by Paolo.
---
tools/testing/selftests/net/rtnetlink.py | 75 ++++++++++++++++++++++--
1 file changed, 71 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/net/rtnetlink.py b/tools/testing/selftests/net/rtnetlink.py
index e9ad5e88da97..99c5a3e7f1f0 100755
--- a/tools/testing/selftests/net/rtnetlink.py
+++ b/tools/testing/selftests/net/rtnetlink.py
@@ -1,17 +1,22 @@
#!/usr/bin/env python3
# SPDX-License-Identifier: GPL-2.0
-from lib.py import ksft_exit, ksft_run, ksft_ge, RtnlAddrFamily
+from lib.py import bkg, ip, ksft_exit, ksft_run, ksft_ge, ksft_true
+from lib.py import NetNS, NetNSEnter, RtnlAddrFamily, RtnlFamily
import socket
+import struct
+import time
+import types
IPV4_ALL_HOSTS_MULTICAST = b'\xe0\x00\x00\x01'
-def dump_mcaddr_check(rtnl: RtnlAddrFamily) -> None:
+def dump_mcaddr_check() -> None:
"""
Verify that at least one interface has the IPv4 all-hosts multicast address.
At least the loopback interface should have this address.
"""
+ rtnl = RtnlAddrFamily()
addresses = rtnl.getmulticast({"ifa-family": socket.AF_INET}, dump=True)
all_host_multicasts = [
@@ -21,9 +26,71 @@ def dump_mcaddr_check(rtnl: RtnlAddrFamily) -> None:
ksft_ge(len(all_host_multicasts), 1,
"No interface found with the IPv4 all-hosts multicast address")
+def ipv4_devconf_notify() -> None:
+ """
+ Configure an interface and set ipv4-devconf values through netlink
+ to verify that the appropriate netlink notifications are being sent.
+ """
+
+ with NetNS() as ns:
+ with NetNSEnter(str(ns)):
+ rtnl = RtnlFamily()
+
+ ifname = "dummy1"
+ ip(f"link add name {ifname} type dummy", ns=str(ns))
+
+ link_info = ip(f"link show dev {ifname}", ns=str(ns), json=True)
+ ksft_true(bool(link_info), f"Failed to retrieve link info for {ifname}")
+ ifindex = link_info[0]["ifindex"]
+ notification_found = False
+
+ # YNL do not support netconf notifications yet
+ with bkg(f"ip monitor", ns=str(ns)) as cmd_obj:
+ original_add_attr = rtnl._add_attr
+ time.sleep(0.5)
+
+ # Currently YNL has a bug for applying devconf values,
+ # this hack fixes it. In essence, YNL is declaring an
+ # array of u32 values, while kernel expects a nested attribute
+ # on set operation.
+ def patched_add_attr(self, space, name, value, search_attrs):
+ if name == 'conf' and value == b"MAGIC_CONF":
+ fwd_attr = struct.pack("=HHI", 8, 1, 1)
+ proxy_arp_attr = struct.pack("=HHI", 8, 3, 1)
+ rp_filter_attr = struct.pack("=HHI", 8, 8, 1)
+ ignore_routes_attr = struct.pack("=HHI", 8, 29, 1)
+
+ return struct.pack("=HH", 36, 0x8001) + fwd_attr \
+ + proxy_arp_attr \
+ + rp_filter_attr \
+ + ignore_routes_attr
+
+ return original_add_attr(space, name, value, search_attrs)
+
+ rtnl._add_attr = types.MethodType(patched_add_attr, rtnl)
+
+ req = {
+ "ifi-index": ifindex,
+ "af-spec": {
+ "inet": {
+ "conf": b"MAGIC_CONF"
+ }
+ }
+ }
+ rtnl.newlink(req)
+ time.sleep(0.5)
+
+ ksft_true(f"inet {ifname} ignore_routes_with_linkdown on" in cmd_obj.stdout,
+ f"No 'ignore_routes_with_linkdown on' notificiation found for interface {ifname}")
+ ksft_true(f"inet {ifname} rp_filter strict" in cmd_obj.stdout,
+ f"No 'rp_filter strict' notificiation found for interface {ifname}")
+ ksft_true(f"inet {ifname} proxy_neigh on" in cmd_obj.stdout,
+ f"No 'proxy_neigh on' notificiation found for interface {ifname}")
+ ksft_true(f"inet {ifname} forwarding on" in cmd_obj.stdout,
+ f"No 'forwarding on' notificiation found for interface {ifname}")
+
def main() -> None:
- rtnl = RtnlAddrFamily()
- ksft_run([dump_mcaddr_check], args=(rtnl, ))
+ ksft_run([dump_mcaddr_check, ipv4_devconf_notify])
ksft_exit()
if __name__ == "__main__":
--
2.53.0
^ permalink raw reply related
* [PATCH 2/3 net-next v3] ipv4: handle devconf post-set actions on netlink updates
From: Fernando Fernandez Mancera @ 2026-05-04 12:31 UTC (permalink / raw)
To: netdev
Cc: linux-kselftest, horms, pabeni, kuba, edumazet, davem, idosch,
dsahern, Fernando Fernandez Mancera
In-Reply-To: <20260504123143.6284-1-fmancera@suse.de>
When IPv4 device configuration parameters are updated via netlink, the
kernel currently only updates the value. This bypasses several
post-modification actions that occur when these same parameters are
updated via sysctl, such as flushing the routing cache or emitting
RTM_NEWNETCONF notifications.
This patch addresses the inconsistency by calling the
devinet_conf_post_set() helper inside inet_set_link_af(). If a flush is
required, we defer it until the netlink attribute parsing loop
completes.
This ensures consistent behavior and side-effects for devconf changes,
regardless of whether they are initiated via sysctl or netlink.
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: handled forwarding notification and disabling LRO
v3: no changes
---
net/ipv4/devinet.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 8300516fb38f..a35b72662e43 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2161,6 +2161,20 @@ static bool devinet_conf_post_set(struct net *net, struct ipv4_devconf *cnf,
NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
ifindex, cnf);
break;
+ case IPV4_DEVCONF_FORWARDING:
+ if (new == 1) {
+ /* it is safe to use container_of() because forwarding case
+ * is only used by the netlink path
+ */
+ struct in_device *idev = container_of(cnf, struct in_device, cnf);
+
+ netif_disable_lro(idev->dev);
+ }
+
+ inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
+ NETCONFA_FORWARDING,
+ ifindex, cnf);
+ return true;
default:
break;
}
@@ -2173,6 +2187,8 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
{
struct in_device *in_dev = __in_dev_get_rtnl(dev);
struct nlattr *a, *tb[IFLA_INET_MAX+1];
+ struct net *net = dev_net(dev);
+ bool flush_cache = false;
int rem;
if (!in_dev)
@@ -2182,8 +2198,17 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
return -EINVAL;
if (tb[IFLA_INET_CONF]) {
- nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
- ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+ nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+ int old_value = ipv4_devconf_get(in_dev, nla_type(a));
+ int new_value = nla_get_u32(a);
+
+ ipv4_devconf_set(in_dev, nla_type(a), new_value);
+ if (devinet_conf_post_set(net, &in_dev->cnf, nla_type(a), new_value,
+ old_value, dev->ifindex))
+ flush_cache = true;
+ }
+ if (flush_cache)
+ rt_cache_flush(net);
}
return 0;
--
2.53.0
^ permalink raw reply related
* [PATCH 1/3 net-next v3] ipv4: centralize devconf sysctl handling
From: Fernando Fernandez Mancera @ 2026-05-04 12:31 UTC (permalink / raw)
To: netdev
Cc: linux-kselftest, horms, pabeni, kuba, edumazet, davem, idosch,
dsahern, Fernando Fernandez Mancera
The logic for handling IPv4 devconf sysctls is scattered. Notification
and cache flushes are managed in devinet_conf_proc(), while a separate
ipv4_doint_and_flush() function and DEVINET_SYSCTL_FLUSHING_ENTRY macro
is used for properties that solely require a cache flush.
This patch refactors the sysctl handling by introducing a centralized
helper, devinet_conf_post_set(). This new function evaluates the changed
attribute and handles all necessary operations like triggering netlink
notifications. It returns a boolean indicating whether a routing cache
flush is required.
Note that the boolean is necessary as this function will be re-used for
netlink IPv4 devconf handling where the cache flushing must wait until
all the attributes have been processed.
Finally, this is introducing a small change in behavior for
IPV4_DEVCONF_ROUTE_LOCALNET. As commit d0daebc3d622 ("ipv4: Add
interface option to enable routing of 127.0.0.0/8") intended, the cache
flush should only be performed when ROUTE_LOCALNET changes from 1 to 0.
Unfortunately, this was not true because while implementing it the
DEVINET_SYSCTL_FLUSHING_ENTRY was used for the attribute, making the
code related to it on devinet_conf_proc() dead.
IPV4_DEVCONF_FORWARDING is still being handled separately as it requires
more operations.
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: no changes
v3: no changes
---
net/ipv4/devinet.c | 127 ++++++++++++++++++++++++---------------------
1 file changed, 68 insertions(+), 59 deletions(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 58fe7cb69545..8300516fb38f 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -2128,6 +2128,46 @@ static int inet_validate_link_af(const struct net_device *dev,
return 0;
}
+static bool devinet_conf_post_set(struct net *net, struct ipv4_devconf *cnf,
+ int attr, int new, int old, int ifindex)
+{
+ if (new == old)
+ return false;
+
+ switch (attr) {
+ case IPV4_DEVCONF_ROUTE_LOCALNET:
+ case IPV4_DEVCONF_ACCEPT_LOCAL:
+ if (new == 0)
+ return true;
+ break;
+ case IPV4_DEVCONF_NOXFRM:
+ case IPV4_DEVCONF_NOPOLICY:
+ case IPV4_DEVCONF_PROMOTE_SECONDARIES:
+ case IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST:
+ case IPV4_DEVCONF_BC_FORWARDING:
+ return true;
+ case IPV4_DEVCONF_RP_FILTER:
+ inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
+ NETCONFA_RP_FILTER,
+ ifindex, cnf);
+ break;
+ case IPV4_DEVCONF_PROXY_ARP:
+ inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
+ NETCONFA_PROXY_NEIGH,
+ ifindex, cnf);
+ break;
+ case IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN:
+ inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
+ NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
+ ifindex, cnf);
+ break;
+ default:
+ break;
+ }
+
+ return false;
+}
+
static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla,
struct netlink_ext_ack *extack)
{
@@ -2509,44 +2549,31 @@ static int devinet_conf_proc(const struct ctl_table *ctl, int write,
if (write) {
struct ipv4_devconf *cnf = ctl->extra1;
- struct net *net = ctl->extra2;
int i = (int *)ctl->data - cnf->data;
+ struct net *net = ctl->extra2;
int ifindex;
- set_bit(i, cnf->state);
-
- if (cnf == net->ipv4.devconf_dflt)
- devinet_copy_dflt_conf(net, i);
- if (i == IPV4_DEVCONF_ACCEPT_LOCAL - 1 ||
- i == IPV4_DEVCONF_ROUTE_LOCALNET - 1)
- if ((new_value == 0) && (old_value != 0))
- rt_cache_flush(net);
+ /* These attributes are bypassing the tracking state,
+ * for the rest track the state and propagate the changes
+ * to default config
+ */
+ switch (i + 1) {
+ case IPV4_DEVCONF_NOXFRM:
+ case IPV4_DEVCONF_NOPOLICY:
+ case IPV4_DEVCONF_PROMOTE_SECONDARIES:
+ case IPV4_DEVCONF_DROP_UNICAST_IN_L2_MULTICAST:
+ break;
+ default:
+ set_bit(i, cnf->state);
+ if (cnf == net->ipv4.devconf_dflt)
+ devinet_copy_dflt_conf(net, i);
+ break;
+ }
- if (i == IPV4_DEVCONF_BC_FORWARDING - 1 &&
- new_value != old_value)
+ ifindex = devinet_conf_ifindex(net, cnf);
+ if (devinet_conf_post_set(net, cnf, i + 1, new_value,
+ old_value, ifindex))
rt_cache_flush(net);
-
- if (i == IPV4_DEVCONF_RP_FILTER - 1 &&
- new_value != old_value) {
- ifindex = devinet_conf_ifindex(net, cnf);
- inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
- NETCONFA_RP_FILTER,
- ifindex, cnf);
- }
- if (i == IPV4_DEVCONF_PROXY_ARP - 1 &&
- new_value != old_value) {
- ifindex = devinet_conf_ifindex(net, cnf);
- inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
- NETCONFA_PROXY_NEIGH,
- ifindex, cnf);
- }
- if (i == IPV4_DEVCONF_IGNORE_ROUTES_WITH_LINKDOWN - 1 &&
- new_value != old_value) {
- ifindex = devinet_conf_ifindex(net, cnf);
- inet_netconf_notify_devconf(net, RTM_NEWNETCONF,
- NETCONFA_IGNORE_ROUTES_WITH_LINKDOWN,
- ifindex, cnf);
- }
}
return ret;
@@ -2599,20 +2626,6 @@ static int devinet_sysctl_forward(const struct ctl_table *ctl, int write,
return ret;
}
-static int ipv4_doint_and_flush(const struct ctl_table *ctl, int write,
- void *buffer, size_t *lenp, loff_t *ppos)
-{
- int *valp = ctl->data;
- int val = *valp;
- int ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
- struct net *net = ctl->extra2;
-
- if (write && *valp != val)
- rt_cache_flush(net);
-
- return ret;
-}
-
#define DEVINET_SYSCTL_ENTRY(attr, name, mval, proc) \
{ \
.procname = name, \
@@ -2633,9 +2646,6 @@ static int ipv4_doint_and_flush(const struct ctl_table *ctl, int write,
#define DEVINET_SYSCTL_COMPLEX_ENTRY(attr, name, proc) \
DEVINET_SYSCTL_ENTRY(attr, name, 0644, proc)
-#define DEVINET_SYSCTL_FLUSHING_ENTRY(attr, name) \
- DEVINET_SYSCTL_COMPLEX_ENTRY(attr, name, ipv4_doint_and_flush)
-
static struct devinet_sysctl_table {
struct ctl_table_header *sysctl_header;
struct ctl_table devinet_vars[IPV4_DEVCONF_MAX];
@@ -2678,15 +2688,14 @@ static struct devinet_sysctl_table {
"ignore_routes_with_linkdown"),
DEVINET_SYSCTL_RW_ENTRY(DROP_GRATUITOUS_ARP,
"drop_gratuitous_arp"),
-
- DEVINET_SYSCTL_FLUSHING_ENTRY(NOXFRM, "disable_xfrm"),
- DEVINET_SYSCTL_FLUSHING_ENTRY(NOPOLICY, "disable_policy"),
- DEVINET_SYSCTL_FLUSHING_ENTRY(PROMOTE_SECONDARIES,
- "promote_secondaries"),
- DEVINET_SYSCTL_FLUSHING_ENTRY(ROUTE_LOCALNET,
- "route_localnet"),
- DEVINET_SYSCTL_FLUSHING_ENTRY(DROP_UNICAST_IN_L2_MULTICAST,
- "drop_unicast_in_l2_multicast"),
+ DEVINET_SYSCTL_RW_ENTRY(NOXFRM, "disable_xfrm"),
+ DEVINET_SYSCTL_RW_ENTRY(NOPOLICY, "disable_policy"),
+ DEVINET_SYSCTL_RW_ENTRY(PROMOTE_SECONDARIES,
+ "promote_secondaries"),
+ DEVINET_SYSCTL_RW_ENTRY(ROUTE_LOCALNET,
+ "route_localnet"),
+ DEVINET_SYSCTL_RW_ENTRY(DROP_UNICAST_IN_L2_MULTICAST,
+ "drop_unicast_in_l2_multicast"),
},
};
--
2.53.0
^ permalink raw reply related
* Re: [PATCH net-next v2 1/2] dpll: move fractional-frequency-offset-ppt under pin-parent-device
From: Jiri Pirko @ 2026-05-04 12:26 UTC (permalink / raw)
To: Ivan Vecera
Cc: netdev, Andrew Lunn, Arkadiusz Kubalewski, David S. Miller,
Donald Hunter, Eric Dumazet, Jakub Kicinski, Jonathan Corbet,
Leon Romanovsky, Mark Bloch, Michal Schmidt, Paolo Abeni,
Pasi Vaananen, Petr Oros, Prathosh Satish, Saeed Mahameed,
Shuah Khan, Simon Horman, Tariq Toukan, Vadim Fedorenko,
linux-doc, linux-kernel, linux-rdma
In-Reply-To: <290673a1-fb5b-4586-b44a-e109cc1a4629@redhat.com>
Mon, May 04, 2026 at 11:36:19AM +0200, ivecera@redhat.com wrote:
>Hi Jiri,
>
>On 5/4/26 10:48 AM, Jiri Pirko wrote:
>> Thu, Apr 30, 2026 at 07:36:10PM +0200, ivecera@redhat.com wrote:
>> > Move the fractional-frequency-offset-ppt attribute from the top-level
>> > pin attributes into the pin-parent-device nested attribute set. This
>> > makes it consistent with phase-offset which is already per-parent and
>> > clarifies that FFO PPT represents the frequency difference between
>> > a pin and its parent DPLL device.
>> >
>> > The top-level fractional-frequency-offset attribute (in PPM) remains
>> > unchanged for backward compatibility.
>>
>> That is odd. The ppt one was added just for higher precision but was
>> semantically the same. Now you change it. Could you still treat both the
>> same?
>>
>WDYM?
>
>Keep fractional-frequency-offset-ppt at the top-level and add both
>fractional-frequency-offset and fractional-frequency-offset-ppt into
>pin-parent-device nested attribute set?
Since both are the same, only different unit, it would make sense to
treat them both the same. That prevents from user confusion, hopefully.
>
>Thanks,
>Ivan
>
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH iwl-net] ice: fix missing priority callbacks for U.FL DPLL pins
From: Loktionov, Aleksandr @ 2026-05-04 12:23 UTC (permalink / raw)
To: Oros, Petr, netdev@vger.kernel.org
Cc: Kitszel, Przemyslaw, Eric Dumazet, Kubalewski, Arkadiusz,
Andrew Lunn, Nguyen, Anthony L, Simon Horman,
intel-wired-lan@lists.osuosl.org, Jakub Kicinski, Paolo Abeni,
David S. Miller, linux-kernel@vger.kernel.org
In-Reply-To: <20260504121603.1702674-1-poros@redhat.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Petr Oros
> Sent: Monday, May 4, 2026 2:16 PM
> To: netdev@vger.kernel.org
> Cc: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Eric Dumazet
> <edumazet@google.com>; Kubalewski, Arkadiusz
> <arkadiusz.kubalewski@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Simon Horman
> <horms@kernel.org>; intel-wired-lan@lists.osuosl.org; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S. Miller
> <davem@davemloft.net>; linux-kernel@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH iwl-net] ice: fix missing priority
> callbacks for U.FL DPLL pins
>
> The U.FL2 input pin advertises
> DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE
> in its capability mask, but ice_dpll_pin_ufl_ops does not provide
> .prio_get and .prio_set callbacks. As a result the DPLL subsystem
> cannot report or accept priority for U.FL pins: pin-get omits the prio
> field on U.FL2 and pin-set with prio is rejected as invalid, even
> though the capability is present. This prevents user space from using
> priority to select or disable U.FL2 as a DPLL input source.
>
> Add the missing .prio_get and .prio_set callbacks to
> ice_dpll_pin_ufl_ops, reusing ice_dpll_sw_input_prio_{get,set}. The
> same ops struct is shared by U.FL1 and U.FL2: U.FL2 (input) delegates
> to the backing hardware input pin, while U.FL1 (output) does not
> advertise DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE so the dpll core
> capability gate never invokes the callback for it. The reused helpers
> also guard on p->direction != DPLL_PIN_DIRECTION_INPUT and !p->input
> as defense in depth.
>
> Fixes: 2dd5d03c77e2 ("ice: redesign dpll sma/u.fl pins control")
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_dpll.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
> b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 27b460926baced..be72a076f7a15c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -2628,6 +2628,8 @@ static const struct dpll_pin_ops
> ice_dpll_pin_ufl_ops = {
> .state_on_dpll_set = ice_dpll_ufl_pin_state_set,
> .state_on_dpll_get = ice_dpll_sw_pin_state_get,
> .direction_get = ice_dpll_pin_sw_direction_get,
> + .prio_get = ice_dpll_sw_input_prio_get,
> + .prio_set = ice_dpll_sw_input_prio_set,
> .frequency_get = ice_dpll_sw_pin_frequency_get,
> .frequency_set = ice_dpll_sw_pin_frequency_set,
> .esync_set = ice_dpll_sw_esync_set,
> --
> 2.53.0
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix missing priority callbacks for U.FL DPLL pins
From: Paul Menzel @ 2026-05-04 12:21 UTC (permalink / raw)
To: Petr Oros
Cc: netdev, Przemek Kitszel, Eric Dumazet, Arkadiusz Kubalewski,
Andrew Lunn, Tony Nguyen, Simon Horman, intel-wired-lan,
Jakub Kicinski, Paolo Abeni, David S. Miller, linux-kernel
In-Reply-To: <20260504121603.1702674-1-poros@redhat.com>
Dear Petr,
Thank you for your patch.
Am 04.05.26 um 14:16 schrieb Petr Oros:
> The U.FL2 input pin advertises DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE
> in its capability mask, but ice_dpll_pin_ufl_ops does not provide
> .prio_get and .prio_set callbacks. As a result the DPLL subsystem
> cannot report or accept priority for U.FL pins: pin-get omits the prio
> field on U.FL2 and pin-set with prio is rejected as invalid, even
> though the capability is present. This prevents user space from using
> priority to select or disable U.FL2 as a DPLL input source.
>
> Add the missing .prio_get and .prio_set callbacks to
> ice_dpll_pin_ufl_ops, reusing ice_dpll_sw_input_prio_{get,set}. The
> same ops struct is shared by U.FL1 and U.FL2: U.FL2 (input) delegates
> to the backing hardware input pin, while U.FL1 (output) does not
> advertise DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE so the dpll core
> capability gate never invokes the callback for it. The reused helpers
> also guard on p->direction != DPLL_PIN_DIRECTION_INPUT and !p->input
> as defense in depth.
Should you resend, it’d be great if you listed the user space commands
to reproduce this.
> Fixes: 2dd5d03c77e2 ("ice: redesign dpll sma/u.fl pins control")
> Signed-off-by: Petr Oros <poros@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_dpll.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 27b460926baced..be72a076f7a15c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -2628,6 +2628,8 @@ static const struct dpll_pin_ops ice_dpll_pin_ufl_ops = {
> .state_on_dpll_set = ice_dpll_ufl_pin_state_set,
> .state_on_dpll_get = ice_dpll_sw_pin_state_get,
> .direction_get = ice_dpll_pin_sw_direction_get,
> + .prio_get = ice_dpll_sw_input_prio_get,
> + .prio_set = ice_dpll_sw_input_prio_set,
> .frequency_get = ice_dpll_sw_pin_frequency_get,
> .frequency_set = ice_dpll_sw_pin_frequency_set,
> .esync_set = ice_dpll_sw_esync_set,
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH net 06/12] netfilter: nf_conntrack_expect: honor expectation helper field
From: Ilya Maximets @ 2026-05-04 12:19 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: i.maximets, netfilter-devel, fw, davem, netdev, kuba, pabeni,
edumazet, horms, Eelco Chaudron, Aaron Conole
In-Reply-To: <afSCXEg-X-ieL9cY@chamomile>
On 5/1/26 12:37 PM, Pablo Neira Ayuso wrote:
> Hi Ilya,
>
> On Thu, Apr 30, 2026 at 10:58:38PM +0200, Ilya Maximets wrote:
>> On 3/26/26 1:51 PM, Pablo Neira Ayuso wrote:
>>> The expectation helper field is mostly unused. As a result, the
>>> netfilter codebase relies on accessing the helper through exp->master.
>>>
>>> Always set on the expectation helper field so it can be used to reach
>>> the helper.
>>>
>>> nf_ct_expect_init() is called from packet path where the skb owns
>>> the ct object, therefore accessing exp->master for the newly created
>>> expectation is safe. This saves a lot of updates in all callsites
>>> to pass the ct object as parameter to nf_ct_expect_init().
>>>
>>> This is a preparation patches for follow up fixes.
>>>
>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>>> ---
>>
>> Hi, Pablo and Florian.
>>
>> I was investigating FTP test failures in OVS with 7.0 kernel and bisected
>> the issue down to this commit. AFAIU, with this change all the related
>> connections over time gain their parents' helpers,. This is causing a change
>> visible to the userspace, because FTP data connections are now reported to
>> have helpers in the conntrack dump:
>>
>> # conntrack -L
>> tcp 6 119 TIME_WAIT src=10.1.1.1 dst=10.1.1.2 sport=59534 dport=21 \
>> src=10.1.1.2 dst=10.1.1.1 sport=21 dport=59534 \
>> [ASSURED] mark=0 helper=ftp use=2
>> tcp 6 119 TIME_WAIT src=10.1.1.2 dst=10.1.1.1 sport=52709 dport=52381 \
>> src=10.1.1.1 dst=10.1.1.2 sport=52381 dport=52709 \
>> [ASSURED] mark=0 helper=ftp use=1
>>
>> Before this commit only the control connection had helper=ftp reported in
>> the dump. The traffic seems to work fine, but our tests fail because we
>> do not expect the helper attached.
>>
>> AFAIU, it's generally not something that should be happening, as helpers
>> on data connections do not really make much sense. But I'm just trying to
>> figure out if you would consider this as a regression and fix in the kernel
>> or if we should adjust our userspace components for this new dump content,
>> which would not be very straightforward to do if we want to be able to run
>> tests on both old and the new versions.
>>
>> What do you think?
>
> It seems previous behaviour to 9c42bc9db90a was inconsistent, ie. only
> the h323 helper sets on exp->helper, then it shows helper= in expected
> connections via ctnetlink. I guess this is for debugging given that
> h323 is actually a family of helpers.
>
> To consistently skip dumping this for expected connections, probably
> this is the way to do:
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conn
> index eda5fe4a75c8..9491ae9e080e 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -226,7 +226,7 @@ static int ctnetlink_dump_helpinfo(struct sk_buff *sk
> const struct nf_conn_help *help = nfct_help(ct);
> struct nf_conntrack_helper *helper;
>
> - if (!help)
> + if (!help || ct->status & IPS_EXPECTED)
> return 0;
>
> rcu_read_lock();
I'm not sure. I tried this change and it fixed one case but broke another.
Looking at what we're testing, the old behavior (at least for FTP) was:
"if helper was committed - report it, if not - don't". i.e. it's not really
about the connection being expected it's about if the user committed the
helper for the connection or not.
Let me explain a few scenarios that we have in the OVS system tests and what
I see with the old kernel (6.19), the new (7.0) and the patch above.
A) The first scenario has the following OpenFlow rules (simplified):
table=0,in_port=1,tcp,action=ct(alg=ftp,commit),2
table=0,in_port=2,tcp,action=ct(table=1)
table=1,in_port=2,tcp,ct_state=+trk+est,action=1
table=1,in_port=2,tcp,ct_state=+trk+rel,action=1
This set of rule blindly commits every packet coming from port 1 with the
helper and sends to port 2. Packets from port 2 are passed through ct and
only related or established traffic is passed to port 1. This is a very
rudimentary setup that users can make to allow ftp from port 1 towards port 2,
but not in the opposite direction.
For this scenario regardless of the kernel version or the patch above I see
that both the data and the control connections have a helper reported in the
ctnetlink dump.
B) The second scenario:
table=0,in_port=1,tcp,action=ct(table=1)
table=1,in_port=1,tcp,ct_state=+trk+new,action=ct(commit,alg=ftp),2
table=1,in_port=1,tcp,ct_state=+trk+est,action=2
table=0,in_port=2,tcp,action=ct(table=1)
table=1,in_port=2,tcp,ct_state=+trk+new+rel,action=ct(commit),1
table=1,in_port=2,tcp,ct_state=+trk+est,action=1
This is a more reasonable setup where new connections are committed on the
way from 1 to 2 and new related connections are committed on the way from
2 to 1. This allows port 1 to initiate the control connection and the port
2 to initiate the related data connection.
In case of active FTP (port 1 initiates control, port 2 initiates the data):
- old: control has the helper, data does not.
- new: both have the helper.
- patch: control has the helper, data does not.
In case of passive FTP (port 1 initiates both the control and data):
- old: both have the helper (because both are +new traffic from port 1).
- new: both have the helper.
- patch: control has the helper, data does not.
C) We can modify the scenario B to avoid committing the helper on related:
table=0,in_port=1,tcp,action=ct(table=1)
table=1,in_port=1,tcp,ct_state=+trk+new-rel,action=ct(commit,alg=ftp),2
table=1,in_port=1,tcp,ct_state=+trk+new+rel,action=ct(commit),2
table=1,in_port=1,tcp,ct_state=+trk+est,action=2
table=0,in_port=2,tcp,action=ct(table=1)
table=1,in_port=2,tcp,ct_state=+trk+new+rel,action=ct(commit),1
table=1,in_port=2,tcp,ct_state=+trk+est,action=1
Here we have (same for active and passive):
- old: control has the helper, data does not.
- new: both have the helper.
- patch: control has the helper, data does not.
I hope that clarifies the situation a little bit.
So, if we want to restore the old behavior, the we'd probably need to track
how connection gained the helper, i.e. was it via commit or was it inherited.
I'm also not sure why we see the helper with the patch above in scenario A that
commits established traffic, but not in B or C that only commits new traffic.
Best regards, Ilya Maximets.
^ permalink raw reply
* [PATCH iwl-net] ice: fix missing priority callbacks for U.FL DPLL pins
From: Petr Oros @ 2026-05-04 12:16 UTC (permalink / raw)
To: netdev
Cc: Petr Oros, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Arkadiusz Kubalewski, intel-wired-lan, linux-kernel
The U.FL2 input pin advertises DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE
in its capability mask, but ice_dpll_pin_ufl_ops does not provide
.prio_get and .prio_set callbacks. As a result the DPLL subsystem
cannot report or accept priority for U.FL pins: pin-get omits the prio
field on U.FL2 and pin-set with prio is rejected as invalid, even
though the capability is present. This prevents user space from using
priority to select or disable U.FL2 as a DPLL input source.
Add the missing .prio_get and .prio_set callbacks to
ice_dpll_pin_ufl_ops, reusing ice_dpll_sw_input_prio_{get,set}. The
same ops struct is shared by U.FL1 and U.FL2: U.FL2 (input) delegates
to the backing hardware input pin, while U.FL1 (output) does not
advertise DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE so the dpll core
capability gate never invokes the callback for it. The reused helpers
also guard on p->direction != DPLL_PIN_DIRECTION_INPUT and !p->input
as defense in depth.
Fixes: 2dd5d03c77e2 ("ice: redesign dpll sma/u.fl pins control")
Signed-off-by: Petr Oros <poros@redhat.com>
---
drivers/net/ethernet/intel/ice/ice_dpll.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 27b460926baced..be72a076f7a15c 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -2628,6 +2628,8 @@ static const struct dpll_pin_ops ice_dpll_pin_ufl_ops = {
.state_on_dpll_set = ice_dpll_ufl_pin_state_set,
.state_on_dpll_get = ice_dpll_sw_pin_state_get,
.direction_get = ice_dpll_pin_sw_direction_get,
+ .prio_get = ice_dpll_sw_input_prio_get,
+ .prio_set = ice_dpll_sw_input_prio_set,
.frequency_get = ice_dpll_sw_pin_frequency_get,
.frequency_set = ice_dpll_sw_pin_frequency_set,
.esync_set = ice_dpll_sw_esync_set,
--
2.53.0
^ permalink raw reply related
* Re: [PATCH net-next 5/5] dt-bindings: net: Add bindings for the ADIN1140
From: Andrew Lunn @ 2026-05-04 12:11 UTC (permalink / raw)
To: Regus, Ciprian
Cc: Parthiban Veerasooran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Shuah Khan, Heiner Kallweit, Russell King, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
devicetree@vger.kernel.org
In-Reply-To: <2de08ad6ba73477299b6aace38b6de4b@analog.com>
> > > + ethernet@0 {
> > > + compatible = "adi,adin1140";
> > > + reg = <0>;
> > > + spi-max-frequency = <23000000>;
> > > +
> > > + interrupt-parent = <&gpio>;
> > > + interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
> >
> > Table 1: OPEN serial 10BASE-T1x Interface Pin Definition
> >
> > IRQn MAC-PHY Interrupt Request (Active Low)
> >
> > Or is this something else which the device gets wrong?
>
> The device generates interrupts correctly (the IRQ signal remains
> asserted while there are active interrupt conditions that have not
> been cleared yet). The oa_tc6 driver requests the interrupt with
> the IRQF_TRIGGER_FALLING flag set
Ah, that is a bug in oa_tc6.c. A falling edge appears to work, until
it does not and then all interrupts stop. So bugs like this are not
obvious. I've been looking out for this more over the last few years
since PHYs are level, not edge, but many developers get them wrong in
DT.
Please could you submit a patch to net to fix this?
Andrew
^ permalink raw reply
* RE: [Intel-wired-lan] [PATCH net] ice: fix PTP hang for E825C devices
From: Rinitha, SX @ 2026-05-04 12:11 UTC (permalink / raw)
To: Loktionov, Aleksandr, intel-wired-lan@lists.osuosl.org,
Nguyen, Anthony L, Loktionov, Aleksandr
Cc: netdev@vger.kernel.org
In-Reply-To: <20260327072332.130320-4-aleksandr.loktionov@intel.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Aleksandr Loktionov
> Sent: 27 March 2026 12:53
> To: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH net] ice: fix PTP hang for E825C devices
>
> From: Grzegorz Nitka <grzegorz.nitka@intel.com>
>
> Change the order of PTP reconfiguration when port goes down or up (ice_down and ice_up calls) to be more graceful and consistent from timestamp interrupts processing perspective.
>
> For both calls (ice_up and ice_down), accompanying ice_ptp_link_change is called which starts/stops PTP timer. This patch changes the order:
> - while link goes down: disable net device Tx first (netif_carrier_off,
> netif_tx_disable), then call ice_ptp_link_change
> - while link goes up: ice_ptp_link_change called first, then re-enable
> net device Tx (netif_tx_start_all_queues)
>
> Otherwise, there is a narrow window in which PTP timestamp request has been triggered and timestamp processing occurs when PTP timer is not enabled yet (up case) or already disabled (down case). This may lead > to undefined behavior and receiving invalid timestamps. This case was observed on E825C devices only.
>
> Fixes: 6b1ff5d39228 ("ice: always call ice_ptp_link_change and make it void")
> Cc: stable@vger.kernel.org
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>
> drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply
* Re: [PATCH v4 04/15] firmware: qcom: Add a PAS TEE service
From: Harshal Dev @ 2026-05-04 11:52 UTC (permalink / raw)
To: Sumit Garg
Cc: linux-arm-msm, devicetree, dri-devel, freedreno, linux-media,
netdev, linux-wireless, ath12k, linux-remoteproc, robh, krzk+dt,
conor+dt, robin.clark, sean, akhilpo, lumag, abhinav.kumar,
jesszhan0024, marijn.suijten, airlied, simona, vikash.garodia,
dikshita.agarwal, bod, mchehab, elder, andrew+netdev, davem,
edumazet, kuba, pabeni, jjohnson, mathieu.poirier,
trilokkumar.soni, mukesh.ojha, pavan.kondeti, jorge.ramirez,
tonyh, vignesh.viswanathan, srinivas.kandagatla, amirreza.zarrabi,
op-tee, apurupa, skare, linux-kernel, Sumit Garg, Bjorn Andersson,
Konrad Dybcio
In-Reply-To: <afiCrIYSm8AK9xn9@sumit-xelite>
On 5/4/2026 4:57 PM, Sumit Garg wrote:
> On Mon, May 04, 2026 at 03:33:06PM +0530, Harshal Dev wrote:
>> Hi Sumit,
>>
>> On 4/27/2026 3:25 PM, Sumit Garg via OP-TEE wrote:
>>> From: Sumit Garg <sumit.garg@oss.qualcomm.com>
>>>
>>> Add support for Peripheral Authentication Service (PAS) driver based
>>> on TEE bus with OP-TEE providing the backend PAS service implementation.
>>>
>>> The TEE PAS service ABI is designed to be extensible with additional API
>>> as PTA_QCOM_PAS_CAPABILITIES. This allows to accommodate any future
>>> extensions of the PAS service needed while still maintaining backwards
>>> compatibility.
>>>
>>> Signed-off-by: Sumit Garg <sumit.garg@oss.qualcomm.com>
>>> ---
>>> drivers/firmware/qcom/Kconfig | 10 +
>>> drivers/firmware/qcom/Makefile | 1 +
>>> drivers/firmware/qcom/qcom_pas_tee.c | 479 +++++++++++++++++++++++++++
>>> 3 files changed, 490 insertions(+)
>>> create mode 100644 drivers/firmware/qcom/qcom_pas_tee.c
>>
>> [...]
>>
>>> diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
>>
>>> +static int qcom_pas_tee_mem_setup(struct device *dev, u32 pas_id,
>>> + phys_addr_t addr, phys_addr_t size)
>>> +{
>>
>> [...]
>>
>>> +
>>> + ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
>>> + if (ret < 0 || inv_arg.ret != 0) {
>>> + dev_err(dev, "PAS mem setup failed, pas_id: %d, ret: %d, err: 0x%x\n",
>>> + pas_id, ret, inv_arg.ret);
>>> + return ret ?: -EINVAL;
>>
>> Following the example from qcom_scm_pas_mem_setup() here:
>> https://elixir.bootlin.com/linux/v7.0.1/source/drivers/firmware/qcom/qcom_scm.c#L778
>>
>> I think it should be:
>> return ret ?: inv_arg.ret;
>
> inv_arg.ret return a GP TEE error code which doesn't map 1:1 to kernel
> error codes. The client drivers won't benefit without having a way to
> decode those errors. So just printing here can help the user to debug
> any issues.
>
>>
>> This way the client (say mdt_loader) knows what error OPTEE PAS service returned.
>> I can see that is tries to print this error:
>> https://elixir.bootlin.com/linux/v7.0.1/source/drivers/soc/qcom/mdt_loader.c#L286
>>
>> With that,
>> Reviewed-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>
>
> Thanks, I hope with above I can keep your tag.
>
Alright sure, makes sense. Please keep the tag.
Regards,
Harshal
> -Sumit
^ permalink raw reply
* Re: [PATCH v7 phy-next 09/27] scsi: ufs: exynos: stop poking into struct phy guts
From: Vladimir Oltean @ 2026-05-04 11:48 UTC (permalink / raw)
To: linux-phy
Cc: Vinod Koul, Neil Armstrong, dri-devel, freedreno,
linux-arm-kernel, linux-arm-msm, linux-can, linux-gpio, linux-ide,
linux-kernel, linux-media, linux-pci, linux-renesas-soc,
linux-riscv, linux-rockchip, linux-samsung-soc, linux-scsi,
linux-sunxi, linux-tegra, linux-usb, netdev, spacemit,
UNGLinuxDriver, Bart Van Assche, Alim Akhtar, Martin K . Petersen,
Peter Griffin, James E.J. Bottomley, Krzysztof Kozlowski,
Chanho Park
In-Reply-To: <20260430110652.558622-10-vladimir.oltean@nxp.com>
[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]
Sashiko says:
> commit 76ded6c0290bdb192c2ae1af9a2e6a47fc39c9a1
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> scsi: ufs: exynos: stop poking into struct phy guts
> This commit modifies the Exynos UFS driver to stop directly accessing the
> generic PHY's power_count. Instead, it tracks the PHY power state internally
> using a new phy_powered_on flag to properly handle link retries and
> suspend/resume operations.
>
> > diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> > --- a/drivers/ufs/host/ufs-exynos.c
> > +++ b/drivers/ufs/host/ufs-exynos.c
> [ ... ]
> > @@ -1513,6 +1516,9 @@ static void exynos_ufs_exit(struct ufs_hba *hba)
> > {
> > struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> >
> > + if (!ufs->phy_powered_on)
> > + return;
> > +
> > phy_power_off(ufs->phy);
> > phy_exit(ufs->phy);
> > }
>
> Does this introduce a regression where initialization resources are leaked
> if the driver is unbound while suspended?
> Looking at exynos_ufs_suspend(), the PHY is powered off and phy_powered_on
> is set to false, but phy_exit() is intentionally not called so the PHY
> remains initialized:
> exynos_ufs_suspend()
> if (!ufshcd_is_link_active(hba) && ufs->phy_powered_on) {
> phy_power_off(ufs->phy);
> ufs->phy_powered_on = false;
> }
> If an unbind or fatal error occurs while in this suspended state,
> exynos_ufs_exit() will see phy_powered_on as false and return immediately.
> The initialization reference and any resources allocated by phy_init()
> would never be released since phy_exit() is completely skipped.
> [ ... ]
I think I've been looking at this issue all wrong, not considering why
the UFS HCD driver calls phy_exit() -> phy_init() at runtime and what
can be done about that.
I have replaced this patch with the one attached, which I will be sending
for v8.
[-- Attachment #2: 0001-scsi-ufs-exynos-use-dedicated-API-for-updating-PHY-b.patch --]
[-- Type: text/x-diff, Size: 9438 bytes --]
From c687a8568c6c7837bb0bd539bf14343d7d0c63a1 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Mon, 4 May 2026 14:00:51 +0300
Subject: [PATCH] scsi: ufs: exynos: use dedicated API for updating PHY bus
width
I am trying to get rid of code instances where PHY consumers (like the
Exynos UFS HCD) poke inside struct phy fields, in order to further turn
struct phy into an opaque data structure.
The ufs-exynos.c driver interacts with phy-samsung-ufs.c in order to
power it on and to update the lane count. For the later purpose, it
(ab)uses phy_set_bus_width().
The phy_set_bus_width() function is a PHY provider function, not a
consumer one, and I am calling its use from ufs-exynos.c an abuse
because
(1) commit 8feed347d33b ("phy: add phy_get_bus_width()/phy_set_bus_width()
calls") clearly states so.
(2) phy_set_bus_width() only alters phy->attrs.bus_width, and does not
call into phy_ops at all. So a consumer that makes a call to
phy_set_bus_width() will not produce any hardware change in the
provider at all.
This is where the Exynos UFS HCD driver decided to be creative and
hijacked phy_init() to pick up the bus_width attribute.
This requires a very careful dance where the PHY consumer needs to
simultaneously juggle two requirements:
- the UFS PHY needs to pick up the updated lane count in its
samsung_ufs_phy_init() handler for the phy_init() call
- phy_init() calls need to be balanced with phy_exit(), otherwise
subsequent phy_init() calls don't make it into samsung_ufs_phy_init()
and just leave the PHY with an elevated init_count
- phy_power_on() can't be called without phy_init()
This is why the following bug fix commits exist:
3d73b200f989 ("scsi: ufs: ufs-exynos: Change ufs phy control sequence")
7f05fd9a3b6f ("scsi: ufs: exynos: Ensure consistent phy reference counts")
Currently the UFS HCD driver tries to keep the PHY init_count and
power_count in tight lockstep, but even this is error-prone. For
example, if exynos_ufs_suspend() runs and then exynos_ufs_exit(),
the PHY power_count will underflow.
If we address the root issue first (phy_init() abused to pick up new
lane count) by introducing a new PHY consumer method which actually does
call into the PHY provider driver, then we are able to absorb the entire
UFS HCD dance and update the lane count without altering the PHY
init_count or power_count.
Then we are much more free to call phy_init() from wherever we want, and
same goes for phy_power_on().
It is typical to call phy_init() right after phy_get(), and doing so
will naturally balance it with phy_exit().
We can also leave the phy_power_on() call to be on demand, placed inside
exynos_ufs_pre_link(). Because this call can be made multiple times and
is not balanced with anything else, we need a consumer-specific "bool
phy_powered_on" which ensures that we call phy_power_on() at most once,
and that exynos_ufs_exit() only calls phy_power_off() if phy_power_on()
was previously called. Using the phy->power_count for this purpose is
undesirable because
(a) it is going away
(b) the PHY API supports multiple consumers for the same provider, so it
cannot offer an equivalent helper because it doesn't want consumers
to interfere with each other
Inside the new samsung_ufs_phy_request_bus_width(), I've sanity checked
that the bus width is either 1 or 2 lanes. This coincides with
samsung_ufs_phy_config() which only configures LANE_0 and LANE_1.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Cc: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Peter Griffin <peter.griffin@linaro.org>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Chanho Park <chanho61.park@samsung.com>
v7->v8:
- rewrote commit after Sashiko pointed out the new handling is still
not correct:
https://sashiko.dev/#/patchset/20260430110652.558622-1-vladimir.oltean@nxp.com
- removed Reviewed-by, Tested-by and Acked-by tags from Alim, Bart and
Peter
v6->v7: collect tags from Martin and Peter
v5->v6: collect tags from Alim Akhtar
v4->v5: collect tag, add "scsi: " prefix to commit title
v3->v4: none
v2->v3:
- add Cc Chanho Park, author of commit 3d73b200f989 ("scsi: ufs:
ufs-exynos: Change ufs phy control sequence")
v1->v2:
- add better ufs->phy_powered_on handling in exynos_ufs_exit(),
exynos_ufs_suspend() and exynos_ufs_resume() which ensures we won't
enter a phy->power_count underrun condition
---
drivers/phy/phy-core.c | 18 +++++++++++
drivers/phy/samsung/phy-samsung-ufs.c | 30 ++++++++++++------
drivers/ufs/host/ufs-exynos.c | 45 ++++++++++++++++++++++-----
drivers/ufs/host/ufs-exynos.h | 1 +
4 files changed, 77 insertions(+), 17 deletions(-)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 21aaf2f76e53..6305efe210d6 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -606,6 +606,24 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
}
EXPORT_SYMBOL_GPL(phy_validate);
+int phy_request_bus_width(struct phy *phy, int bus_width)
+{
+ int ret;
+
+ if (!phy)
+ return -EINVAL;
+
+ if (!phy->ops->request_bus_width)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&phy->mutex);
+ ret = phy->ops->request_bus_width(phy, bus_width);
+ mutex_unlock(&phy->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_request_bus_width);
+
/**
* _of_phy_get() - lookup and obtain a reference to a phy by phandle
* @np: device_node for which to get the phy
diff --git a/drivers/phy/samsung/phy-samsung-ufs.c b/drivers/phy/samsung/phy-samsung-ufs.c
index 00e570d699f3..5d7b842bff1a 100644
--- a/drivers/phy/samsung/phy-samsung-ufs.c
+++ b/drivers/phy/samsung/phy-samsung-ufs.c
@@ -161,16 +161,6 @@ static int samsung_ufs_phy_clks_init(struct samsung_ufs_phy *phy)
return devm_clk_bulk_get(phy->dev, num_clks, phy->clks);
}
-static int samsung_ufs_phy_request_bus_width(struct phy *phy, int bus_width)
-{
- if (bus_width != 1 && bus_width != 2)
- return -EINVAL;
-
- ss_phy->lane_cnt = phy->attrs.bus_width;
-
- return 0;
-}
-
static int samsung_ufs_phy_init(struct phy *phy)
{
struct samsung_ufs_phy *ss_phy = get_samsung_ufs_phy(phy);
@@ -213,6 +203,26 @@ static int samsung_ufs_phy_power_off(struct phy *phy)
return 0;
}
+static int samsung_ufs_phy_request_bus_width(struct phy *phy, int bus_width)
+{
+ struct samsung_ufs_phy *ss_phy = get_samsung_ufs_phy(phy);
+
+ if (bus_width != 1 && bus_width != 2)
+ return -EINVAL;
+
+ ss_phy->lane_cnt = phy->attrs.bus_width;
+
+ if (phy->init_count)
+ samsung_ufs_phy_init(phy);
+
+ if (phy->power_count) {
+ samsung_ufs_phy_power_off(phy);
+ return samsung_ufs_phy_power_on(phy);
+ }
+
+ return 0;
+}
+
static int samsung_ufs_phy_set_mode(struct phy *generic_phy,
enum phy_mode mode, int submode)
{
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index fb616d1599eb..b90876b268db 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -959,6 +959,40 @@ static void exynos_ufs_phy_exit(struct exynos_ufs *ufs)
phy_exit(ufs->phy);
}
+static int exynos_ufs_phy_power_on(struct exynos_ufs *ufs)
+{
+ int ret;
+
+ if (ufs->phy_powered_on)
+ return 0;
+
+ ret = phy_power_on(ufs->phy);
+ if (ret) {
+ dev_err(ufs->hba->dev, "Failed to power on PHY: %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+
+ ufs->phy_powered_on = true;
+
+ return 0;
+}
+
+static void exynos_ufs_phy_power_off(struct exynos_ufs *ufs)
+{
+ int ret;
+
+ if (!ufs->phy_powered_on)
+ return;
+
+ ret = phy_power_off(ufs->phy);
+ if (ret)
+ dev_warn(ufs->hba->dev, "Failed to power off PHY: %pe\n",
+ ERR_PTR(ret));
+
+ ufs->phy_powered_on = false;
+}
+
static int exynos_ufs_phy_update_bus_width(struct exynos_ufs *ufs)
{
struct ufs_hba *hba = ufs->hba;
@@ -979,10 +1013,7 @@ static int exynos_ufs_phy_update_bus_width(struct exynos_ufs *ufs)
if (ret)
return ret;
- if (generic_phy->power_count)
- phy_power_off(generic_phy);
-
- return phy_power_on(generic_phy);
+ return exynos_ufs_phy_power_on(ufs);
}
static void exynos_ufs_config_unipro(struct exynos_ufs *ufs)
@@ -1524,7 +1555,7 @@ static void exynos_ufs_exit(struct ufs_hba *hba)
{
struct exynos_ufs *ufs = ufshcd_get_variant(hba);
- phy_power_off(ufs->phy);
+ exynos_ufs_phy_power_off(ufs);
exynos_ufs_phy_exit(ufs);
}
@@ -1739,7 +1770,7 @@ static int exynos_ufs_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
ufs->drv_data->suspend(ufs);
if (!ufshcd_is_link_active(hba))
- phy_power_off(ufs->phy);
+ exynos_ufs_phy_power_off(ufs);
return 0;
}
@@ -1749,7 +1780,7 @@ static int exynos_ufs_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
struct exynos_ufs *ufs = ufshcd_get_variant(hba);
if (!ufshcd_is_link_active(hba))
- phy_power_on(ufs->phy);
+ exynos_ufs_phy_power_on(ufs);
exynos_ufs_config_smu(ufs);
exynos_ufs_fmp_resume(hba);
diff --git a/drivers/ufs/host/ufs-exynos.h b/drivers/ufs/host/ufs-exynos.h
index abe7e472759e..683b9150e2ba 100644
--- a/drivers/ufs/host/ufs-exynos.h
+++ b/drivers/ufs/host/ufs-exynos.h
@@ -227,6 +227,7 @@ struct exynos_ufs {
int avail_ln_rx;
int avail_ln_tx;
int rx_sel_idx;
+ bool phy_powered_on;
struct ufs_pa_layer_attr dev_req_params;
struct ufs_phy_time_cfg t_cfg;
ktime_t entry_hibern8_t;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net v2 0/2] openvswitch: fix self-deadlock on release of tunnel vports
From: Ilya Maximets @ 2026-05-04 11:43 UTC (permalink / raw)
To: netdev
Cc: i.maximets, Aaron Conole, Eelco Chaudron, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Shuah Khan, Yuan Tan, Yang Yang, dev, linux-kernel,
linux-kselftest
In-Reply-To: <20260430233848.440994-1-i.maximets@ovn.org>
On 5/1/26 1:38 AM, Ilya Maximets wrote:
> Two patches - the fix for the actual bug and the selftest that reproduces it.
>
> I missed the self-deadlock in the original patch that introduced the issue,
> because testing required code modification in the ovs-vswitchd to force it to
> use legacy tunnel ports. I thought I made the change correctly, but apparently
> something went wrong and the tests were run with the standard LWT infra instead.
> The selftest added in this patch set will at least prevent this kind of mistakes
> in the future.
>
> I mentioned, however, that these tunnel vports are legacy and not actually used
> by ovs-vswitchd. RTM_NEWLINK + COLLECT_METADATA is used in conjunction with the
> standard OVS_VPORT_TYPE_NETDEV instead since 2017. The code to use the legacy
> tunnels still exists in ovs-vswitchd however, but only as a fallback for older
> kernels and we're planning to remove it in the next release. I'll be sending an
> RFC to remove support for these legacy tunnel types from the kernel, as they
> serve no real purpose today and only increase the uAPI surface for CVEs, but
> we need to fix the known bugs for stable versions.
>
>
> Version 2:
> - Added Ack from Eelco to the first patch (not to the second as it
> changed a little).
> - Removed now unused import socket in the dpctl.py [pylint/ruff].
>
> - Regarding comments from both Sashiko instances on the selftest patch:
>
> * The background process is not waited for / not killed.
> If it hangs it will not be killable anyway, so it's not a problem.
Both sashiko instances still flag this. Looks like the cover letter is not
included in the prompt.
If someone thinks I should add the suggested kill on exit, I can, but it will
not be effective in case the process hangs.
Best regards, Ilya Maximets.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/mlx5: Add VHCA_ID page management mode support
From: Moshe Shemesh @ 2026-05-04 11:41 UTC (permalink / raw)
To: Jakub Kicinski, tariqt
Cc: edumazet, pabeni, andrew+netdev, davem, saeedm, leon, mbloch,
agoldberger, netdev, linux-rdma, linux-kernel, gal, dtatulea
In-Reply-To: <20260503014520.4098577-1-kuba@kernel.org>
On 5/3/2026 4:45 AM, Jakub Kicinski wrote:
>
> 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/mlx5: Add VHCA_ID page management mode support
>
> This commit adds support for VHCA_ID-based page management mode. It
> updates page management commands to conditionally use vhca_id and adds
> a function to resolve the function type via vport lookup.
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> @@ -852,6 +852,48 @@ bool mlx5_esw_vport_vhca_id(struct mlx5_eswitch *esw, u16 vportn, u16 *vhca_id)
>> return true;
>> }
>>
>> +u16 mlx5_esw_vhca_id_to_func_type(struct mlx5_core_dev *dev, u16 vhca_id)
>> +{
>> + struct mlx5_eswitch *esw = dev->priv.eswitch;
>> + struct mlx5_vport *vport;
>> + unsigned long i;
>> + u16 type;
>> +
>> + if (vhca_id == MLX5_CAP_GEN(dev, vhca_id))
>> + return MLX5_SELF;
>> +
>> + if (!esw)
>> + return MLX5_FUNC_TYPE_NONE;
>> +
>> + mutex_lock(&esw->state_lock);
>
> Can this race with driver unload?
>
> Since the page allocator workqueue (pg_wq) is asynchronous and destroyed
> after the eswitch structure is freed, could a pending page request evaluate
> esw as valid just before it's freed, and then attempt to lock it here?
>
> Also, is there a potential deadlock when acquiring esw->state_lock here?
>
> If a thread holds the lock while sending a firmware command, and the
> firmware issues a page request to complete it, the pg_wq worker will block
> here waiting for the lock. Would this prevent the firmware command from
> completing?
In give_pages() the function mlx5_esw_vhca_id_to_func_type() is called
only after the firmware got the pages. Following other mail comments
will use caching instead of lock.
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> [ ... ]
>> -static int give_pages(struct mlx5_core_dev *dev, u16 func_id, int npages,
>> +static int give_pages(struct mlx5_core_dev *dev, u16 func_vhca_id, int npages,
>> int event, bool ec_function)
>> {
>> - u32 function = get_function(func_id, ec_function);
>> + u32 function = get_function_key(dev, func_vhca_id, ec_function);
>> u32 out[MLX5_ST_SZ_DW(manage_pages_out)] = {0};
>> int inlen = MLX5_ST_SZ_BYTES(manage_pages_in);
>
> This isn't a new issue introduced by this patch, but does the npages
> calculation risk an integer overflow?
>
> The npages parameter is derived from the firmware payload, and the
> allocation size is calculated as:
>
> inlen += npages * MLX5_FLD_SZ_BYTES(manage_pages_in, pas[0]);
>
> If a compromised firmware provides a massive npages value, could this
> overflow the 32-bit inlen and result in an undersized kvzalloc allocation,
> followed by an out-of-bounds write in the subsequent loop?
Technically yes, but as Sashiko writes here, it is not relevant to this
patch or this series. it could be addressed as a separate hardening patch.
>
> [ ... ]
>
>> @@ -577,8 +626,9 @@ static int reclaim_pages(struct mlx5_core_dev *dev, u16 func_id, int npages,
>> if (nclaimed)
>> *nclaimed = num_claimed;
>>
>> - func_type = func_id_to_type(dev, func_id, ec_function);
>> - dev->priv.page_counters[func_type] -= num_claimed;
>> + func_type = func_vhca_id_to_type(dev, func_vhca_id, ec_function);
>> + if (func_type != MLX5_FUNC_TYPE_NONE)
>> + dev->priv.page_counters[func_type] -= num_claimed;
>
> Does this dynamically look up the vport during page reclaim?
>
> If an SF or VF has been destroyed and its vport removed, the lookup will
> return MLX5_FUNC_TYPE_NONE. Will this cause the decrement to be skipped,
> leading to a counter leak and triggering the WARN() statements in
> mlx5_reclaim_startup_pages() during driver unload?
VF and SF vports are removed after reclaiming pages, but as already
wrote on the other mail, will try caching
>
> [ ... ]
>
>> @@ -658,30 +708,101 @@ static int req_pages_handler(struct notifier_block *nb,
> [ ... ]
>> + } else {
>> + if (MLX5_CAP_GEN_MAX(dev, icm_mng_function_id_mode) ==
>> + MLX5_ID_MODE_FUNCTION_VHCA_ID) {
>> + err = mlx5_pagealloc_migrate_pf_to_vhca_id(dev);
>> + if (err)
>> + return err;
>> + mlx5_page_mgt_mode_set(dev, MLX5_PAGE_MGT_MODE_VHCA_ID);
>> + }
>> + }
>
> What happens to the device state if mlx5_pagealloc_migrate_pf_to_vhca_id()
> fails?
>
> The firmware has already been instructed to use VHCA_ID mode during driver
> load, but if the migration fails, the function returns early and leaves
> dev->priv.page_mgt_mode as FUNC_ID.
>
> When the initialization aborts and mlx5_reclaim_startup_pages() runs, will
> the driver send reclaim commands using the old ID, causing the firmware
> to reject them and leak the allocated DMA memory?
If mlx5_pagealloc_migrate_pf_to_vhca_id() fails, it can fail only on
xa_insert of the new key, OOM issue and we couldn't set the new key. if
it does fail, the device init fails entirely anyway.
^ permalink raw reply
* Re: [PATCH net] openvswitch: vport: fix race between tunnel creation and linking
From: Ilya Maximets @ 2026-05-04 11:38 UTC (permalink / raw)
To: netdev
Cc: i.maximets, Aaron Conole, Eelco Chaudron, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, dev,
linux-kernel, Yuan Tan, Yifan Wu, Juefei Pu, Xin Liu, Yang Yang
In-Reply-To: <20260430213349.407991-1-i.maximets@ovn.org>
On 4/30/26 11:32 PM, Ilya Maximets wrote:
> When a tunnel vport is created it first creates the tunnel device, e.g.,
> with geneve_dev_create_fb(), then it calls ovs_netdev_link() to take a
> reference and link it to the device that represents openvswitch datapath.
>
> The creation of the device is happening under RTNL, but then RTNL is
> released and re-acquired to find the device by name. It is technically
> possible for the tunnel device to be re-named or deleted within that
> window while RTNL is not held, and some other device created in its
> place. This will cause a non-tunnel device to be referenced in the
> vport and tunnel-specific functions used on it, e.g. vxlan_get_options()
> that directly casts the private netdev data into a struct vxlan_dev
> causing an invalid memory access:
>
> BUG: KASAN: slab-use-after-free in vxlan_get_options+0x323/0x3a0
> vxlan_get_options+0x323/0x3a0
> ovs_vport_cmd_new+0x6e3/0xd30
>
> Fix that by taking a reference to the just created device before
> releasing RTNL. This ensures that the device in the vport is always
> the one that was just created. The search by name is only needed
> for a standard vport-netdev that links pre-existing devices, so that
> functionality and device type checks are moved to netdev_create().
>
> It is also awkward that ovs_netdev_link() takes ownership of the vport
> and destroys it on failure. It doesn't know the type of the port it is
> dealing with, so we need to pass down the indicator that it's a tunnel,
> so the link can be properly deleted on failure.
>
> It's possible to refactor the logic to make the ovs_netdev_link() do
> only the linking part and let the callers perform a proper destruction,
> but it will be much more code for each legacy tunnel port type, so it
> is not worth it for the bug fix.
>
> Fixes: 614732eaa12d ("openvswitch: Use regular VXLAN net_device device")
> Reported-by: Yuan Tan <tanyuan98@outlook.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Reported-by: Yang Yang <n05ec@lzu.edu.cn>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
> net/openvswitch/vport-geneve.c | 5 ++-
> net/openvswitch/vport-gre.c | 5 ++-
> net/openvswitch/vport-netdev.c | 58 ++++++++++++++++++++--------------
> net/openvswitch/vport-netdev.h | 2 +-
> net/openvswitch/vport-vxlan.c | 5 ++-
> 5 files changed, 48 insertions(+), 27 deletions(-)
>
...
> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
> index 12055af832dc0..a92ca8b37f96a 100644
> --- a/net/openvswitch/vport-netdev.c
> +++ b/net/openvswitch/vport-netdev.c
> @@ -73,37 +73,21 @@ static struct net_device *get_dpdev(const struct datapath *dp)
> return local->dev;
> }
>
> -struct vport *ovs_netdev_link(struct vport *vport, const char *name)
> +struct vport *ovs_netdev_link(struct vport *vport, bool tunnel)
> {
> int err;
>
> - vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
> - if (!vport->dev) {
> + if (WARN_ON_ONCE(!vport->dev)) {
> err = -ENODEV;
> goto error_free_vport;
> }
> - /* Ensure that the device exists and that the provided
> - * name is not one of its aliases.
> - */
> - if (strcmp(name, ovs_vport_name(vport))) {
> - err = -ENODEV;
> - goto error_put;
> - }
> - netdev_tracker_alloc(vport->dev, &vport->dev_tracker, GFP_KERNEL);
> - if (vport->dev->flags & IFF_LOOPBACK ||
> - (vport->dev->type != ARPHRD_ETHER &&
> - vport->dev->type != ARPHRD_NONE) ||
> - ovs_is_internal_dev(vport->dev)) {
> - err = -EINVAL;
> - goto error_put;
> - }
>
> rtnl_lock();
> err = netdev_master_upper_dev_link(vport->dev,
> get_dpdev(vport->dp),
> NULL, NULL, NULL);
> if (err)
> - goto error_unlock;
> + goto error_put_unlock;
>
> err = netdev_rx_handler_register(vport->dev, netdev_frame_hook,
> vport);
Sashiko-gemini reports that here we could be linking an already unregistering
device since we're not checking the registration status after re-acquiring the
lock. Which seems like an issue, which is related, but fairly separate from
what this patch is trying to fix. It is also not specific to the tunnel ports.
So, should be addressed separately.
Best regards, Ilya Maximets.
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/mlx5: Add VHCA_ID page management mode support
From: Moshe Shemesh @ 2026-05-04 11:33 UTC (permalink / raw)
To: Jakub Kicinski, tariqt
Cc: edumazet, pabeni, andrew+netdev, davem, saeedm, leon, mbloch,
agoldberger, netdev, linux-rdma, linux-kernel, gal, dtatulea
In-Reply-To: <20260503014501.4098393-1-kuba@kernel.org>
On 5/3/2026 4:45 AM, Jakub Kicinski wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/mlx5: Add VHCA_ID page management mode support
>
> This change adds a VHCA_ID-based page management mode that uses vhca_id
> instead of function_id as the effective function identifier when the
> firmware advertises icm_mng_function_id_mode. Boot pages keep FUNC_ID
> semantics for backward compatibility, and tracked pages are migrated to
> the vhca_id key after set_hca_cap().
>
> A few questions below on the per-type counter accounting, the boot-page
> migration helper, and locking on the FW-driven page path.
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> index e0eafcf0c52a..d3eaefc5c0e0 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> @@ -852,6 +852,48 @@ bool mlx5_esw_vport_vhca_id(struct mlx5_eswitch *esw, u16 vportn, u16 *vhca_id)
>> return true;
>> }
>>
>> +u16 mlx5_esw_vhca_id_to_func_type(struct mlx5_core_dev *dev, u16 vhca_id)
>> +{
>> + struct mlx5_eswitch *esw = dev->priv.eswitch;
>> + struct mlx5_vport *vport;
>> + unsigned long i;
>> + u16 type;
>> +
>> + if (vhca_id == MLX5_CAP_GEN(dev, vhca_id))
>> + return MLX5_SELF;
>> +
>> + if (!esw)
>> + return MLX5_FUNC_TYPE_NONE;
>> +
>> + mutex_lock(&esw->state_lock);
>
> This function is reached from give_pages()/reclaim_pages()/
> release_all_pages() via func_vhca_id_to_type(), which in turn runs
> from the pg_wq work handler triggered by firmware page-request EQE
> events. Does acquiring esw->state_lock on that path introduce a new
> lock dependency?
>
> Several eswitch paths (for example mlx5_esw_vport_enable(),
> mlx5_esw_vport_disable(), mlx5_eswitch_set_vport_mac()) hold
> state_lock while synchronously issuing firmware commands. Before this
> patch, the page path held no eswitch locks.
>
> Would it be safer to resolve the func_type outside of state_lock, for
> example by caching the vhca_id-to-type mapping separately, or by
> attaching the resolved type to the fw_page at give time so reclaim
> does not need to look it up again?
>
I don't think there is a real issue with the lock, but for sure better
without, I will try caching vhca_id to type mapping.
>> + mlx5_esw_for_each_vport(esw, i, vport) {
>> + if (vport->vhca_id != vhca_id)
>> + continue;
>> +
>> + if (vport->vport == MLX5_VPORT_HOST_PF) {
>> + type = MLX5_HOST_PF;
>> + goto unlock;
>> + }
>> +
>> + if (xa_get_mark(&esw->vports, i, MLX5_ESW_VPT_SF)) {
>> + type = MLX5_SF;
>> + goto unlock;
>> + }
>> +
>> + if (xa_get_mark(&esw->vports, i, MLX5_ESW_VPT_VF)) {
>> + type = MLX5_VF;
>> + goto unlock;
>> + }
>> +
>> + type = MLX5_EC_VF;
>> + goto unlock;
>
> Can this fall-through misattribute counters?
>
> If a matching vport is not HOST_PF and has neither the VPT_SF nor the
> VPT_VF mark, it is unconditionally classified as MLX5_EC_VF. Would an
> UPLINK, ECPF, manager, or adjacent-style vport whose vhca_id happens
> to match end up charged to the EC_VF bucket here?
>
The fallthrough is correct in practice - uplink is the physical port,
not a separate HCA function, FW doesn't request pages for it. ECPF is
the esw manager, if requested here can be only as self, will catch the
MLX5_SELF condition. Adjacent VFs are caught correctly by the VF mark.
>> + }
>> + type = MLX5_FUNC_TYPE_NONE;
>> +unlock:
>> + mutex_unlock(&esw->state_lock);
>> + return type;
>> +}
>> +
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> index a242053f3a58..52cf341ad6b3 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> @@ -597,6 +597,9 @@ static int handle_hca_cap(struct mlx5_core_dev *dev, void *set_ctx)
>> if (MLX5_CAP_GEN_MAX(dev, release_all_pages))
>> MLX5_SET(cmd_hca_cap, set_hca_cap, release_all_pages, 1);
>>
>> + if (MLX5_CAP_GEN_MAX(dev, icm_mng_function_id_mode))
>> + MLX5_SET(cmd_hca_cap, set_hca_cap, icm_mng_function_id_mode, 1);
>> +
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
>> index 77ffa31cc505..7ebe88aa3b3e 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
>
> [ ... ]
>
>> @@ -89,12 +105,21 @@ static u16 func_id_to_type(struct mlx5_core_dev *dev, u16 func_id, bool ec_funct
>> return MLX5_SF;
>> }
>>
>> +static u16 func_vhca_id_to_type(struct mlx5_core_dev *dev, u16 func_vhca_id,
>> + bool ec_function)
>> +{
>> + if (mlx5_page_mgt_mode_is_vhca_id(dev))
>> + return mlx5_esw_vhca_id_to_func_type(dev, func_vhca_id);
>> +
>> + return func_id_to_type(dev, func_vhca_id, ec_function);
>> +}
>
> Can the give/reclaim accounting become asymmetric in VHCA_ID mode?
>
> The per-type counters are only updated when the resolved type is not
> MLX5_FUNC_TYPE_NONE:
>
> func_type = func_vhca_id_to_type(dev, func_vhca_id, ec_function);
> if (func_type != MLX5_FUNC_TYPE_NONE)
> dev->priv.page_counters[func_type] += npages;
>
> Since mlx5_esw_vhca_id_to_func_type() walks the eswitch vport table
> dynamically on every call, can the give and the corresponding reclaim
> resolve to different types?
>
> For example, if a give runs before vport->vhca_id is populated in
> mlx5_esw_vport_caps_get(), the lookup returns MLX5_FUNC_TYPE_NONE and
> the counter is not incremented. Later, when the vport is fully
> populated, reclaim resolves to a real type and decrements the counter
> below the amount ever added, which on a u32 drives it to a very large
> value.
>
> Similarly, if a vport is removed before reclaim, the increment at
> give time is recorded but the decrement at reclaim is skipped, so the
> counter leaks upward.
>
> Would caching the func_type on the fw_page at give time and reusing
> it on reclaim make the accounting symmetric by construction?
Following first comment, I will try caching.
>
> [ ... ]
>
>> @@ -658,30 +708,101 @@ static int req_pages_handler(struct notifier_block *nb,
>> * req->npages (and not min ()).
>> */
>> req->npages = max_t(s32, npages, MAX_RECLAIM_NPAGES);
>> - req->ec_function = ec_function;
>> + if (!mlx5_page_mgt_mode_is_vhca_id(dev))
>> + req->ec_function = ec_function;
>> req->release_all = release_all;
>> INIT_WORK(&req->work, pages_work_handler);
>> queue_work(dev->priv.pg_wq, &req->work);
>> return NOTIFY_OK;
>> }
>>
>> +/*
>> + * After set_hca_cap(), the second satisfy_startup_pages(dev, 0) may see
>> + * VHCA_ID mode. If page_root_xa already has the PF entry from the first
>> + * (boot) call under FUNC_ID keys 0 or (ec_function << 16), migrate that
>> + * entry to the device vhca_id key so lookups use VHCA_ID semantics.
>> + */
>> +static int mlx5_pagealloc_migrate_pf_to_vhca_id(struct mlx5_core_dev *dev)
>> +{
>> + u32 vhca_id_key, old_key;
>> + struct rb_root *root;
>> + struct fw_page *fwp;
>> + struct rb_node *p;
>> + bool ec_function;
>> + int err;
>> +
>> + if (xa_empty(&dev->priv.page_root_xa))
>> + return 0;
>> +
>> + vhca_id_key = MLX5_CAP_GEN(dev, vhca_id);
>> + ec_function = mlx5_core_is_ecpf(dev);
>> +
>> + old_key = ec_function ? (1U << 16) : 0;
>> + root = xa_load(&dev->priv.page_root_xa, old_key);
>> + if (!root)
>> + return 0;
>
> Does this assume the boot-path func_vhca_id was always 0?
>
> The boot call to mlx5_cmd_query_pages() reads func_vhca_id directly
> from the firmware output, and give_pages() then uses that value to
> compute the key. The migration here instead hardcodes old_key as
> ec_function ? (1U << 16) : 0.
>
> If firmware returned a non-zero boot function_id, xa_load(old_key)
> returns NULL, the function silently returns 0, the caller flips the
> mode to VHCA_ID, and the original rb_root is orphaned in page_root_xa
> under the old key. Subsequent free_fwp()/find_fw_page() paths would
> then hit WARN_ON_ONCE(!root) and leak the DMA mappings and pages.
>
> Would it be more robust to look up the actual key used at boot
> (derived from the stored func_vhca_id), and to treat the "xa not
> empty but old_key absent" case as an invariant violation rather than
> silently succeeding?
>
During boot (mlx5_satisfy_startup_pages(dev, 1)), the mode is FUNC_ID.
The mlx5_cmd_query_pages() call sets embedded_cpu_function in the
request but leaves function_id as 0 (the PF queries its own pages).
Firmware returns function_id in the output, which for the PF's own pages
is always 0.
The "silently returns 0" when xa_load returns NULL is also fine, it
means no boot pages were allocated.
>> +
>> + if (old_key == vhca_id_key)
>> + return 0;
>> +
>> + err = xa_insert(&dev->priv.page_root_xa, vhca_id_key, root, GFP_KERNEL);
>> + if (err) {
>> + mlx5_core_warn(dev,
>> + "failed to migrate page root key 0x%x to vhca_id 0x%x\n",
>> + old_key, vhca_id_key);
>> + return err;
>> + }
>> +
>> + xa_erase(&dev->priv.page_root_xa, old_key);
>> +
>> + for (p = rb_first(root); p; p = rb_next(p)) {
>> + fwp = rb_entry(p, struct fw_page, rb_node);
>> + fwp->function = vhca_id_key;
>> + }
>
> Is the ordering here safe against any concurrent free_fwp()?
>
> Between xa_erase(old_key) and the loop that updates fwp->function,
> every fw_page still carries the old key while page_root_xa no longer
> resolves it. If a free_fwp() were to run in that window:
>
> root = xa_load(&dev->priv.page_root_xa, fwp->function);
> if (WARN_ON_ONCE(!root))
> return;
>
> it would return early, skipping dma_unmap_page(), __free_page(), and
> kfree(fwp), leaking the DMA mapping and the backing page.
>
> No concurrent free path is structurally reachable today because this
> runs before the EQ notifier is registered in mlx5_pagealloc_start(),
> but would it be cleaner to update the fwp->function values first,
> then swap the xarray entries (or store the new value at a single key)
> so the two views cannot disagree?
>
Reordering is safer, I will reorder.
>> +
>> + return 0;
>> +}
>> +
>> int mlx5_satisfy_startup_pages(struct mlx5_core_dev *dev, int boot)
>> {
>> - u16 func_id;
>> + bool ec_function = false;
>> + u16 func_vhca_id;
>> s32 npages;
>> int err;
>>
>> - err = mlx5_cmd_query_pages(dev, &func_id, &npages, boot);
>> + /* When boot flag is set, the icm_mng_function_id_mode capability is
>> + * not yet set (only set after set_hca_cap()), so use FUNC_ID mode
>> + * for backward compatibility. When boot is false, set mode from
>> + * cap (set_hca_cap has run successfully).
>> + */
>> + if (boot) {
>> + mlx5_page_mgt_mode_set(dev, MLX5_PAGE_MGT_MODE_FUNC_ID);
>> + } else {
>> + if (MLX5_CAP_GEN_MAX(dev, icm_mng_function_id_mode) ==
>> + MLX5_ID_MODE_FUNCTION_VHCA_ID) {
>
> The comment just above says "set mode from cap (set_hca_cap has run
> successfully)", which reads as "use the current/negotiated cap value",
> but the check uses MLX5_CAP_GEN_MAX rather than MLX5_CAP_GEN. The
> sibling code in drivers/net/ethernet/mellanox/mlx5/core/debugfs.c uses
> MLX5_CAP_GEN(dev, icm_mng_function_id_mode) for the same semantic
> check.
>
> Could the comment and the _MAX usage be made consistent? If anyone
> later adds a conditional around the MLX5_SET() in handle_hca_cap()
> (for example a module parameter), the driver would start flipping to
> VHCA_ID mode based on _MAX without actually having enabled the feature
> in firmware.
OK, I will rewrite the comment.
>
>> + err = mlx5_pagealloc_migrate_pf_to_vhca_id(dev);
>> + if (err)
>> + return err;
>> + mlx5_page_mgt_mode_set(dev, MLX5_PAGE_MGT_MODE_VHCA_ID);
>> + }
>> + }
>
> Once page_mgt_mode is flipped to MLX5_PAGE_MGT_MODE_VHCA_ID, it stays
> set in dev->priv and is only reset on the next boot=1 call. Between
> teardown and the next boot=1 path (for example during health
> recovery, PCI reset, or any async flow that runs page work before
> mlx5_satisfy_startup_pages(dev, 1) re-runs), would a FW page-request
> EQE observe a stale mode?
>
> The req_pages EQ notifier is registered via mlx5_pagealloc_start()
> and unregistered via mlx5_pagealloc_stop(), so this is not reachable
> today, but would it be worth resetting the mode explicitly on the
> teardown side rather than relying on the next reinit?
No. Once flipped to MLX5_PAGE_MGT_MODE_VHCA_ID, both driver and firmware
keep using it till reopen function (next boot=1 call).
>
> [ ... ]
>
>> @@ -751,6 +874,9 @@ int mlx5_reclaim_startup_pages(struct mlx5_core_dev *dev)
>> WARN(dev->priv.fw_pages,
>> "FW pages counter is %d after reclaiming all pages\n",
>> dev->priv.fw_pages);
>> + if (mlx5_page_mgt_mode_is_vhca_id(dev) && !dev->priv.eswitch)
>> + return 0;
>> +
>
> Does this guard address the asymmetric-counter case raised above?
>
> It only skips the per-type WARNs when the eswitch is entirely absent.
> In the common case where the eswitch is present but a vport's
> vhca_id/marks change between give and reclaim, the counters can still
> drift and these WARNs would still fire on normal teardown paths.
>
Following first comment, will try caching.
> [ ... ]
^ permalink raw reply
* Re: [RFC PATCH net-next 1/2] net: napi: Fix interrupts permanently disabled during busy poll
From: Dragos Tatulea @ 2026-05-04 11:30 UTC (permalink / raw)
To: Björn Töpel, Jakub Kicinski
Cc: David S. Miller, Eric Dumazet, Paolo Abeni, Simon Horman,
Daniel Borkmann, Martin Karsten, Gal Pressman, Tariq Toukan,
Joe Damato, Frederik Deweerdt, netdev, linux-kernel
In-Reply-To: <bq662gcnuk3yltgzg4q5w672xsbsdf3abqrej2mrg6rl74ts2v@5uv5a35jrlp7>
On Wed, Apr 29, 2026 at 12:43:54PM +0000, Dragos Tatulea wrote:
> On Wed, Apr 29, 2026 at 02:13:43PM +0200, Björn Töpel wrote:
> > Dragos Tatulea <dtatulea@nvidia.com> writes:
> >
> > > On Tue, Apr 28, 2026 at 05:38:44PM -0700, Jakub Kicinski wrote:
> > >> On Tue, 28 Apr 2026 17:51:30 +0000 Dragos Tatulea wrote:
> > >> > + local_irq_save(flags);
> > >> > + hrtimer_start(&napi->timer, ns_to_ktime(timeout),
> > >> > + HRTIMER_MODE_REL_PINNED);
> > >> > clear_bit(NAPI_STATE_SCHED, &napi->state);
> > >> > + local_irq_restore(flags);
> > >>
> > >> I don't think disabling IRQ is necessary?
> > >> Isn't it legal to clear the bit first then schedule the timer?
> > >> The timer does not own the napi instance.
> > >
> > > Isn't the following scenario possible (but extremely unlikely)?
> > >
> > > 1. busy_poll_stop(): napi timer is schedueled.
> > > 2. Hard irq pre-empts busy_poll_stop() and takes an unusually long time.
> > > 4. napi timer triggers (also hard irq), napi_watchdog() skips schedule
> > > because NAPI_STATE_SCHED is set.
> > > 5. busy_poll_stop(): NAPI_STATE_SCHED gets cleared.
> >
> > (Nice work finding the bug! I had to jog my memory to understand the
> > gnarly busy-poll details again!)
> >
> > You're right that you need the save/restore if you do arm timer/clear,
> > but not if you do what Jakub suggests.
> >
> > clear_bit(...);
> > hrtimer_start();
> >
> Ah sorry, I didn't read it right the first time because I was too fixed
> on the order.
>
> > That would also follow the scheme in napi_schedule_done(). (Note that
> > swapping arm/clear does mean that we can get a wasted-timer outcome.)
> >
> If it is wasted is fine. I figured that this could end up with a timer
> scheduled after the napi deletion:
>
> 1. busy_poll_stop(): clears SCHED bit.
> 2. napi_disable() runs past SCHED wait, cancels timer and caller
> deletes napi mem.
> 3. busy_poll_stop(): napi->timer is armed but napi is freed memory
> by now.
>
> What am I missing here?
>
Gentle ping. Once I understand how/why the above is incorrect I can
update the patch and send it.
Thanks,
Dragos
^ 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