| Message ID | 20240304150914.11444-23-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
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? > +static void ovpn_get_drvinfo(struct net_device *dev, > + struct ethtool_drvinfo *info) > +{ > + strscpy(info->driver, DRV_NAME, sizeof(info->driver)); > + strscpy(info->version, DRV_VERSION, sizeof(info->version)); Please leave version untouched. The ethtool core will then fill it in with something useful. > + strscpy(info->bus_info, "ovpn", sizeof(info->bus_info)); This is also not accurate. There is no bus involved. Andrew
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? > >> +static void ovpn_get_drvinfo(struct net_device *dev, >> + struct ethtool_drvinfo *info) >> +{ >> + strscpy(info->driver, DRV_NAME, sizeof(info->driver)); >> + strscpy(info->version, DRV_VERSION, sizeof(info->version)); > > Please leave version untouched. The ethtool core will then fill it in > with something useful. will do! > >> + 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. Regards,
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? > > > + 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. Andrew
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 >
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; + + return 0; +} + +static void ovpn_get_drvinfo(struct net_device *dev, + struct ethtool_drvinfo *info) +{ + strscpy(info->driver, DRV_NAME, sizeof(info->driver)); + strscpy(info->version, DRV_VERSION, sizeof(info->version)); + strscpy(info->bus_info, "ovpn", sizeof(info->bus_info)); +} + +static const struct ethtool_ops ovpn_ethtool_ops = { + .get_link_ksettings = ovpn_get_link_ksettings, + .get_drvinfo = ovpn_get_drvinfo, + .get_link = ethtool_op_get_link, + .get_ts_info = ethtool_op_get_ts_info, +}; + static void ovpn_setup(struct net_device *dev) { /* compute the overhead considering AEAD encryption */ @@ -95,6 +126,7 @@ static void ovpn_setup(struct net_device *dev) dev->needs_free_netdev = true; + dev->ethtool_ops = &ovpn_ethtool_ops; dev->netdev_ops = &ovpn_netdev_ops; dev->priv_destructor = ovpn_struct_free;
Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/main.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)