Re: [PATCH net-next v2 22/22] ovpn: add basic ethtool support

[Antonio Quartulli <antonio@openvpn.net>]

On 06/03/2024 20:40, Andrew Lunn wrote:
> On Wed, Mar 06, 2024 at 04:42:03PM +0100, Antonio Quartulli wrote:
>> On 05/03/2024 00:04, Andrew Lunn wrote:
>>> On Mon, Mar 04, 2024 at 04:09:13PM +0100, Antonio Quartulli wrote:
>>>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>>>> ---
>>>>    drivers/net/ovpn/main.c | 32 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
>>>> index 95a94ccc99c1..9dfcf2580659 100644
>>>> --- a/drivers/net/ovpn/main.c
>>>> +++ b/drivers/net/ovpn/main.c
>>>> @@ -13,6 +13,7 @@
>>>>    #include "ovpnstruct.h"
>>>>    #include "packet.h"
>>>> +#include <linux/ethtool.h>
>>>>    #include <linux/genetlink.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/moduleparam.h>
>>>> @@ -83,6 +84,36 @@ static const struct net_device_ops ovpn_netdev_ops = {
>>>>    	.ndo_get_stats64        = dev_get_tstats64,
>>>>    };
>>>> +static int ovpn_get_link_ksettings(struct net_device *dev,
>>>> +				   struct ethtool_link_ksettings *cmd)
>>>> +{
>>>> +	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported, 0);
>>>> +	ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising, 0);
>>>> +	cmd->base.speed	= SPEED_1000;
>>>> +	cmd->base.duplex = DUPLEX_FULL;
>>>> +	cmd->base.port = PORT_TP;
>>>> +	cmd->base.phy_address = 0;
>>>> +	cmd->base.transceiver = XCVR_INTERNAL;
>>>> +	cmd->base.autoneg = AUTONEG_DISABLE;
>>>
>>> Why? It is a virtual device. Speed and duplex is meaningless. You
>>> could run this over FDDI, HIPPI, or RFC 1149? So why PORT_TP?
>>
>> To be honest, I couldn't find any description to help me with deciding what
>> to set there and I just used a value I saw in other Ethernet drivers.
>>
>> Do you have any recommendation?
>> For the other fields: do you think they make sense? The speed value is
>> always debatable...The actual speed depends on the transport interface and
>> there might be multiple involved. Maybe SPEED_UNKNOWN is more appropriate?
> 
> Turn it around. What is your use case? What is using this information?
> I would just not implement this function. But maybe you know of
> something which actually requires it?

At the moment there is nothing on my side requiring this function.

I thought it was recommended to provide an implementation so that any 
potential user would be happy (some monitoring tool? some other module 
in the kernel?)

But if you think there is no meaningful use case for it, then I think it 
makes sense to just drop it, as we are filling it with random/virtual 
values.

> 
>>>> +	strscpy(info->bus_info, "ovpn", sizeof(info->bus_info));
>>>
>>> This is also not accurate. There is no bus involved.
>>
>> Should I just leave it empty then?
>>
>> My concern is that a user expects $something and it will crash on my empty
>> string. But if empty is allowed, I will just go with it.
> 
> Empty is allowed. The bridge uses "N/A", which i would say is also
> correct. tun/tap does however use "tun", so "ovpn" is not that
> different i supposes.

I just grepped through the kernel and I can see various patterns.
I also found:

  899         strscpy(info->bus_info, "batman", sizeof(info->bus_info));

:-)

All in all, I agree we can sick to "ovpn".

Regards,

> 
> 	Andrew
> 

-- 
Antonio Quartulli
OpenVPN Inc.