Re: [RFC net-next v2 2/6] ethtool: Add loopback GET/SET netlink implementation

[Björn Töpel]

Björn Töpel <bjorn@xxxxxxxxxx> writes:

> Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx> writes:
>
>> Hi Björn,
>>
>> On 08/03/2026 13:40, Björn Töpel wrote:
>>> Add the kernel-side ETHTOOL_MSG_LOOPBACK_GET,
>>> ETHTOOL_MSG_LOOPBACK_SET, and ETHTOOL_MSG_LOOPBACK_NTF handlers using
>>> the standard ethnl_request_ops infrastructure.
>>> 
>>> GET collects loopback entries from per-component helpers via
>>> loopback_get_entries(). SET parses the nested entry attributes,
>>> dispatches each to loopback_set_one(), and only sends a notification
>>> when the state is changed.
>>> 
>>> No components are wired yet.
>>> 
>>> Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxx>
>>
>> At a first glance, what I see is that you're using the GET ->doit method
>> to retrieve an array of loopback entries. The pefered approach in that
>> case is to use the GET ->dumpit command instead, issueing a netlink DUMP
>> request to list all available loopback entities on a given netdev.
>>
>> If you want some reference on that, take a look at the phy.c + the
>> 'perphy' helpers in net/ethtool/netlink.c
>>
>> The idea is that you can pass a netdev ifindex in the header of the DUMP
>> request, which you can use to dump all loopbacks the passed netdev.
>>
>> You can also check the ethtool code itself, you'll see that when you use
>> the "ethtool --show-phys eth0" command for example, it issues a DUMP
>> request to the kernel.
>
> Ah, got it! Thanks!

Thanks for pointing this out! I like how using this can get rid of the
fixed size array in struct ethtool_loopback_cfg, which had a bad smell
to me.

Thinking out loud -- Now, using a similar scheme for loopback requires
some thought around the sub-iterator: Loopback has "multiple"
sub-iterator: (component, [id], name)... or needs scheme to deal with
one sub-iter, and loopback can share a lot of code with the
perphy-functions.


Björn