Re: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat netlink attributes

Re: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat netlink attributes

[Jakub Kicinski]

On Sun, 3 May 2026 10:35:27 +0300 Danielle Ratson wrote:
> --- a/Documentation/netlink/specs/rt-link.yaml
> +++ b/Documentation/netlink/specs/rt-link.yaml
> @@ -1700,6 +1700,9 @@ attribute-sets:
>        -
>          name: backup-nhid
>          type: u32
> +      -
> +        name: neigh-forward-grat
> +        type: flag

I think this should be u8 ? neigh-vlan-suppress looks buggy too

flag is a type without a payload, the presence of the attr is
the entire information

None of the AIs seem to catch this, I think you may have over-split
this submission a little bit. This patch may have been better off
squashed into patch 4 ?
-- 
pw-bot: cr

Re: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat netlink attributes

[Ido Schimmel]

On Tue, May 05, 2026 at 07:00:44PM -0700, Jakub Kicinski wrote:
> On Sun, 3 May 2026 10:35:27 +0300 Danielle Ratson wrote:
> > --- a/Documentation/netlink/specs/rt-link.yaml
> > +++ b/Documentation/netlink/specs/rt-link.yaml
> > @@ -1700,6 +1700,9 @@ attribute-sets:
> >        -
> >          name: backup-nhid
> >          type: u32
> > +      -
> > +        name: neigh-forward-grat
> > +        type: flag
> 
> I think this should be u8 ? neigh-vlan-suppress looks buggy too

I pointed this out during internal review, but assumed I am missing
something since almost all the attributes use flag when they are in fact
u8. We can fix neigh-forward-grat to use u8 in v2 and change the rest in
net. To be clear, I believe the following should be converted from flag
to u8:

mode, guard, protect, fast-leave, learning, unicast-flood, proxyarp,
learning-sync, proxyarp-wifi, mcast-flood, mcast-to-ucast, vlan-tunnel,
bcast-flood, neigh-suppress, isolated, mrp-ring-open, mrp-in-open,
locked, mab, neigh-vlan-suppress

> flag is a type without a payload, the presence of the attr is
> the entire information
> 
> None of the AIs seem to catch this, I think you may have over-split
> this submission a little bit. This patch may have been better off
> squashed into patch 4 ?

Related: The AI also did not catch that the spec was missing (easy to
forget for rtnetlink). Do you think it's worth adding to review-prompts?

RE: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat netlink attributes

[Danielle Ratson]

> -----Original Message-----
> From: Ido Schimmel <idosch@nvidia.com>
> Sent: Wednesday, 6 May 2026 10:04
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Danielle Ratson <danieller@nvidia.com>; netdev@vger.kernel.org;
> donald.hunter@gmail.com; davem@davemloft.net; edumazet@google.com;
> pabeni@redhat.com; horms@kernel.org; razor@blackwall.org;
> andrew+netdev@lunn.ch; shuah@kernel.org; ast@fiberby.net;
> liuhangbin@gmail.com; daniel@iogearbox.net; Andy Roulin
> <aroulin@nvidia.com>; fmaurer@redhat.com; sdf.kernel@gmail.com;
> sd@queasysnail.net; kees@kernel.org; nickgarlis@gmail.com;
> amorenoz@redhat.com; alasdair@mcwilliam.dev;
> johannes.wiesboeck@aisec.fraunhofer.de; Petr Machata
> <petrm@nvidia.com>; linux-kernel@vger.kernel.org; bridge@lists.linux.dev;
> linux-kselftest@vger.kernel.org
> Subject: Re: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat
> netlink attributes
> 
> On Tue, May 05, 2026 at 07:00:44PM -0700, Jakub Kicinski wrote:
> > On Sun, 3 May 2026 10:35:27 +0300 Danielle Ratson wrote:
> > > --- a/Documentation/netlink/specs/rt-link.yaml
> > > +++ b/Documentation/netlink/specs/rt-link.yaml
> > > @@ -1700,6 +1700,9 @@ attribute-sets:
> > >        -
> > >          name: backup-nhid
> > >          type: u32
> > > +      -
> > > +        name: neigh-forward-grat
> > > +        type: flag
> >
> > I think this should be u8 ? neigh-vlan-suppress looks buggy too
> 
> I pointed this out during internal review, but assumed I am missing something
> since almost all the attributes use flag when they are in fact u8. We can fix

