* Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup
From: Nnamdi Onyeyiri @ 2026-07-20 22:53 UTC (permalink / raw)
To: Emil Tsalapatis
Cc: bpf, davem, edumazet, horms, jakub, jiayuan.chen, john.fastabend,
kuba, kuniyu, ncardwell, netdev, pabeni, sashiko-reviews,
linux-kernel
In-Reply-To: <DK3PKY6RN5FI.4DKPSHWNEBHM@etsalapatis.com>
On Mon, Jul 20, 2026 at 05:16:08PM -0400, Emil Tsalapatis wrote:
> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
> > recvfrom()/recv() are documented as only returning EAGAIN for blocking sockets
> > when they have a receive timeout configured. However, adding a blocking
> > ipv4 tcp socket without a receive timeout to a sockmap will cause EAGAIN errors
> > sporadically. A socket with a receive timeout may return EAGAIN before the
> > timeout expires.
> >
> > There are 2 code paths affected by this:
> >
> > 1. tcp_bpf_recvmsg() - Used when the socket has been added to a sockmap
> > that has no verdict program attached.
> >
> > 2. tcp_bpf_recvmsg_parser() - Used when the socket has been added to a
> > sockmap that has a verdict program. To reproduce this issue, it is
> > enough for the verdict program to do nothing but return SK_PASS.
> >
> > In both cases this happens when tcp_msg_wait_data() wakes spuriously
> > (returning 0). To fix it, we now loop back to msg_bytes_ready instead
> > of returning -EAGAIN on spurious wakeup.
> >
> > To ensure the looping does not cause sockets with a SO_RCVTIMEO set to
> > wait excessively long, tcp_msg_wait_data() now takes a pointer to timeo,
> > allowing sk_wait_event() to update it as appropriate.
> >
> > The logic in tcp_bpf_recvmsg_parser() that allow it to handle signals,
> > socket errors and closuers in its loop was also added to tcp_bpf_recvmsg().
> >
> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
> > ---
> > net/ipv4/tcp_bpf.c | 69 ++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 60 insertions(+), 9 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> > index cc0bd73f36b6..aa5c5d741599 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
> >
> > #ifdef CONFIG_BPF_SYSCALL
> > static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
> > - long timeo)
> > + long *timeo)
> > {
> > DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > int ret = 0;
> > @@ -187,12 +187,12 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
> > if (sk->sk_shutdown & RCV_SHUTDOWN)
> > return 1;
> >
> > - if (!timeo)
> > + if (!*timeo)
> > return ret;
> >
> > add_wait_queue(sk_sleep(sk), &wait);
> > sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> > - ret = sk_wait_event(sk, &timeo,
> > + ret = sk_wait_event(sk, timeo,
> > !list_empty(&psock->ingress_msg) ||
> > !skb_queue_empty_lockless(&sk->sk_receive_queue), &wait);
> > sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> > @@ -229,6 +229,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> > int copied_from_self = 0;
> > int copied = 0;
> > u32 seq;
> > + long timeo;
> >
> > if (unlikely(flags & MSG_ERRQUEUE))
> > return inet_recv_error(sk, msg, len);
> > @@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> > }
> > }
> >
> > + timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > +
> > msg_bytes_ready:
> > copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
> > /* The typical case for EFAULT is the socket was gracefully
> > @@ -280,7 +283,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> > }
> > seq += copied_from_self;
> > if (!copied) {
> > - long timeo;
> > int data;
> >
> > if (sock_flag(sk, SOCK_DONE))
> > @@ -299,7 +301,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> > goto out;
> > }
> >
> > - timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > if (!timeo) {
> > copied = -EAGAIN;
> > goto out;
> > @@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
> > goto out;
> > }
> >
> > - data = tcp_msg_wait_data(sk, psock, timeo);
> > + data = tcp_msg_wait_data(sk, psock, &timeo);
> > if (data < 0) {
> > copied = data;
> > goto unlock;
> > }
> > if (data && !sk_psock_queue_empty(psock))
> > goto msg_bytes_ready;
> > + if (!data && timeo > 0)
> > + goto msg_bytes_ready;
> > copied = -EAGAIN;
> > }
> > out:
> > @@ -355,6 +358,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> > {
> > struct sk_psock *psock;
> > int copied, ret;
> > + long timeo;
> >
> > if (unlikely(flags & MSG_ERRQUEUE))
> > return inet_recv_error(sk, msg, len);
> > @@ -371,14 +375,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> > return tcp_recvmsg(sk, msg, len, flags);
> > }
> > lock_sock(sk);
> > +
> > + timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > +
> > msg_bytes_ready:
> > copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
> > if (!copied) {
> > - long timeo;
> > int data;
> >
> > - timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > - data = tcp_msg_wait_data(sk, psock, timeo);
> > + if (sock_flag(sk, SOCK_DONE)) {
> > + ret = 0;
> > + goto unlock;
> > + }
> > +
> > + if (sk->sk_err) {
> > + if (!sk_psock_queue_empty(psock))
> > + goto msg_bytes_ready;
> > + if (!skb_queue_empty(&sk->sk_receive_queue)) {
> > + release_sock(sk);
> > + sk_psock_put(sk, psock);
> > + return tcp_recvmsg(sk, msg, len, flags);
> > + }
> > + ret = sock_error(sk);
> > + goto unlock;
> > + }
> > +
> > + if (sk->sk_shutdown & RCV_SHUTDOWN) {
> > + if (!sk_psock_queue_empty(psock))
> > + goto msg_bytes_ready;
> > + if (!skb_queue_empty(&sk->sk_receive_queue)) {
> > + release_sock(sk);
> > + sk_psock_put(sk, psock);
> > + return tcp_recvmsg(sk, msg, len, flags);
> > + }
> > + ret = 0;
> > + goto unlock;
>
> These two error handling routines above look identical. Can you refactor
> them?
>
Will do. My understanding is the same logic is needed to address the
issue Sashiko raised with the SOCK_DONE check as well.
> > + }
> > +
> > + if (sk->sk_state == TCP_CLOSE) {
> > + ret = -ENOTCONN;
> > + goto unlock;
> > + }
> > +
> > + if (!timeo) {
> > + ret = -EAGAIN;
> > + goto unlock;
> > + }
> > +
>
> Since this handling (which Sashiko flags by the way, correctly AFAICT)
> are taken from tcp_bpf_recvmsg, there is obvious overlap between the two
> functions. Please factor those out so that they share the logic between
> them.
>
> pw-bot: cr
>
Sashiko highlighted the "if (!timeo)" and signal_pending early returns
when MSG_DONTWAIT is set, but I think I'm missing part of the picture.
By the time we reach these branches, haven't we already checked for data
in sk_receive_queue (line 372, after the patch is applied to 7.2-rc2)
[copied below for ease of viewing]:
if (!skb_queue_empty(&sk->sk_receive_queue) &&
sk_psock_queue_empty(psock)) {
sk_psock_put(sk, psock);
return tcp_recvmsg(sk, msg, len, flags);
}
and in the psock (line 382) [again copied below for viewing]:
copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
> > + if (signal_pending(current)) {
> > + ret = sock_intr_errno(timeo);
> > + goto unlock;
> > + }
> > +
> > + data = tcp_msg_wait_data(sk, psock, &timeo);
> > if (data < 0) {
> > ret = data;
> > goto unlock;
> > @@ -390,6 +439,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> > sk_psock_put(sk, psock);
> > return tcp_recvmsg(sk, msg, len, flags);
> > }
> > + if (!data && timeo > 0)
> > + goto msg_bytes_ready;
> > copied = -EAGAIN;
> > }
> > ret = copied;
>
^ permalink raw reply
* Re: [PATCH v2] selftests: Open /dev/udmabuf O_RDONLY
From: Jakub Kicinski @ 2026-07-20 23:10 UTC (permalink / raw)
To: T.J. Mercier
Cc: Shuah Khan, kraxel, vivek.kasireddy, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kselftest, linux-kernel, netdev,
bpf, Bobby Eshleman
In-Reply-To: <CABdmKX0-tubji_pBiaZbePrxVLA9qkw5ez61wyzjDz8-pko9SQ@mail.gmail.com>
On Fri, 17 Jul 2026 09:29:40 -0700 T.J. Mercier wrote:
> > Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
>
> Thanks Bobby.
>
> Shuah, would you mind picking this up?
Please split the networking part off, networking selftests must go via
networking trees.
^ permalink raw reply
* Re: [RESEND PATCH] net/core: consolidate RPS dispatch into netif_rps() helpers
From: Jakub Kicinski @ 2026-07-20 23:25 UTC (permalink / raw)
To: Jemmy Wong
Cc: netdev, linux-kernel, andrew+netdev, davem, edumazet, pabeni,
horms
In-Reply-To: <20260707154855.12134-1-jemmywong512@gmail.com>
On Tue, 7 Jul 2026 23:48:55 +0800 Jemmy Wong wrote:
> The RPS steering logic in netif_rx_internal(), netif_receive_skb_internal()
> and netif_receive_skb_list_internal() was open-coded three times, each with
> its own #ifdef CONFIG_RPS block and manual rcu_read_lock()/unlock() pairs.
>
> Factor it into two helpers, netif_rps() for the single-skb path and
> netif_rps_list() for the list path, and switch the callers to
> guard(rcu)/scoped_guard(rcu). A new internal NET_RX_UNHANDLED sentinel lets
> a helper report "RPS did not take this skb" so the caller falls back to the
> local enqueue / __netif_receive_skb() path; it never escapes to callers.
>
> netif_rps_list() keeps the early static_branch_unlikely(&rps_needed) bail
> out so the list is not needlessly walked and re-spliced when RPS is
> compiled in but disabled.
>
> No functional change intended.
You haven't read Paolo's reply, please go away.
^ permalink raw reply
* Re: [PATCH] net: hip04: quiesce tx coalesce timer before teardown
From: Jakub Kicinski @ 2026-07-20 23:32 UTC (permalink / raw)
To: Fan Wu
Cc: Simon Horman, Fan Wu, netdev, shenjian15, salil.mehta,
andrew+netdev, David S . Miller, edumazet, pabeni, linux-kernel,
stable
In-Reply-To: <4631C8F9-598D-4114-AD88-BEC5D8617BB6@zju.edu.cn>
On Sat, 11 Jul 2026 13:34:24 +0800 Fan Wu wrote:
> Thanks for the v1 review. The updated patch fixes the PHY teardown ordering
> and IRQ lifetime issues: hip04_remove() unregisters the netdev, running
> .ndo_stop and phy_stop(), before phy_disconnect(), and frees the devm-managed
> IRQ before free_netdev().
>
> The remaining NULL tx_desc cleanup in hip04_free_ring() and RX refill
> failure paths are independent pre-existing error-path issues. I will address them
> in separate patches rather than expanding this teardown fix. In particular, the
> RX fix must preserve the old descriptor mapping until a replacement buffer is
> successfully allocated and mapped.
If the patch matters you need to repost it, it wasn't processed in time
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: microchip: enable SGMII port for KSZ9897
From: Jakub Kicinski @ 2026-07-20 23:33 UTC (permalink / raw)
To: Tapio Reijonen
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Paolo Abeni, Russell King, netdev,
linux-kernel
In-Reply-To: <20260703-ksz9897-sgmii-port-v1-1-387e7632dd0b@vaisala.com>
On Fri, 03 Jul 2026 06:37:28 +0000 Tapio Reijonen wrote:
> The KSZ9897 has an SGMII/1000BASE-X capable port 7, but its chip_info
> entry never set .sgmii_port. As a result ksz_has_sgmii_port() is false,
> ksz9477_pcs_create() is never called, and ksz9477_phylink_mac_select_pcs()
> returns NULL for port 7, so no PCS is ever instantiated and a
> phy-mode = "sgmii" or "1000base-x" link on that port cannot come up.
The netdev patch queue has overflown, if this patch is necessary you
will have to repost it.
^ permalink raw reply
* Re: [PATCH net-next v9 0/4] Add support for RTL8261C_CG
From: Jakub Kicinski @ 2026-07-20 23:34 UTC (permalink / raw)
To: javen
Cc: andrew, hkallweit1, linux, davem, edumazet, pabeni, freddy_gu, nb,
maxime.chevallier, netdev, linux-kernel, daniel, vladimir.oltean
In-Reply-To: <20260703071330.1707-1-javen_xu@realsil.com.cn>
On Fri, 3 Jul 2026 15:13:26 +0800 javen wrote:
> Add support for RTL8261C_CG and add support for loading firmware.
The netdev patch queue has overflown, if this patch is necessary you
will have to repost it.
^ permalink raw reply
* Re: [PATCH net-next 01/12] net: dsa: qca8k: Move register access completion into DSA core
From: Jakub Kicinski @ 2026-07-20 23:37 UTC (permalink / raw)
To: lukeh
Cc: Jakub Kicinski, olteanv, davem, edumazet, pabeni, vivien.didelot,
gregory.clement, andrew, horms, f.fainelli, tobias,
cedric.jehasse, kieran, mh, max, christoph.mellauner, simon.gapp,
Ryan.Wilkins, mattias.forsblad, netdev, linux-kernel
In-Reply-To: <20260703-net-next-dsa-rmu-v1-1-a03febf20bb4@padl.com>
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: dsa: qca8k: Move register access completion into DSA core
This patch factors the request/reply completion used by the qca8k
mgmt-Ethernet path into a new dsa_inband helper in the DSA core, moves
the reinit_completion() and msecs_to_jiffies() conversion into the
helper, and switches every qca8k mgmt-eth call site to use it.
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 4c928983b862..fe6fb69b6f92 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -332,8 +332,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>
> skb->dev = priv->mgmt_conduit;
>
> - reinit_completion(&mgmt_eth_data->rw_done);
> -
> /* Increment seq_num and set it in the mdio pkt */
> mgmt_eth_data->seq++;
> qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
> @@ -341,8 +339,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>
> dev_queue_xmit(skb);
>
> - ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> - QCA8K_ETHERNET_TIMEOUT);
> + ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> + QCA8K_ETHERNET_TIMEOUT);
[High]
Could this reorder introduce a race between the sender and the ack
handler?
Before the patch every mgmt-eth call site did:
reinit_completion(&mgmt_eth_data->rw_done);
...
dev_queue_xmit(skb);
wait_for_completion_timeout(&mgmt_eth_data->rw_done, ...);
so the completion was armed before the frame could produce a reply.
After the patch each call site does:
dev_queue_xmit(skb);
dsa_inband_wait_for_completion(&mgmt_eth_data->inband, ...);
and dsa_inband_wait_for_completion() itself does:
int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
{
unsigned long jiffies = msecs_to_jiffies(timeout_ms);
reinit_completion(&inband->completion);
return wait_for_completion_timeout(&inband->completion, jiffies);
}
so reinit_completion() now runs after dev_queue_xmit().
The reply is delivered from the RX softirq path via
qca8k_rw_reg_ack_handler() -> dsa_inband_complete() -> complete(), with
no mutex synchronising it against the sender. If the reply is processed
in the window between dev_queue_xmit() returning and the reinit inside
the helper, done is set to 1 by the ack handler and then cleared back to
0 by reinit_completion(), and wait_for_completion_timeout() then blocks
for the full QCA8K_ETHERNET_TIMEOUT and returns 0.
Callers then report -ETIMEDOUT even though the reply arrived. Every
mgmt-eth caller looks affected: qca8k_read_eth(), qca8k_write_eth(),
qca8k_phy_eth_busy_wait() and both branches plus the exit path of
qca8k_phy_eth_command().
The next patch in the series ("net: dsa: qca8K: Move queuing for request
frame into the core", 98b48914b271) introduces dsa_inband_request() and
its comment/commit message state that reinit_completion() must precede
dev_queue_xmit() to avoid this race. Should the reinit be moved back
before dev_queue_xmit() in this patch, or should this patch be squashed
with the follow-up, to avoid a bisect window where mgmt-eth reads and
writes can spuriously fail?
> @@ -384,8 +382,6 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>
> skb->dev = priv->mgmt_conduit;
>
> - reinit_completion(&mgmt_eth_data->rw_done);
> -
> /* Increment seq_num and set it in the mdio pkt */
> mgmt_eth_data->seq++;
> qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
> @@ -393,8 +389,8 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>
> dev_queue_xmit(skb);
>
> - ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> - QCA8K_ETHERNET_TIMEOUT);
> + ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> + QCA8K_ETHERNET_TIMEOUT);
[ ... ]
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 8c16ef23cc10..9b0c109b3058 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1347,6 +1348,17 @@ int dsa_port_simple_hsr_join(struct dsa_switch *ds, int port,
> int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
> struct net_device *hsr);
>
> +/* Perform operations on a switch by sending it request in Ethernet
> + * frames and expecting a response in a frame.
> + */
> +struct dsa_inband {
> + struct completion completion;
> +};
> +
> +void dsa_inband_init(struct dsa_inband *inband);
> +void dsa_inband_complete(struct dsa_inband *inband);
> +int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
[Low]
Should the return type of dsa_inband_wait_for_completion() be
unsigned long to match wait_for_completion_timeout(), and should
timeout_ms be unsigned int?
The current declaration narrows the wait_for_completion_timeout() return
(unsigned long jiffies remaining) to int, and a negative timeout_ms
would be interpreted by msecs_to_jiffies() as MAX_JIFFY_OFFSET. qca8k
callers only check ret <= 0 at a 5 ms timeout so the truncation is
invisible today, but this helper is EXPORT_SYMBOL_GPL and intended as a
pattern for further DSA drivers, so fixing the signature later would be
an uAPI-ish change to a core API. Would a documented bool "timed_out"
return also work, since callers only care about zero vs non-zero?
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 9cb732f6b1e3..dc4e5cdb2f5b 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -1834,6 +1834,28 @@ int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
> }
> EXPORT_SYMBOL_GPL(dsa_port_simple_hsr_leave);
>
> +void dsa_inband_init(struct dsa_inband *inband)
> +{
> + init_completion(&inband->completion);
> +}
> +EXPORT_SYMBOL_GPL(dsa_inband_init);
> +
> +void dsa_inband_complete(struct dsa_inband *inband)
> +{
> + complete(&inband->completion);
> +}
> +EXPORT_SYMBOL_GPL(dsa_inband_complete);
> +
> +int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
> +{
> + unsigned long jiffies = msecs_to_jiffies(timeout_ms);
> +
> + reinit_completion(&inband->completion);
> +
> + return wait_for_completion_timeout(&inband->completion, jiffies);
> +}
> +EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
[Low]
The local variable is named jiffies, which shadows the global kernel
symbol jiffies from <linux/jiffies.h>. There is no runtime effect today
since the function does not reference the global, but any later addition
to this function that types "jiffies" would silently bind to the local
timeout value. Would renaming to timeout_j or timeout_jiffies be
preferable, especially given the helper is exported and likely to be
copied by other DSA drivers?
^ permalink raw reply
* Re: [PATCH v11] mm/page_alloc: boost watermarks on atomic allocation failure
From: Andrew Morton @ 2026-07-20 23:37 UTC (permalink / raw)
To: Qiliang Yuan
Cc: David Hildenbrand, Lorenzo Stoakes, Liam R. Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Axel Rasmussen, Yuanchu Xie, Wei Xu, Brendan Jackman,
Johannes Weiner, Zi Yan, Lance Yang, SeongJae Park,
Matthew Wilcox, netdev
In-Reply-To: <20260720-feat-mm-page_alloc-v11-v11-1-7376b02c27b3@gmail.com>
On Mon, 20 Jul 2026 16:15:48 +0800 Qiliang Yuan <realwujing@gmail.com> wrote:
> Atomic allocations (GFP_ATOMIC) are prone to failure under heavy memory
> pressure as they cannot enter direct reclaim.
>
> Handle these failures by introducing a watermark boost mechanism for
> atomic requests. Refactor boost_watermark() using an internal helper to
> support both fragmentation and atomic paths. Apply zone-proportional
> boosts (~0.1% of managed pages) for atomic allocations, while
> decoupling it from watermark_boost_factor.
Thanks for persisting with this.
You didn't retain Vlastimil's Reviewed-by: from v8?
This is very much a networking thing - they must have considered
similar things. But my not-very-energetic attempts to get input from
networking people have thus far failed.
> This failure signature keeps recurring in production: a host running
> a downstream 4.19 kernel logged 144 order-0 GFP_ATOMIC failures over a
> 4h15m window, all through the same NIC driver receive softirq path,
> across several unrelated network-facing services on the box. This
> confirms the underlying problem is real and ongoing. It does not by
> itself measure this patch's effect, since the fix has not been
> deployed on that fleet yet.
We'll of course be very interested in these results. Do you know
if/when they'll be available?
Anyway, let me get this into mm.git and linux-next so we can at least
parallelize wider testing with ongoing review.
^ permalink raw reply
* Re: [PATCH net-next 02/12] net: dsa: qca8K: Move queuing for request frame into the core
From: Jakub Kicinski @ 2026-07-20 23:37 UTC (permalink / raw)
To: lukeh
Cc: Jakub Kicinski, olteanv, davem, edumazet, pabeni, vivien.didelot,
gregory.clement, andrew, horms, f.fainelli, tobias,
cedric.jehasse, kieran, mh, max, christoph.mellauner, simon.gapp,
Ryan.Wilkins, mattias.forsblad, netdev, linux-kernel
In-Reply-To: <20260703-net-next-dsa-rmu-v1-2-a03febf20bb4@padl.com>
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: dsa: qca8K: Move queuing for request frame into the core
This patch introduces dsa_inband_request() in the DSA core which
reinitialises the completion, transmits the skb via dev_queue_xmit(),
and waits for a reply. The qca8k driver is converted to use this new
helper in place of the open-coded dev_queue_xmit() plus
dsa_inband_wait_for_completion() sequence.
[Low]
The subject line uses qca8K with a capital K, which is inconsistent
with qca8k as used everywhere else in the driver and in the rest of
the series.
The commit body refers to functions that do not exist:
Combine the queuing of the request and waiting for the completion into
one core helper. Add the function dsa_rmu_request() to perform this.
Access to statistics is not a strict request/reply, so the
dsa_rmu_wait_for_completion needs to be kept.
It is also no possible to combine dsa_rmu_request() and
dsa_rmu_wait_for_completion() since we need to avoid the race of
sending the request, receiving a reply, and the completion has not
been reinitialised because the schedule at decided to do other things.
The introduced symbol is dsa_inband_request(), and the preserved
symbol is dsa_inband_wait_for_completion(); should the message refer
to those names so that git log --grep for dsa_inband_request finds
this commit?
There are also a couple of small grammar issues in the same paragraph
("It is also no possible" and "the schedule at decided to do other
things").
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index fe6fb69b6f92..4da6094552f1 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
[ ... ]
> @@ -757,8 +747,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
> qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
> mgmt_eth_data->ack = false;
>
> - dev_queue_xmit(clear_skb);
> -
> dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> QCA8K_ETHERNET_TIMEOUT);
[High]
The other five hunks replace dev_queue_xmit() plus
dsa_inband_wait_for_completion() with a single dsa_inband_request()
call. This hunk deletes dev_queue_xmit(clear_skb) but leaves the bare
dsa_inband_wait_for_completion() in place, so no packet is ever sent
for the clear step.
A few consequences follow from that:
Does this leak clear_skb on every path through qca8k_phy_eth_command()
that reaches the exit: label? The skb is allocated by
qca8k_alloc_mdio_header() and, after this change, nothing ever hands
it to dev_queue_xmit() or frees it.
Does every PHY read/write through this driver now stall for
QCA8K_ETHERNET_TIMEOUT (200ms) at the exit: label, since the
completion is reinitialised but no request is ever queued for the
switch to reply to?
Does this also skip resetting the switch's MDIO master, which was the
purpose of the clear_skb step?
The next patch in the series, 68baa3a653c1 ("net: dsa: qca8k:
dsa_inband_request: More normal return values"), replaces the
dsa_inband_wait_for_completion() call with
dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
QCA8K_ETHERNET_TIMEOUT);
Would it be worth squashing or reordering so that bisection landing
on this commit does not see hangs and skb leaks on every management
frame PHY access?
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 9b0c109b3058..6b5aeb99ec3b 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1357,6 +1357,8 @@ struct dsa_inband {
>
> void dsa_inband_init(struct dsa_inband *inband);
> void dsa_inband_complete(struct dsa_inband *inband);
> +int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
> + int timeout_ms);
> int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
[ ... ]
^ permalink raw reply
* Re: [PATCH net-next 04/12] net: dsa: qca8k: Drop replies with wrong sequence numbers
From: Jakub Kicinski @ 2026-07-20 23:37 UTC (permalink / raw)
To: lukeh
Cc: Jakub Kicinski, olteanv, davem, edumazet, pabeni, vivien.didelot,
gregory.clement, andrew, horms, f.fainelli, tobias,
cedric.jehasse, kieran, mh, max, christoph.mellauner, simon.gapp,
Ryan.Wilkins, mattias.forsblad, netdev, linux-kernel
In-Reply-To: <20260703-net-next-dsa-rmu-v1-4-a03febf20bb4@padl.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: dsa: qca8k: Drop replies with wrong sequence numbers
The change makes qca8k_rw_reg_ack_handler() return early when the reply
sequence number does not match, rather than only setting ack on match.
The intent is to avoid signalling completion for a late reply that could
be mistaken for the next genuine reply.
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index b0cbe72c15b4..3b3fe9601617 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -185,9 +185,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> /* We can ignore odd value, we always round up them in the alloc function. */
> len *= sizeof(u16);
>
> - /* Make sure the seq match the requested packet */
> - if (get_unaligned_le32(&mgmt_ethhdr->seq) == mgmt_eth_data->seq)
> - mgmt_eth_data->ack = true;
> + /* Make sure the seq match the requested packet. If not, drop. */
> + if (get_unaligned_le32(&mgmt_ethhdr->seq) != mgmt_eth_data->seq)
> + return;
[High]
After this change, is mgmt_eth_data->ack ever assigned true anywhere in
the driver? A grep across drivers/net/dsa/qca/ shows five sites that
initialise it to false before dsa_inband_request(), five sites that read
it afterwards, and no remaining site that ever writes true.
The callers still do, for example in qca8k_read_eth():
mgmt_eth_data->ack = false;
ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
QCA8K_ETHERNET_TIMEOUT);
...
ack = mgmt_eth_data->ack;
mutex_unlock(&mgmt_eth_data->mutex);
if (ret)
return ret;
if (!ack)
return -EINVAL;
With the assignment gone, does every successful reply now leave ack ==
false and cause qca8k_read_eth(), qca8k_write_eth() and the read/write/
clear paths in qca8k_phy_eth_command() to return -EINVAL on their fast
path?
The higher-level callers (qca8k_bulk_read(), qca8k_bulk_write(),
qca8k_regmap_update_bits(), qca8k_internal_mdio_read() and
qca8k_internal_mdio_write()) fall back to the slow MII path on error, so
the device keeps functioning, but is the Ethernet management fast path
effectively disabled between this patch and its follow-up cleanup?
Would it be cleaner to fold the removal of the ack field (and the
matching if (!ack) return -EINVAL checks in every caller) into this same
commit, so that each commit in the series is self-contained and bisects
cleanly?
^ permalink raw reply
* Re: [PATCH net-next 07/12] net: dsa: qca8k: Pass error code from reply decoder to requester
From: Jakub Kicinski @ 2026-07-20 23:37 UTC (permalink / raw)
To: lukeh
Cc: Jakub Kicinski, olteanv, davem, edumazet, pabeni, vivien.didelot,
gregory.clement, andrew, horms, f.fainelli, tobias,
cedric.jehasse, kieran, mh, max, christoph.mellauner, simon.gapp,
Ryan.Wilkins, mattias.forsblad, netdev, linux-kernel
In-Reply-To: <20260703-net-next-dsa-rmu-v1-7-a03febf20bb4@padl.com>
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: dsa: qca8k: Pass error code from reply decoder to requester
This patch changes dsa_inband_complete() to accept a response buffer and
error code, and updates dsa_inband_request() to return that error code to
the caller. The qca8k driver call sites are updated to pass response
buffers through the new API and to use the returned status directly.
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 499cc8ef5dde..ffa3b1ba23be 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -167,6 +167,8 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> struct qca_mgmt_ethhdr *mgmt_ethhdr;
> u32 command;
> u8 len, cmd;
> + u32 data[4];
> + int err = 0;
> int i;
>
> mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
> @@ -191,7 +193,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> return;
>
> if (cmd == MDIO_READ) {
> - u32 *val = mgmt_eth_data->data;
> + u32 *val = &data[0];
>
> *val = get_unaligned_le32(&mgmt_ethhdr->mdio_data);
[High]
Can the decode loop overflow the on-stack data[4] buffer? The wire-supplied
length in QCA_HDR_MGMT_LENGTH is a 4-bit field with a maximum value of 15;
after this block in qca8k_rw_reg_ack_handler():
if (len == 15)
len++;
len *= sizeof(u16);
len can reach 32. Later the copy loop uses:
data_len = min_t(int, QCA_HDR_MGMT_DATA2_LEN /* 28 */,
len - QCA_HDR_MGMT_DATA1_LEN /* 4 */);
...
for (i = sizeof(u32); i <= data_len; i += sizeof(u32)) {
*val = get_unaligned_le32(data2);
val++;
data2++;
}
which writes val[1]..val[7] (28 bytes past *val), overrunning the 16-byte
u32 data[4] destination. This overflow existed in the previous storage
(mgmt_eth_data->data[4] on the heap) and this patch relocates the sink onto
the softirq call stack, where the overrun can clobber saved registers or
the return address. The driver advertises .max_raw_read = 32 through its
regmap, so a legitimate 32-byte bulk read is sufficient to reach this
path; a malformed reply is not required. Should the ack handler bound the
decode to sizeof(data)?
> @@ -213,7 +215,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> }
> }
>
> - dsa_inband_complete(&mgmt_eth_data->inband);
> + dsa_inband_complete(&mgmt_eth_data->inband, &data, sizeof(data), err);
> }
>
> static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> @@ -314,7 +316,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> {
> struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
> struct sk_buff *skb;
> - int err;
> + u32 data[4];
> int ret;
>
> skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> @@ -335,30 +337,25 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>
> ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> qca8k_mdio_header_fill_seq_num,
> + data, sizeof(data),
> QCA8K_ETHERNET_TIMEOUT);
> + if (ret < 0)
> + goto out;
>
> - *val = mgmt_eth_data->data[0];
> + *val = data[0];
> if (len > QCA_HDR_MGMT_DATA1_LEN)
> - memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> -
> - err = mgmt_eth_data->err;
> + memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
>
> +out:
> mutex_unlock(&mgmt_eth_data->mutex);
>
> - if (ret)
> - return ret;
> -
> - if (err)
> - return err;
> -
> - return 0;
> + return ret;
> }
[Critical]
Does qca8k_read_eth() now return a positive value on success? Looking at
dsa_inband_complete() and dsa_inband_request() in net/dsa/dsa.c after this
patch:
net/dsa/dsa.c:dsa_inband_complete() {
...
if (!err) {
spin_lock_bh(&inband->resp_lock);
resp_len = min(inband->resp_len, resp_len);
if (inband->resp && resp)
memcpy(inband->resp, resp, resp_len);
spin_unlock_bh(&inband->resp_lock);
inband->err = resp_len;
}
...
}
net/dsa/dsa.c:dsa_inband_request() {
...
return inband->err;
}
so a successful read returns sizeof(data) = 16, not 0. qca8k_read_eth()
propagates that positive value to its callers, but qca8k_regmap_update_bits_eth()
still uses:
ret = qca8k_read_eth(priv, reg, &val, sizeof(val));
if (ret)
return ret;
and qca8k_bulk_read() uses:
if (priv->mgmt_conduit && !qca8k_read_eth(...))
return 0;
Would qca8k_regmap_update_bits_eth() then return 16 as an error to regmap,
and would qca8k_bulk_read() unconditionally fall through to the MII path
even after a successful ethernet read?
> static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> {
[ ... ]
> @@ -573,7 +563,7 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
> struct sk_buff *read_skb, u32 *val)
> {
> struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
> - int err;
> + u32 data[4];
> int ret;
>
> if (!skb)
> @@ -581,17 +571,13 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
>
> ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> qca8k_mdio_header_fill_seq_num,
> + data, sizeof(data),
> QCA8K_ETHERNET_TIMEOUT);
>
> - err = mgmt_eth_data->err;
> -
> if (ret)
> return ret;
>
> - if (err)
> - return err;
> -
> - *val = mgmt_eth_data->data[0];
> + *val = data[0];
>
> return 0;
> }
[Critical]
With the new positive-on-success return from dsa_inband_request(), does
qca8k_phy_eth_busy_wait() now return before writing *val on every
successful transfer?
ret = dsa_inband_request(..., data, sizeof(data), ...);
if (ret)
return ret;
*val = data[0];
ret is 16 on success, so the function returns without touching *val. The
caller in qca8k_phy_eth_command() then feeds an uninitialised local into
read_poll_timeout():
drivers/net/dsa/qca/qca8k-8xxx.c:qca8k_phy_eth_command() {
...
u32 write_val, clear_val = 0, val;
...
ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
!(val & QCA8K_MDIO_MASTER_BUSY), 0,
QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
mgmt_eth_data, read_skb, &val);
...
}
Does the busy-wait loop then read val uninitialised on the first
iteration?
> @@ -604,8 +590,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
> struct qca8k_mgmt_eth_data *mgmt_eth_data;
> u32 write_val, clear_val = 0, val;
> struct net_device *mgmt_conduit;
> + u32 resp_data[4];
> int ret, ret1;
> - int err;
>
> if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
> return -EINVAL;
[ ... ]
> @@ -703,19 +682,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
> if (read) {
> ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
> qca8k_mdio_header_fill_seq_num,
> + resp_data, sizeof(resp_data),
> QCA8K_ETHERNET_TIMEOUT);
>
> - err = mgmt_eth_data->err;
> -
> if (ret)
> goto exit;
>
> - if (err) {
> - ret = err;
> - goto exit;
> - }
> -
> - ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> + ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> } else {
> kfree_skb(read_skb);
> }
[Critical]
In this read branch dsa_inband_request() returns 16 on a successful
response, so if (ret) goto exit; fires and the code never reaches:
ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
qca8k_phy_eth_command() therefore returns 16 (0x10) for every successful
internal PHY read via the ethernet fast path. Is this the intended
behaviour, given that the value is propagated as the register contents up
through qca8k_internal_mdio_read() and qca8k_mdio_read()?
The follow-up commit dacfa5deb518 ("net: dsa: qca8k: Update error
handling") converts each of these if (ret) checks to if (ret < 0) and
adds an explicit ret = 0; in qca8k_read_eth(). Would folding those changes
into this commit make the series bisectable, since between these two
commits every ethernet-fast-path MDIO read returns 16 and
qca8k_phy_eth_busy_wait() leaves its output argument uninitialised?
^ permalink raw reply
* Re: [PATCH 1/1] net: usb: aqc111: fix set_mac_address return value for bonding
From: Jakub Kicinski @ 2026-07-20 23:38 UTC (permalink / raw)
To: Hanson Wang; +Cc: netdev, linux-usb, oneukum
In-Reply-To: <20260703073936.462231-1-hanson.wang@ugreen.com>
On Fri, 3 Jul 2026 15:39:36 +0800 Hanson Wang wrote:
> - return aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
> - ETH_ALEN, net->dev_addr);
> + ret = aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN,
> + ETH_ALEN, net->dev_addr);
nit: sashiko suggest the indent of the continuation line is off, which
seems true. Please fix this and repost
^ permalink raw reply
* Re: [PATCH v2 net-next] ethtool: link 10000baseCR to SFF-8431, Appendix-E SFP+ DA
From: Jakub Kicinski @ 2026-07-20 23:43 UTC (permalink / raw)
To: Siddaraju DH
Cc: Michal Kubecek, Andrew Lunn, Maxime Chevallier, netdev,
Shubham Das, Balaji Chintalapalle, Vijay Srinivasan,
Magnus Lindberg, Niklas Damberg, Jonas Wirandi, Siddaraju DH
In-Reply-To: <20260703100537.1109838-1-siddaraju.dh@intel.com>
On Fri, 3 Jul 2026 15:35:37 +0530 Siddaraju DH wrote:
> Add comment to clarify the physical media 10000baseCR follows.
>
> 10000baseCR does not correspond to any IEEE 802.3 *base-CR PMD.
> It has no autonegotiation, no link training, and no mandatory FEC.
> The industry standard for this media type is SFF-8431 Appendix-E
> Direct Attach cable, also known as 10G_SFI_DA.
This patch fell out of patchwork, due to vacations and conferences.
Please repost.
^ permalink raw reply
* Re: [PATCH net-next] devlink: Replace strlcat() with seq_buf
From: Jakub Kicinski @ 2026-07-20 23:47 UTC (permalink / raw)
To: Ian Bridges
Cc: Jiri Pirko, David S. Miller, Eric Dumazet, Paolo Abeni,
Simon Horman, netdev, linux-kernel, linux-hardening
In-Reply-To: <akiCtALVjd9TF8Xl@dev>
On Fri, 3 Jul 2026 22:49:08 -0500 Ian Bridges wrote:
> In preparation for removing the strlcat() API[1], replace its uses in
> __devlink_compat_running_version().
>
> The function accumulates a variable number of version strings into a
> fixed buffer, which is what seq_buf is for. The seq_buf is anchored at
> the end of any existing string in the buffer and each version string
> is appended with a single seq_buf_printf(). The output is unchanged,
> including under truncation.
>
> Link: https://github.com/KSPP/linux/issues/370 [1]
It's fine.
^ permalink raw reply
* Re: [PATCH net v3 1/2] octeon_ep: fix skb frags overflow in the RX path
From: Jakub Kicinski @ 2026-07-20 23:49 UTC (permalink / raw)
To: maoyixie.tju
Cc: Jakub Kicinski, vburru, sedara, andrew+netdev, davem, edumazet,
pabeni, maciej.fijalkowski, netdev, linux-kernel
In-Reply-To: <20260704061511.2350737-2-maoyixie.tju@gmail.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeon_ep: fix skb frags overflow in the RX path
This change adds a check in __octep_oq_process_rx() so that packets whose
fragment count would exceed MAX_SKB_FRAGS are dropped before build_skb()
via octep_oq_drop_rx(), mirroring the existing build_skb-failure path.
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> index e6ebc7e44a00..bdbed58c7b2a 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_rx.c
> @@ -453,6 +453,15 @@ static int __octep_oq_process_rx(struct octep_device *oct,
>
> octep_oq_next_pkt(oq, buff_info, &read_idx, &desc_used);
>
> + if (buff_info->len > oq->max_single_buffer_size) {
> + u16 data_len = buff_info->len - oq->max_single_buffer_size;
> +
> + if (DIV_ROUND_UP(data_len, oq->buffer_size) > MAX_SKB_FRAGS) {
> + octep_oq_drop_rx(oq, buff_info, &read_idx, &desc_used);
> + continue;
> + }
> + }
> +
[High]
Before this new check runs, the outer octep_oq_next_pkt() has already
processed the head descriptor:
static void octep_oq_next_pkt(struct octep_oq *oq,
struct octep_rx_buffer *buff_info,
u32 *read_idx, u32 *desc_used)
{
dma_unmap_page(oq->dev, oq->desc_ring[*read_idx].buffer_ptr,
PAGE_SIZE, DMA_FROM_DEVICE);
buff_info->page = NULL;
...
}
At that point the head page has refcount 1 from dev_alloc_page() and no
struct sk_buff owns it yet, because build_skb() has not been called. resp_hw
is only a virtual pointer, so nothing else holds a reference to the page.
When this new path takes the continue via octep_oq_drop_rx(), the head
page is never freed — octep_oq_drop_rx() only walks fragment descriptors.
Does this drop path leak the head Rx page? The same question applies to
the pre-existing build_skb() failure path just below, but the new path
is device-triggerable in normal operation (packets larger than ~66 KB on
4 KB pages, which the commit message notes is reachable with data_len
close to 65535).
[High]
octep_oq_drop_rx() also appears to reuse the same head buff_info pointer
for every fragment descriptor:
static void octep_oq_drop_rx(struct octep_oq *oq,
struct octep_rx_buffer *buff_info,
u32 *read_idx, u32 *desc_used)
{
int data_len = buff_info->len - oq->max_single_buffer_size;
while (data_len > 0) {
octep_oq_next_pkt(oq, buff_info, read_idx, desc_used);
data_len -= oq->buffer_size;
}
}
The passed-in buff_info is the head slot's buff_info, not
&oq->buff_info[*read_idx] for each fragment. As a result
octep_oq_next_pkt() only clears the head slot's page pointer and never
NULLs the fragment slots' oq->buff_info[i].page.
Does this cause the fragment pages to be leaked on the next
octep_oq_refill(), where those non-NULL page pointers get overwritten
with a fresh dev_alloc_page() reference?
And if octep_oq_free_ring_buffers() runs before refill (netdev close or
reset), does it observe those stale page pointers and call
dma_unmap_page() on descriptor addresses that octep_oq_next_pkt() has
already unmapped inside drop_rx?
This defect predates this patch (it appears to come from the commit
that introduced octep_oq_drop_rx()), but this patch adds a new
device-triggerable call site (fragment count above MAX_SKB_FRAGS) that
would exercise it under normal jumbo Rx.
[Medium]
The new drop path silently discards the packet without updating any
counter:
> + if (DIV_ROUND_UP(data_len, oq->buffer_size) > MAX_SKB_FRAGS) {
> + octep_oq_drop_rx(oq, buff_info, &read_idx, &desc_used);
> + continue;
> + }
The commit message states:
octep_oq_drop_rx() consumes its descriptors like the build_skb failure
path.
The sibling build_skb() failure path increments oq->stats->alloc_failures,
but this new path does not, and ndo_get_stats64 (octep_get_stats64) is
not updated to bump rx_dropped either.
Should this drop bump a counter visible in ip -s link show or ethtool -S
so the loss is observable?
^ permalink raw reply
* Re: [PATCH net v3 2/2] octeon_ep_vf: fix skb frags overflow in the RX path
From: Jakub Kicinski @ 2026-07-20 23:49 UTC (permalink / raw)
To: maoyixie.tju
Cc: Jakub Kicinski, vburru, sedara, andrew+netdev, davem, edumazet,
pabeni, maciej.fijalkowski, netdev, linux-kernel
In-Reply-To: <20260704061511.2350737-3-maoyixie.tju@gmail.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeon_ep_vf: fix skb frags overflow in the RX path
This patch adds a MAX_SKB_FRAGS check in __octep_vf_oq_process_rx() so
packets that would require more fragments than the skb can hold are
dropped, and factors the descriptor-drain loop into a new
octep_vf_oq_drop_rx() helper that is shared with the napi_build_skb()
failure path.
> diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c
> index d98247408242..aa77b673ae3e 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_rx.c
> @@ -357,6 +357,29 @@ static inline u32 octep_vf_oq_next_idx(struct octep_vf_oq *oq, u32 idx)
> return (idx + 1 == oq->max_count) ? 0 : idx + 1;
> }
>
> +static void octep_vf_oq_drop_rx(struct octep_vf_oq *oq,
> + struct octep_vf_rx_buffer *buff_info,
> + u32 *read_idx, u32 *desc_used)
> +{
> + u16 data_len = buff_info->len - oq->max_single_buffer_size;
> +
> + (*desc_used)++;
> + *read_idx = octep_vf_oq_next_idx(oq, *read_idx);
> + while (data_len) {
> + dma_unmap_page(oq->dev, oq->desc_ring[*read_idx].buffer_ptr,
> + PAGE_SIZE, DMA_FROM_DEVICE);
> + buff_info = (struct octep_vf_rx_buffer *)
> + &oq->buff_info[*read_idx];
> + buff_info->page = NULL;
> + if (data_len < oq->buffer_size)
> + data_len = 0;
> + else
> + data_len -= oq->buffer_size;
> + (*desc_used)++;
> + *read_idx = octep_vf_oq_next_idx(oq, *read_idx);
> + }
> +}
[High]
Does octep_vf_oq_drop_rx() leak the pages backing the dropped
descriptors?
The helper dma_unmap_page()s each fragment descriptor and then just sets
buff_info->page = NULL, without ever calling put_page() on the page that
was allocated by dev_alloc_page() in octep_vf_oq_fill_ring_buffers() or
octep_vf_oq_refill().
Compare with octep_vf_oq_free_ring_buffers() in the same file, which
appears to be the intended release pattern:
for (i = 0; i < oq->max_count; i++) {
if (oq->buff_info[i].page) {
dma_unmap_page(oq->dev, desc_ring[i].buffer_ptr,
PAGE_SIZE, DMA_FROM_DEVICE);
put_page(oq->buff_info[i].page);
oq->buff_info[i].page = NULL;
desc_ring[i].buffer_ptr = 0;
}
}
After octep_vf_oq_drop_rx() returns, octep_vf_oq_refill() will overwrite
buff_info[refill_idx].page with a freshly allocated page from
dev_alloc_page(), so the previously-mapped page pointer is lost without
a matching put_page(), isn't it?
The head descriptor of the dropped packet looks affected too. The caller
in __octep_vf_oq_process_rx() has already stashed the head page's kernel
address into resp_hw and cleared buff_info->page:
resp_hw = page_address(buff_info->page);
buff_info->page = NULL;
On the two paths that now call octep_vf_oq_drop_rx() (the new
MAX_SKB_FRAGS-exceeded branch and the napi_build_skb() failure branch),
napi_build_skb() is either never called or returned NULL, so no skb has
taken ownership of resp_hw. Is the head page also leaked in these cases?
> @@ -431,25 +454,16 @@ static int __octep_vf_oq_process_rx(struct octep_vf_device *oct,
> struct skb_shared_info *shinfo;
> u16 data_len;
>
> + data_len = buff_info->len - oq->max_single_buffer_size;
> + if (DIV_ROUND_UP(data_len, oq->buffer_size) > MAX_SKB_FRAGS) {
> + octep_vf_oq_drop_rx(oq, buff_info, &read_idx, &desc_used);
> + continue;
> + }
> +
> skb = napi_build_skb((void *)resp_hw, PAGE_SIZE);
> if (!skb) {
> oq->stats->alloc_failures++;
> - desc_used++;
> - read_idx = octep_vf_oq_next_idx(oq, read_idx);
> - data_len = buff_info->len - oq->max_single_buffer_size;
> - while (data_len) {
> - dma_unmap_page(oq->dev, oq->desc_ring[read_idx].buffer_ptr,
> - PAGE_SIZE, DMA_FROM_DEVICE);
> - buff_info = (struct octep_vf_rx_buffer *)
> - &oq->buff_info[read_idx];
> - buff_info->page = NULL;
> - if (data_len < oq->buffer_size)
> - data_len = 0;
> - else
> - data_len -= oq->buffer_size;
> - desc_used++;
> - read_idx = octep_vf_oq_next_idx(oq, read_idx);
> - }
> + octep_vf_oq_drop_rx(oq, buff_info, &read_idx, &desc_used);
> continue;
> }
[High]
The napi_build_skb() failure path had the same missing put_page() before
this patch, so that part is a pre-existing issue.
The newly added MAX_SKB_FRAGS check reaches octep_vf_oq_drop_rx() on a
device-triggered path (buff_info->len comes from the response header),
so a misbehaving or malicious device that repeatedly sends packets whose
fragment count exceeds MAX_SKB_FRAGS would drop roughly 18 pages per
packet with no accounting.
Would it be reasonable to have octep_vf_oq_drop_rx() put_page() the head
page (or have the caller do so before invoking it) and put_page() each
fragment page after dma_unmap_page(), mirroring the release sequence in
octep_vf_oq_free_ring_buffers()?
^ permalink raw reply
* Re: [PATCH net v3 0/2] octeon_ep, octeon_ep_vf: fix skb frags overflow in the RX path
From: Jakub Kicinski @ 2026-07-20 23:50 UTC (permalink / raw)
To: Maoyi Xie
Cc: Veerasenareddy Burru, Sathesh Edara, Andrew Lunn,
David S . Miller, Eric Dumazet, Paolo Abeni, Maciej Fijalkowski,
netdev, linux-kernel
In-Reply-To: <20260704061511.2350737-1-maoyixie.tju@gmail.com>
On Sat, 4 Jul 2026 14:15:09 +0800 Maoyi Xie wrote:
> Both octeon_ep and octeon_ep_vf build an skb for a multi-buffer RX packet
> by adding one fragment per buffer_size chunk of a device-reported length.
> Neither bounds the count against MAX_SKB_FRAGS. A long packet yields about
> 18 fragments, one past the default MAX_SKB_FRAGS of 17, so
> skb_add_rx_frag() writes past shinfo->frags[].
>
> Each driver now checks the fragment count before it builds the skb and
> drops a packet that would not fit.
Why are you working on this driver?
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] net: dsa: initial support for MT7628 embedded switch
From: Jakub Kicinski @ 2026-07-20 23:52 UTC (permalink / raw)
To: joey
Cc: Jakub Kicinski, netdev, horms, pabeni, edumazet, davem, olteanv,
andrew, devicetree, robh, krzk+dt, conor+dt, arinc.unal,
Landen.Chao, dqfext, sean.wang, daniel
In-Reply-To: <20260704105659.140970-5-joey@tinyisr.com>
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: dsa: initial support for MT7628 embedded switch
This patch adds a DSA driver for the MT7628 embedded switch, which has
five 100Mbps user ports and a 1Gbps CPU port. tag_8021q is used to
isolate ports via the switch's 16-entry VLAN table, with the double tag
feature enabled to simulate VLAN-unaware behaviour.
> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 000000000000..eccdb05fce9b
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c
[ ... ]
> +static const struct dsa_switch_ops mt7628_switch_ops = {
> + .get_tag_protocol = mt7628_get_tag_proto,
> + .setup = mt7628_setup,
> + .teardown = mt7628_teardown,
> + .port_enable = mt7628_port_enable,
> + .port_disable = mt7628_port_disable,
> + .phylink_get_caps = mt7628_phylink_get_caps,
> + .tag_8021q_vlan_add = mt7628_dsa_8021q_vlan_add,
> + .tag_8021q_vlan_del = mt7628_dsa_8021q_vlan_del,
> +};
[Medium]
How is a user expected to place two of the user ports into a Linux bridge?
The ops table does not implement port_bridge_join or port_bridge_leave,
so dsa_switch_bridge_join() in net/dsa/switch.c returns -EOPNOTSUPP:
if (!ds->ops->port_bridge_join)
return -EOPNOTSUPP;
That makes ip link set swpN master brX fail for every user port, which
seems to contradict the commit message:
The switch hardware has a very limited 16 entry VLAN table.
Configuring VLANs is the only way to control switch forwarding.
Currently 6 entries are used by tag_8021q to isolate the ports.
Without a port_bridge_join hook, dsa_tag_8021q_bridge_join() is never
invoked, so there is no path to migrate a port's tag_8021q VID from its
standalone VID to a bridge VID.
Would wiring the tag_8021q helpers directly, similar to what
vitesse-vsc73xx-core.c does, be sufficient here:
.port_bridge_join = dsa_tag_8021q_bridge_join,
.port_bridge_leave = dsa_tag_8021q_bridge_leave,
Related bridge-lifecycle callbacks such as port_stp_state_set,
port_pre_bridge_flags, port_bridge_flags and port_fast_age also appear
to be absent, is that intentional for this initial submission?
^ permalink raw reply
* Re: [PATCH net-next v6 4/4] net: dsa: initial support for MT7628 embedded switch
From: Jakub Kicinski @ 2026-07-20 23:52 UTC (permalink / raw)
To: joey
Cc: Jakub Kicinski, netdev, horms, pabeni, edumazet, davem, olteanv,
andrew, devicetree, robh, krzk+dt, conor+dt, arinc.unal,
Landen.Chao, dqfext, sean.wang, daniel
In-Reply-To: <20260704105659.140970-5-joey@tinyisr.com>
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: dsa: initial support for MT7628 embedded switch
Add support for the MT7628 embedded switch. The switch has 5 built-in
100Mbps user ports and one 1Gbps CPU port.
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 4ab567c5bbafc..daa1d3d4ba609 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -63,6 +63,14 @@ config NET_DSA_MT7530_MMIO
> are directly mapped into the SoCs register space rather than being
> accessible via MDIO.
>
> +config NET_DSA_MT7628
> + tristate "MediaTek MT7628 Embedded Ethernet switch support"
> + select NET_DSA_TAG_MT7628
> + select MEDIATEK_FE_SOC_PHY
> + help
[Severity: Low]
Can this lead to a build or link failure on randconfig? It looks like this
is missing a select REGMAP_MMIO and depends on HAS_IOMEM, which might cause
devm_regmap_init_mmio() to be unresolved when CONFIG_REGMAP_MMIO is not set.
> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> new file mode 100644
> index 0000000000000..eccdb05fce9b7
> --- /dev/null
> +++ b/drivers/net/dsa/mt7628.c
> @@ -0,0 +1,649 @@
> [ ... ]
> +static int mt7628_setup_internal_mdio(struct dsa_switch *ds)
> +{
> + struct mt7628_esw *esw = ds->priv;
> + struct device *dev = ds->dev;
> + struct mii_bus *bus;
> +
> + bus = devm_mdiobus_alloc(dev);
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "MT7628 internal MDIO bus";
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
> + bus->priv = esw;
> + bus->read = mt7628_mii_read;
> + bus->write = mt7628_mii_write;
> + bus->parent = dev;
> +
> + ds->user_mii_bus = bus;
> + bus->phy_mask = ~ds->phys_mii_mask;
> +
> + return devm_mdiobus_register(dev, bus);
> +}
[Severity: High]
Does this cause a resource leak and sysfs name collision if the switch setup
callback runs multiple times?
If the DSA master device unbinds and rebinds, dsa_tree_teardown() and
dsa_tree_setup() can be triggered again. Because devm_mdiobus_alloc() and
devm_mdiobus_register() tie the resource to the platform device's lifetime
rather than the DSA tree setup/teardown cycle, the first registration will
leak, and the second might fail with -EEXIST.
> [ ... ]
> +static void mt7628_switch_init(struct dsa_switch *ds)
> +{
> + struct mt7628_esw *esw = ds->priv;
> +
> + regmap_write(esw->regmap, MT7628_ESW_REG_FCT0,
> + FIELD_PREP(MT7628_ESW_FCT0_DROP_SET_TH, 0x50) |
> + FIELD_PREP(MT7628_ESW_FCT0_DROP_RLS_TH, 0x78) |
> + FIELD_PREP(MT7628_ESW_FCT0_FC_SET_TH, 0xa0) |
> + FIELD_PREP(MT7628_ESW_FCT0_FC_RLS_TH, 0xc8));
[Severity: High]
Can traffic leak across isolated ports (like WAN and LAN) during boot?
It looks like mt7628_switch_init() resets the switch but fails to explicitly
set the MT7628_ESW_POC0_PORT_DISABLE bits for the user ports. Since the
DSA core relies on port_disable during ndo_close and doesn't automatically
disable ports upon initialization, does the hardware default to acting as
an unmanaged switch, bridging all networks until the interfaces are brought
up administratively?
^ permalink raw reply
* Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup
From: Emil Tsalapatis @ 2026-07-20 23:58 UTC (permalink / raw)
To: Nnamdi Onyeyiri
Cc: bpf, davem, edumazet, horms, jakub, jiayuan.chen, john.fastabend,
kuba, kuniyu, ncardwell, netdev, pabeni, sashiko-reviews,
linux-kernel
In-Reply-To: <al6iBv_Yn4YSCmWc@localhost.localdomain>
On Mon Jul 20, 2026 at 6:53 PM EDT, Nnamdi Onyeyiri wrote:
> On Mon, Jul 20, 2026 at 05:16:08PM -0400, Emil Tsalapatis wrote:
>> On Mon Jul 20, 2026 at 1:15 PM EDT, Nnamdi Onyeyiri wrote:
>> > recvfrom()/recv() are documented as only returning EAGAIN for blocking sockets
>> > when they have a receive timeout configured. However, adding a blocking
>> > ipv4 tcp socket without a receive timeout to a sockmap will cause EAGAIN errors
>> > sporadically. A socket with a receive timeout may return EAGAIN before the
>> > timeout expires.
>> >
>> > There are 2 code paths affected by this:
>> >
>> > 1. tcp_bpf_recvmsg() - Used when the socket has been added to a sockmap
>> > that has no verdict program attached.
>> >
>> > 2. tcp_bpf_recvmsg_parser() - Used when the socket has been added to a
>> > sockmap that has a verdict program. To reproduce this issue, it is
>> > enough for the verdict program to do nothing but return SK_PASS.
>> >
>> > In both cases this happens when tcp_msg_wait_data() wakes spuriously
>> > (returning 0). To fix it, we now loop back to msg_bytes_ready instead
>> > of returning -EAGAIN on spurious wakeup.
>> >
>> > To ensure the looping does not cause sockets with a SO_RCVTIMEO set to
>> > wait excessively long, tcp_msg_wait_data() now takes a pointer to timeo,
>> > allowing sk_wait_event() to update it as appropriate.
>> >
>> > The logic in tcp_bpf_recvmsg_parser() that allow it to handle signals,
>> > socket errors and closuers in its loop was also added to tcp_bpf_recvmsg().
>> >
>> > Signed-off-by: Nnamdi Onyeyiri <nnamdio@gmail.com>
>> > ---
>> > net/ipv4/tcp_bpf.c | 69 ++++++++++++++++++++++++++++++++++++++++------
>> > 1 file changed, 60 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
>> > index cc0bd73f36b6..aa5c5d741599 100644
>> > --- a/net/ipv4/tcp_bpf.c
>> > +++ b/net/ipv4/tcp_bpf.c
>> > @@ -179,7 +179,7 @@ EXPORT_SYMBOL_GPL(tcp_bpf_sendmsg_redir);
>> >
>> > #ifdef CONFIG_BPF_SYSCALL
>> > static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>> > - long timeo)
>> > + long *timeo)
>> > {
>> > DEFINE_WAIT_FUNC(wait, woken_wake_function);
>> > int ret = 0;
>> > @@ -187,12 +187,12 @@ static int tcp_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>> > if (sk->sk_shutdown & RCV_SHUTDOWN)
>> > return 1;
>> >
>> > - if (!timeo)
>> > + if (!*timeo)
>> > return ret;
>> >
>> > add_wait_queue(sk_sleep(sk), &wait);
>> > sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>> > - ret = sk_wait_event(sk, &timeo,
>> > + ret = sk_wait_event(sk, timeo,
>> > !list_empty(&psock->ingress_msg) ||
>> > !skb_queue_empty_lockless(&sk->sk_receive_queue), &wait);
>> > sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>> > @@ -229,6 +229,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> > int copied_from_self = 0;
>> > int copied = 0;
>> > u32 seq;
>> > + long timeo;
>> >
>> > if (unlikely(flags & MSG_ERRQUEUE))
>> > return inet_recv_error(sk, msg, len);
>> > @@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> > }
>> > }
>> >
>> > + timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > +
>> > msg_bytes_ready:
>> > copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
>> > /* The typical case for EFAULT is the socket was gracefully
>> > @@ -280,7 +283,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> > }
>> > seq += copied_from_self;
>> > if (!copied) {
>> > - long timeo;
>> > int data;
>> >
>> > if (sock_flag(sk, SOCK_DONE))
>> > @@ -299,7 +301,6 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> > goto out;
>> > }
>> >
>> > - timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > if (!timeo) {
>> > copied = -EAGAIN;
>> > goto out;
>> > @@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>> > goto out;
>> > }
>> >
>> > - data = tcp_msg_wait_data(sk, psock, timeo);
>> > + data = tcp_msg_wait_data(sk, psock, &timeo);
>> > if (data < 0) {
>> > copied = data;
>> > goto unlock;
>> > }
>> > if (data && !sk_psock_queue_empty(psock))
>> > goto msg_bytes_ready;
>> > + if (!data && timeo > 0)
>> > + goto msg_bytes_ready;
>> > copied = -EAGAIN;
>> > }
>> > out:
>> > @@ -355,6 +358,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>> > {
>> > struct sk_psock *psock;
>> > int copied, ret;
>> > + long timeo;
>> >
>> > if (unlikely(flags & MSG_ERRQUEUE))
>> > return inet_recv_error(sk, msg, len);
>> > @@ -371,14 +375,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>> > return tcp_recvmsg(sk, msg, len, flags);
>> > }
>> > lock_sock(sk);
>> > +
>> > + timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > +
>> > msg_bytes_ready:
>> > copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>> > if (!copied) {
>> > - long timeo;
>> > int data;
>> >
>> > - timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>> > - data = tcp_msg_wait_data(sk, psock, timeo);
>> > + if (sock_flag(sk, SOCK_DONE)) {
>> > + ret = 0;
>> > + goto unlock;
>> > + }
>> > +
>> > + if (sk->sk_err) {
>> > + if (!sk_psock_queue_empty(psock))
>> > + goto msg_bytes_ready;
>> > + if (!skb_queue_empty(&sk->sk_receive_queue)) {
>> > + release_sock(sk);
>> > + sk_psock_put(sk, psock);
>> > + return tcp_recvmsg(sk, msg, len, flags);
>> > + }
>> > + ret = sock_error(sk);
>> > + goto unlock;
>> > + }
>> > +
>> > + if (sk->sk_shutdown & RCV_SHUTDOWN) {
>> > + if (!sk_psock_queue_empty(psock))
>> > + goto msg_bytes_ready;
>> > + if (!skb_queue_empty(&sk->sk_receive_queue)) {
>> > + release_sock(sk);
>> > + sk_psock_put(sk, psock);
>> > + return tcp_recvmsg(sk, msg, len, flags);
>> > + }
>> > + ret = 0;
>> > + goto unlock;
>>
>> These two error handling routines above look identical. Can you refactor
>> them?
>>
>
> Will do. My understanding is the same logic is needed to address the
> issue Sashiko raised with the SOCK_DONE check as well.
>
>> > + }
>> > +
>> > + if (sk->sk_state == TCP_CLOSE) {
>> > + ret = -ENOTCONN;
>> > + goto unlock;
>> > + }
>> > +
>> > + if (!timeo) {
>> > + ret = -EAGAIN;
>> > + goto unlock;
>> > + }
>> > +
>>
>> Since this handling (which Sashiko flags by the way, correctly AFAICT)
>> are taken from tcp_bpf_recvmsg, there is obvious overlap between the two
>> functions. Please factor those out so that they share the logic between
>> them.
>>
>> pw-bot: cr
>>
>
> Sashiko highlighted the "if (!timeo)" and signal_pending early returns
> when MSG_DONTWAIT is set, but I think I'm missing part of the picture.
> By the time we reach these branches, haven't we already checked for data
> in sk_receive_queue (line 372, after the patch is applied to 7.2-rc2)
> [copied below for ease of viewing]:
>
> if (!skb_queue_empty(&sk->sk_receive_queue) &&
> sk_psock_queue_empty(psock)) {
> sk_psock_put(sk, psock);
> return tcp_recvmsg(sk, msg, len, flags);
> }
>
> and in the psock (line 382) [again copied below for viewing]:
>
> copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
If I'm understanding your question correctly, and AFAICT:
sk_msg_recvmsg only drains the psock out of sk_msg data. If any data is
still in the backing struct sock but has _not_ been drained into the psock
it gets missed under the new code.
>
>> > + if (signal_pending(current)) {
>> > + ret = sock_intr_errno(timeo);
>> > + goto unlock;
>> > + }
>> > +
>> > + data = tcp_msg_wait_data(sk, psock, &timeo);
>> > if (data < 0) {
>> > ret = data;
>> > goto unlock;
>> > @@ -390,6 +439,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>> > sk_psock_put(sk, psock);
>> > return tcp_recvmsg(sk, msg, len, flags);
>> > }
>> > + if (!data && timeo > 0)
>> > + goto msg_bytes_ready;
>> > copied = -EAGAIN;
>> > }
>> > ret = copied;
>>
^ permalink raw reply
* Re: [PATCH] net: xscale: add missing MODULE_DEVICE_TABLE()
From: Jakub Kicinski @ 2026-07-21 0:19 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Richard Cochran, netdev, linux-kernel
In-Reply-To: <20260704122722.5715-1-pengpeng@iscas.ac.cn>
On Sat, 4 Jul 2026 20:27:22 +0800 Pengpeng Hou wrote:
> The driver has an OF match table wired to .of_match_table, but does
> not export the table with MODULE_DEVICE_TABLE().
>
> Add the missing MODULE_DEVICE_TABLE(of, ...) entry so module alias
> information is generated for OF based module autoloading.
>
> This is a source-level fix. It does not claim dynamic hardware
> reproduction; the evidence is the driver-owned match table, its use by
> the platform driver, and the missing module alias publication.
The last paragraph is phrased strangely. Please write it.
The usual phrase is something like
"Found by code inspection, I don't have access to this HW."
^ permalink raw reply
* Re: [PATCH] net: alacritech: add missing MODULE_DEVICE_TABLE()
From: Jakub Kicinski @ 2026-07-21 0:19 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Lino Sanfilippo, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, netdev, linux-kernel
In-Reply-To: <20260704152053.49780-1-pengpeng@iscas.ac.cn>
On Sat, 4 Jul 2026 23:20:53 +0800 Pengpeng Hou wrote:
> The driver has a match table for the pci bus wired into its driver
> structure, but the table is not exported with MODULE_DEVICE_TABLE().
>
> Add the missing MODULE_DEVICE_TABLE() entry so module alias information
> is generated for automatic module loading.
>
> This is a source-level fix. It does not claim dynamic hardware
> reproduction; the evidence is the driver-owned match table, its use by
> the driver registration structure, and the missing module alias
> publication.
ditto
^ permalink raw reply
* Re: [PATCH net v2] bnge/bng_re: fix ring ID widths
From: Jakub Kicinski @ 2026-07-21 0:20 UTC (permalink / raw)
To: Vikas Gupta
Cc: davem, edumazet, pabeni, andrew+netdev, horms, netdev,
linux-kernel, linux-rdma, leonro, jgg, bhargava.marreddy,
rahul-rg.gupta, vsrama-krishna.nemani, rajashekar.hudumula,
ajit.khaparde, Siva Reddy Kallam, Dharmender Garg,
Yendapally Reddy Dhananjaya Reddy
In-Reply-To: <20260704164747.1995227-1-vikas.gupta@broadcom.com>
On Sat, 4 Jul 2026 22:17:47 +0530 Vikas Gupta wrote:
> Firmware requires more than 16 bits to address TX ring IDs for its
> internal QP management. Widen the associated HSI ring ID fields to
> 32 bits. The values firmware assigns remain within 24 bits, bounded
> by the hardware doorbell XID field.
>
> RX, completion, and NQ ring IDs are unaffected and remain 16-bit.
The netdev patch queue has overflown. Please repost.
^ permalink raw reply
* Re: [PATCH net-next v7 4/5] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA
From: Jakub Kicinski @ 2026-07-21 0:22 UTC (permalink / raw)
To: Linus Walleij
Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marek Vasut, Simon Horman,
Russell King, netdev, devicetree
In-Reply-To: <20260704-ks8995-to-ksz8-v7-4-2af0eaa545a8@kernel.org>
On Sat, 04 Jul 2026 21:39:36 +0200 Linus Walleij wrote:
> This adds support for the Microchip KSZ8995XA also known as the
> Micrel KS8995XA switch to the KSZ driver.
>
> Notice: there are also KSZ8995E and KSZ8995MA. These are BOTH
> different from the KSZ8995XA.
>
> The helper macros are named ksz_is_ksz8995xa() to make it
> possible to add E and MA support in the future.
Clang says:
../drivers/net/dsa/microchip/ksz8.c:263:13: warning: variable 'reg_4q' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~
../drivers/net/dsa/microchip/ksz8.c:288:29: note: uninitialized use occurs here
288 | ret = ksz_prmw8(dev, port, reg_4q, mask_4q, data_4q);
| ^~~~~~
../drivers/net/dsa/microchip/ksz8.c:263:9: note: remove the 'if' if its condition is always false
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
264 | /* This switch has no 4way split support */
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
265 | mask_2q = KSZ8795_PORT_2QUEUE_SPLIT_EN;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
266 | reg_2q = REG_PORT_CTRL_0;
| ~~~~~~~~~~~~~~~~~~~~~~~~~
267 | } else {
| ~~~~~~
../drivers/net/dsa/microchip/ksz8.c:237:11: note: initialize the variable 'reg_4q' to silence this warning
237 | u8 reg_4q, reg_2q;
| ^
| = '\0'
../drivers/net/dsa/microchip/ksz8.c:263:13: warning: variable 'mask_4q' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~
../drivers/net/dsa/microchip/ksz8.c:288:37: note: uninitialized use occurs here
288 | ret = ksz_prmw8(dev, port, reg_4q, mask_4q, data_4q);
| ^~~~~~~
../drivers/net/dsa/microchip/ksz8.c:263:9: note: remove the 'if' if its condition is always false
263 | } else if (ksz_is_ksz8995xa(dev)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
264 | /* This switch has no 4way split support */
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
265 | mask_2q = KSZ8795_PORT_2QUEUE_SPLIT_EN;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
266 | reg_2q = REG_PORT_CTRL_0;
| ~~~~~~~~~~~~~~~~~~~~~~~~~
267 | } else {
| ~~~~~~
../drivers/net/dsa/microchip/ksz8.c:236:12: note: initialize the variable 'mask_4q' to silence this warning
236 | u8 mask_4q, mask_2q;
| ^
| = '\0'
^ permalink raw reply
* Re: [PATCH] net: pcs: xpcs-plat: fix runtime PM initialization
From: Jakub Kicinski @ 2026-07-21 0:24 UTC (permalink / raw)
To: Coia Prant
Cc: netdev, Andrew Lunn, Heiner Kallweit, Russell King,
David S . Miller, Eric Dumazet, Paolo Abeni, Serge Semin,
linux-kernel, stable
In-Reply-To: <20260704214808.1566710-1-coiaprant@gmail.com>
On Sun, 5 Jul 2026 05:48:08 +0800 Coia Prant wrote:
> The driver calls `pm_runtime_set_active()` before runtime PM is enabled,
> and before the clock is prepared and enabled.
>
> This causes the clock to be unprepared/disabled later in the suspend
> callback even though it was never prepared/enabled, resulting in warnings:
>
> clk_csr already disabled
> clk_csr already unprepared
>
> Fix this by setting the initial runtime PM status to SUSPENDED instead
> of ACTIVE.
>
> The clock will be properly enabled when the device is first resumed
> via runtime PM (e.g., during MDIO access).
Seems a bit odd that this hasn't been discovered until now.
Could you add more details about your platform and maybe
a hypothesis why we haven't noticed?
^ 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