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@intel.com/

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(-)

-- 
2.34.1

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@intel.com/
> 
> 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@intel.com>

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@intel.com/
> > 
> > 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@intel.com>

Not a single comment? Really? Three Rb and three Sb tags from Intel
staff and nobody found even a tiny problem? Sigh...

Let's start with an easiest one. What about introducing an unused
platform flag for ARP-offload?

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?

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.

Joseph

On 8/30/2025 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@intel.com/ 
>>>
>>>
>>> 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@intel.com>
> Not a single comment? Really? Three Rb and three Sb tags from Intel
> staff and nobody found even a tiny problem? Sigh...
Hi Joseph,
Thank you for your time and valuable review
>
> Let's start with an easiest one. What about introducing an unused
> platform flag for ARP-offload?
Right, this patch should not be here. Will be removed in next revision.
> 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?

While trying to enable ARP offload,
we found out that when interface was set down and up,
MAC_ARP_Address and ARP offload enable bit were reset to default values,
the address was set to 0xFFFFFFFF and ARP offload was disabled.
There was two possible solutions out of this:
a) caching address and ARP offload bit state
b) enabling ARP while interface is down.
We choose to go with second solution.
But given that fact this code depends on unused STMMAC_ARP_OFFLOAD_EN flag,
i guess whether it is fine or not, should not be placed in patchset.

> 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?
Yes, we have a validation team that verified proposed solution.
> 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.
>
> Joseph

BR,
Sebastian

On Tue, Sep 09, 2025 at 08:26:29PM +0200, Sebastian Basierski wrote:
> On 8/30/2025 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@intel.com/
> > > > 
> > > > 
> > > > 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@intel.com>
> > Not a single comment? Really? Three Rb and three Sb tags from Intel
> > staff and nobody found even a tiny problem? Sigh...
> Hi Joseph,
> Thank you for your time and valuable review
> > 
> > Let's start with an easiest one. What about introducing an unused
> > platform flag for ARP-offload?
> Right, this patch should not be here. Will be removed in next revision.
> > 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?
> 

> While trying to enable ARP offload,
> we found out that when interface was set down and up,
> MAC_ARP_Address and ARP offload enable bit were reset to default values,
> the address was set to 0xFFFFFFFF and ARP offload was disabled.
> There was two possible solutions out of this:
> a) caching address and ARP offload bit state
> b) enabling ARP while interface is down.
> We choose to go with second solution.
> But given that fact this code depends on unused STMMAC_ARP_OFFLOAD_EN flag,
> i guess whether it is fine or not, should not be placed in patchset.

The reasoning doesn't explain the outcome you provided. Even if the
controller is reset on the device switching on/off/on cycles the
driver will re-initialize it anyway. The same could have happened with
the ARP-engine too should the patch 1 be in in-place. But besides of
that you submitted patch 2, which _enables_ the network interface to
respond on the ARP-requests with some initial IP-address even if the
interface is set down by the user. This makes the host being visible
to the surrounding network devices behind the user back if the PHY,
for instance, is unmanaged or reported as fixed. Is that what Intel
wanted?

> 
> > 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?

> Yes, we have a validation team that verified proposed solution.

So did they notice anything strange? Is the validation team qualified
enough to correctly evaluate the change?

Joseph

On 14-Sep-25 14:53, Joseph Steel wrote:

> On Tue, Sep 09, 2025 at 08:26:29PM +0200, Sebastian Basierski wrote:
>> On 8/30/2025 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@intel.com/
>>>>>
>>>>>
>>>>> 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@intel.com>
>>> Not a single comment? Really? Three Rb and three Sb tags from Intel
>>> staff and nobody found even a tiny problem? Sigh...
>> Hi Joseph,
>> Thank you for your time and valuable review
>>> Let's start with an easiest one. What about introducing an unused
>>> platform flag for ARP-offload?
>> Right, this patch should not be here. Will be removed in next revision.
>>> 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?
>> While trying to enable ARP offload,
>> we found out that when interface was set down and up,
>> MAC_ARP_Address and ARP offload enable bit were reset to default values,
>> the address was set to 0xFFFFFFFF and ARP offload was disabled.
>> There was two possible solutions out of this:
>> a) caching address and ARP offload bit state
>> b) enabling ARP while interface is down.
>> We choose to go with second solution.
>> But given that fact this code depends on unused STMMAC_ARP_OFFLOAD_EN flag,
>> i guess whether it is fine or not, should not be placed in patchset.
> The reasoning doesn't explain the outcome you provided. Even if the
> controller is reset on the device switching on/off/on cycles the
> driver will re-initialize it anyway. The same could have happened with
> the ARP-engine too should the patch 1 be in in-place. But besides of
> that you submitted patch 2, which _enables_ the network interface to
> respond on the ARP-requests with some initial IP-address even if the
> interface is set down by the user. This makes the host being visible
> to the surrounding network devices behind the user back if the PHY,
> for instance, is unmanaged or reported as fixed. Is that what Intel
> wanted?
>
>>> 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?
>> Yes, we have a validation team that verified proposed solution.
> So did they notice anything strange? Is the validation team qualified
> enough to correctly evaluate the change?
>
> Joseph

Hi Joseph,

Thanks for your input. We'll remove both ARP offload-related patches (1 
and 2) from this patchset until an actual platform that uses them will 
be introduced later on. When that time comes we'll also check your 
comments regarding the "host being visible to the surrounding network 
devices behind the user" and adjust the patches accordingly.

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@intel.com/
>>>
>>> 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@intel.com>
> 
> 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

On 8/29/2025 7:46 PM, 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@intel.com/
>>>
>>> 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@intel.com>
> 
> Not a single comment? Really? Three Rb and three Sb tags from Intel
> staff and nobody found even a tiny problem? Sigh...
> 

Not everyone will find every issue. I'm certainly no expert in this
driver. This is why it is good to have many reviewers.
> Let's start with an easiest one. What about introducing an unused
> platform flag for ARP-offload?
> 
> 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?
> 
> So to speak at this stage I'd give NAK at least for the patches 1 and
> 2.

> >> The series looks good to me.
> >>
> >> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> > 
> > Not a single comment? Really? Three Rb and three Sb tags from Intel
> > staff and nobody found even a tiny problem? Sigh...
> > 
> 
> Not everyone will find every issue. I'm certainly no expert in this
> driver. This is why it is good to have many reviewers.

As a rule of thumb, Maintainers ignore multiple Reviewed-by when then
call come from the same company. At least if they don't actually point
out issues.

There is a nice quote from a bootlin/free-electrons developer. It is
something like: In order to get my patches merged faster, i review
other peoples patches, so freeing up Maintainer time to look at my
patches.

Jacob is a good example of that, he looks at patches from many
developers. Maybe more Intel people can help out reviewing patches,
particularly other stmmac patches, in order to get their own merged?

	Andrew