This is in fact the reason why I also changed it myself to use flag before sending.

> neigh-forward-grat to use u8 in v2 and change the rest in net. To be clear, I
> believe the following should be converted from flag to u8:
> 
> mode, guard, protect, fast-leave, learning, unicast-flood, proxyarp, learning-
> sync, proxyarp-wifi, mcast-flood, mcast-to-ucast, vlan-tunnel, bcast-flood,
> neigh-suppress, isolated, mrp-ring-open, mrp-in-open, locked, mab, neigh-
> vlan-suppress
> 

So should we proceed as Ido suggested?

> > flag is a type without a payload, the presence of the attr is the
> > entire information
> >
> > None of the AIs seem to catch this, I think you may have over-split
> > this submission a little bit. This patch may have been better off
> > squashed into patch 4 ?

It seems like the patch has enough content, but I can squash. I guess ill split the commit between patches 4 and 5 accordingly.

> 
> Related: The AI also did not catch that the spec was missing (easy to forget for
> rtnetlink). Do you think it's worth adding to review-prompts?

Re: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat netlink attributes

[Jakub Kicinski]

On Wed, 6 May 2026 08:31:01 +0000 Danielle Ratson wrote:
> > > I think this should be u8 ? neigh-vlan-suppress looks buggy too  
> > 
> > I pointed this out during internal review, but assumed I am missing something
> > since almost all the attributes use flag when they are in fact u8. We can fix  
> 
> This is in fact the reason why I also changed it myself to use flag before sending.
> 
> > neigh-forward-grat to use u8 in v2 and change the rest in net. To be clear, I
> > believe the following should be converted from flag to u8:
> > 
> > mode, guard, protect, fast-leave, learning, unicast-flood, proxyarp, learning-
> > sync, proxyarp-wifi, mcast-flood, mcast-to-ucast, vlan-tunnel, bcast-flood,
> > neigh-suppress, isolated, mrp-ring-open, mrp-in-open, locked, mab, neigh-
> > vlan-suppress
> 
> So should we proceed as Ido suggested?

I think so. Tweak this patch and send a separate fix to net for the
existing attrs.

> > > flag is a type without a payload, the presence of the attr is the
> > > entire information
> > >
> > > None of the AIs seem to catch this, I think you may have over-split
> > > this submission a little bit. This patch may have been better off
> > > squashed into patch 4 ?  
> 
> It seems like the patch has enough content, but I can squash. I guess
> ill split the commit between patches 4 and 5 accordingly.

Could be my lack of familiarity with bridge but FWIW reading the
submission it felt like it was split by "area of code" rather than
logical steps. Splitting work into patches is a bit of an art..

> > Related: The AI also did not catch that the spec was missing (easy
> > to forget for rtnetlink). Do you think it's worth adding to
> > review-prompts?  

I assumed Sashiko missed this because it doesn't spent too much time on
the series as a whole right now (I was going to tweak that but looks
like Claude is having another meltdown, crazy inference latency,
patches backlogging for review..)

Are you saying that the review prompts-based test is also not catching
this even if you give it the range of commits that form the series?

Re: [PATCH net-next 1/6] bridge: uapi: Add neigh_forward_grat netlink attributes

[Ido Schimmel]

On Wed, May 06, 2026 at 03:05:56PM -0700, Jakub Kicinski wrote:
> > > Related: The AI also did not catch that the spec was missing (easy
> > > to forget for rtnetlink). Do you think it's worth adding to
> > > review-prompts?  
> 
> I assumed Sashiko missed this because it doesn't spent too much time on
> the series as a whole right now (I was going to tweak that but looks
> like Claude is having another meltdown, crazy inference latency,
> patches backlogging for review..)
> 
> Are you saying that the review prompts-based test is also not catching
> this even if you give it the range of commits that form the series?

Yes. I noticed this during the STP mode review:

https://lore.kernel.org/netdev/20260324200051.GA572287@shredder/

Where the entire code changes are in a single patch.

AI review of this patch:

https://sashiko.dev/#/patchset/20260324184942.2828691-1-aroulin%40nvidia.com
https://netdev-ai.bots.linux.dev/ai-review.html?id=d0cd7a7e-a803-45df-8121-555484a45315#patch-0

We can try to add this to the review-prompts and see if it helps.