Message-ID: <CANn89i+FkSb-SNxUu_s9RzjM1qKG4uv5KZau2hhFDEPYXo-=Nw@mail.gmail.com>
Date: Tue, 29 Jul 2025 06:36:58 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: "Karumanchi, Vineeth" <vineeth@....com>
Cc: vineeth.karumanchi@....com, git@....com, vinicius.gomes@...el.com, 
	jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us, 
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net] net: taprio: Validate offload support using
 NETIF_F_HW_TC in hw_features

On Tue, Jul 29, 2025 at 3:41 AM Karumanchi, Vineeth <vineeth@....com> wrote:
>
> Hi Eric,
>
> On 7/29/2025 12:15 PM, Eric Dumazet wrote:
> > On Mon, Jul 28, 2025 at 11:10 PM Vineeth Karumanchi
> > <vineeth.karumanchi@....com> wrote:
> >>
> >> The current taprio offload validation relies solely on the presence of
> >> .ndo_setup_tc, which is insufficient. Some IP versions of a driver expose
> >> .ndo_setup_tc but lack actual hardware offload support for taprio.
> >>
> >> To address this, add a check for NETIF_F_HW_TC in netdev->hw_features.
> >> This ensures that taprio offload is only enabled on devices that
> >> explicitly advertise hardware traffic control capabilities.
> >>
> >> Note: Some drivers already set NETIF_F_HW_TC alongside .ndo_setup_tc.
> >> Follow-up patches will be submitted to update remaining drivers if this
> >> approach is accepted.
> >
> > Hi Vineeth
> >
> > Could you give more details ? "Some IP versions of a driver" and "Some
> > drivers" are rather vague.
>
> At present, I’m only familiar with the GEM IP, which supports TSN Qbv in
> its later versions. The GEM implementations found in Zynq and ZynqMP
> devices do not support TSN Qbv, whereas the updated versions integrated
> into Versal devices do offer TSN Qbv support.

Is this an out-of-tree driver ? I do not find macb_taprio_setup_replace()

I think most drivers should return -EOPNOTSUPP in this case.

>
> >
> > Also what happens without your patch ? Freeze / crash, or nothing at all ?
> >
>
> Crash!
>
> root $# tc qdisc replace dev end0 parent root handle 100 taprio num_tc 2
> map 0 1 queues 1@0 1@1 base-time 500 sched-entry S 0x1 100000
> sched-entry S 0x2 50000 flags 2 cycle-time 250000
>
> [   31.667952] Internal error: synchronous external abort:
> 0000000096000210 [#1]  SMP
> [   31.675529] Modules linked in:
> [   31.678576] CPU: 0 UID: 0 PID: 660 Comm: tc Not tainted
> 6.16.0-rc6-01628-g2933e636b919-dirty #14 NONE
> [   31.687870] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
> [   31.692819] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [   31.699771] pc : hw_readl_native+0x8/0x10
> [   31.703782] lr : macb_taprio_setup_replace+0x2b0/0x508
> [   31.708912] sp : ffff80008223b4f0
> [   31.712211] x29: ffff80008223b570 x28: 0000000000000040 x27:
> 000000003b9aca00
> [   31.719346] x26: 0000000000000000 x25: ffff00080b5b7048 x24:
> 00000000000003e8
> [   31.726473] x23: 0000000000000003 x22: ffff00080b5b49c0 x21:
> ffff000808727c80
> [   31.733600] x20: ffff000800150208 x19: 0000000000000000 x18:
> 020c49ba5e353f7d
> [   31.740727] x17: 0000000000000002 x16: 00000000000249f0 x15:
> 0044b82fa09b5a53
> [   31.747854] x14: 00000000ee6b27ff x13: 000000003e7fe4a7 x12:
> ffff000808727c90
> [   31.754981] x11: 0000000000000002 x10: 0000000000024be4 x9 :
> 0000000000000000
> [   31.762108] x8 : 0000000000000002 x7 : 00000000000061a8 x6 :
> 000000000000186a
> [   31.769235] x5 : 0000000000000000 x4 : 0000000000018894 x3 :
> 0000000000000000
> [   31.776362] x2 : ffff800080928d78 x1 : ffff80008190d880 x0 :
> ffff80008190d000
> [   31.783490] Call trace:
> [   31.785921]  hw_readl_native+0x8/0x10 (P)
> [   31.789922]  macb_setup_tc+0x13c/0x190
> [   31.793663]  taprio_change+0x768/0xb90
> [   31.797405]  taprio_init+0x1d0/0x280
> [   31.800973]  qdisc_create+0x114/0x40c
> [   31.804627]  tc_modify_qdisc+0x4c8/0x804
> [   31.808542]  rtnetlink_rcv_msg+0x284/0x374
> [   31.812631]  netlink_rcv_skb+0x60/0x130
> [   31.816459]  rtnetlink_rcv+0x18/0x24
> [   31.820027]  netlink_unicast+0x1e8/0x304
> [   31.823942]  netlink_sendmsg+0x168/0x3a4
> [   31.827857]  ____sys_sendmsg+0x220/0x268
> [   31.831772]  ___sys_sendmsg+0xb0/0x108
> [   31.835514]  __sys_sendmsg+0x9c/0x100
> [   31.839168]  __arm64_sys_sendmsg+0x24/0x30
> [   31.843257]  invoke_syscall+0x48/0x10c
> [   31.846999]  el0_svc_common.constprop.0+0xc0/0xe0
> [   31.851695]  do_el0_svc+0x1c/0x28
> [   31.855003]  el0_svc+0x34/0x104
> [   31.858136]  el0t_64_sync_handler+0x10c/0x138
> [   31.862485]  el0t_64_sync+0x198/0x19c
> [   31.866144] Code: 88dffc00 88dffc00 f9400000 8b21c001 (b9400020)
> [   31.872227] ---[ end trace 0000000000000000 ]---
> [   31.876836] note: tc[660] exited with irqs disabled
> Segmentation fault
>
>
> >>
> >> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@....com>
> >
> > Patches targeting net branch should include a Fixes: tag.
> >
> > Thanks
> >
> >> ---
> >>   net/sched/sch_taprio.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> >> index 2b14c81a87e5..a797995bdc8d 100644
> >> --- a/net/sched/sch_taprio.c
> >> +++ b/net/sched/sch_taprio.c
> >> @@ -1506,7 +1506,7 @@ static int taprio_enable_offload(struct net_device *dev,
> >>          struct tc_taprio_caps caps;
> >>          int tc, err = 0;
> >>
> >> -       if (!ops->ndo_setup_tc) {
> >> +       if (!ops->ndo_setup_tc || !(dev->hw_features & NETIF_F_HW_TC)) {
> >>                  NL_SET_ERR_MSG(extack,
> >>                                 "Device does not support taprio offload");
> >>                  return -EOPNOTSUPP;
> >> --
> >> 2.34.1
> >>
>
> --
> 🙏 vineeth
>

Powered by blists - more mailing lists