On 06/03/2024 20:40, Andrew Lunn wrote:

On Wed, Mar 06, 2024 at 04:42:03PM +0100, Antonio Quartulli wrote:

quoted

On 05/03/2024 00:04, Andrew Lunn wrote:

quoted

On Mon, Mar 04, 2024 at 04:09:13PM +0100, Antonio Quartulli wrote:

quoted

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.

quoted

quoted

quoted

+	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.