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

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2304 bytes --]



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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]