Re: [PATCH net-next 0/4] net: stmmac: new features

[Cezary Rojewski]

On 2025-08-30 4:46 AM, Joseph Steel wrote:
> On Fri, Aug 29, 2025 at 02:23:24PM -0700, Jacob Keller wrote:
> > On 8/28/2025 7:45 AM, Konrad Leszczynski wrote:
> > > This series adds four new patches which introduce features such as ARP
> > > Offload support, VLAN protocol detection and TC flower filter support.
> > >
> > > Patchset has been created as a result of discussion at [1].
> > >
> > > [1] https://lore.kernel.org/netdev/20250826113247.3481273-1-konrad.leszczynski@xxxxxxxxx/
> > >
> > > v1 -> v2:
> > > - add missing SoB lines
> > > - place ifa_list under RCU protection
> > >
> > > Karol Jurczenia (3):
> > >    net: stmmac: enable ARP Offload on mac_link_up()
> > >    net: stmmac: set TE/RE bits for ARP Offload when interface down
> > >    net: stmmac: add TC flower filter support for IP EtherType
> > >
> > > Piotr Warpechowski (1):
> > >    net: stmmac: enhance VLAN protocol detection for GRO
> > >
> > >   drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
> > >   .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++++++++---
> > >   .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 19 +++++++++-
> > >   include/linux/stmmac.h                        |  1 +
> > >   4 files changed, 50 insertions(+), 6 deletions(-)
> >
> > The series looks good to me.
> >
> > Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
>
> Not a single comment? Really? Three Rb and three Sb tags from Intel
> staff and nobody found even a tiny problem? Sigh...

Hi Joseph,

Given how things look on the list (just v1, no comments), it's 
understandable to think that folks just appended their Reviewed-by and 
Signed-off-by tags without actually paying attention.

While I'm not part of the stmmac team directly - I do maintain the 
sound/soc/intel drivers - I did participate in shaping the patchset - 
titles, messages, division and such. What you see here has been 
rewritten a number of times before being sent.
 > Let's start with an easiest one. What about introducing an unused
> platform flag for ARP-offload?

That's a good point. No change in this patchset shall be tied to a 
specific platform that does not exist in the upstream kernel and thus 
simply has no users. The team will revisit for v2 and drop the patch if 
true.

> Next is more serious one. What about considering a case that
> IP-address can be changed or removed while MAC link is being up?
>
> Why does Intel want to have ARP requests being silently handled even
> when a link is completely set down by the host, when PHY-link is
> stopped and PHY is disconnected, after net_device::ndo_stop() is
> called?
>
> Finally did anyone test out the functionality of the patches 1 and
> 2? What does arping show for instance for just three ARP requests?
> Nothing strange?

I'll let Sebastian or Konrad comment on that as they are experienced 
with the IP.
> So to speak at this stage I'd give NAK at least for the patches 1 and
> 2.
>
> BTW I've been working with the driver for quite some time and AFAICS
> Intel contributed if not half but at least quarter of it' mess.

Not sure whether this bit helps anyone. The new faces are here to help, 
not to repeat the mistakes of the past.

Kind regards,
Czarek