* [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support @ 2016-02-03 3:04 Nikolay Aleksandrov 2016-02-03 3:04 ` [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions Nikolay Aleksandrov ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-03 3:04 UTC (permalink / raw) To: netdev; +Cc: mst, roopa, davem, Nikolay Aleksandrov From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Hi, Patch 1 adds ethtool speed/duplex validation functions which check if the value is defined. Patch 2 adds support for ethtool (get|set)_settings and uses the validation functions to check the user-supplied values. v2: split in 2 patches to allow everyone to make use of the validation functions and allow virtio_net devices to be half duplex v3: added a check to return error if the user tries to change anything else besides duplex/speed as per Michael's comment v4: Set port type to PORT_OTHER v5: clear diff1.port (ignore port) when checking for changes since we set it now and ethtool uses it in the set request Sorry about the pointless iterations, should've all covered now. Cheers, Nik Nikolay Aleksandrov (2): ethtool: add speed/duplex validation functions virtio_net: add ethtool support for set and get of settings drivers/net/virtio_net.c | 60 ++++++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/ethtool.h | 34 +++++++++++++++++++++++++ 2 files changed, 94 insertions(+) -- 2.4.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-03 3:04 [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support Nikolay Aleksandrov @ 2016-02-03 3:04 ` Nikolay Aleksandrov 2016-02-03 23:32 ` Stephen Hemminger 2016-02-03 3:04 ` [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings Nikolay Aleksandrov 2016-02-07 19:31 ` [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support David Miller 2 siblings, 1 reply; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-03 3:04 UTC (permalink / raw) To: netdev; +Cc: mst, roopa, davem, Nikolay Aleksandrov From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Add functions which check if the speed/duplex are defined. Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> --- v2: new patch v3: added Michael's ack v4, v5: no change include/uapi/linux/ethtool.h | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 57fa39005e79..b2e180181629 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -1319,11 +1319,45 @@ enum ethtool_sfeatures_retval_bits { #define SPEED_UNKNOWN -1 +static inline int ethtool_validate_speed(__u32 speed) +{ + switch (speed) { + case SPEED_10: + case SPEED_100: + case SPEED_1000: + case SPEED_2500: + case SPEED_5000: + case SPEED_10000: + case SPEED_20000: + case SPEED_25000: + case SPEED_40000: + case SPEED_50000: + case SPEED_56000: + case SPEED_100000: + case SPEED_UNKNOWN: + return 1; + } + + return 0; +} + /* Duplex, half or full. */ #define DUPLEX_HALF 0x00 #define DUPLEX_FULL 0x01 #define DUPLEX_UNKNOWN 0xff +static inline int ethtool_validate_duplex(__u8 duplex) +{ + switch (duplex) { + case DUPLEX_HALF: + case DUPLEX_FULL: + case DUPLEX_UNKNOWN: + return 1; + } + + return 0; +} + /* Which connector port. */ #define PORT_TP 0x00 #define PORT_AUI 0x01 -- 2.4.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-03 3:04 ` [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions Nikolay Aleksandrov @ 2016-02-03 23:32 ` Stephen Hemminger 2016-02-03 23:49 ` Rick Jones ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Stephen Hemminger @ 2016-02-03 23:32 UTC (permalink / raw) To: Nikolay Aleksandrov; +Cc: netdev, mst, roopa, davem, Nikolay Aleksandrov On Wed, 3 Feb 2016 04:04:36 +0100 Nikolay Aleksandrov <razor@blackwall.org> wrote: > > +static inline int ethtool_validate_speed(__u32 speed) > +{ No need for inline. But why check for valid value at all. At some point in the future, there will be yet another speed adopted by some standard body and the switch statement would need another value. Why not accept any value? This is a virtual device. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-03 23:32 ` Stephen Hemminger @ 2016-02-03 23:49 ` Rick Jones 2016-02-04 12:47 ` Michael S. Tsirkin 2016-02-04 11:02 ` Nikolay Aleksandrov 2016-02-04 12:18 ` Michael S. Tsirkin 2 siblings, 1 reply; 15+ messages in thread From: Rick Jones @ 2016-02-03 23:49 UTC (permalink / raw) To: Stephen Hemminger, Nikolay Aleksandrov Cc: netdev, mst, roopa, davem, Nikolay Aleksandrov On 02/03/2016 03:32 PM, Stephen Hemminger wrote: > But why check for valid value at all. At some point in the > future, there will be yet another speed adopted by some standard body > and the switch statement would need another value. > > Why not accept any value? This is a virtual device. > And even for not-quite-virtual devices - such as a VC/FlexNIC in an HPE blade server there can be just about any speed set. I think we went down a path of patching some things to address that many years ago. It would be a shame to undo that. rick ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-03 23:49 ` Rick Jones @ 2016-02-04 12:47 ` Michael S. Tsirkin 2016-02-04 17:30 ` Rick Jones 0 siblings, 1 reply; 15+ messages in thread From: Michael S. Tsirkin @ 2016-02-04 12:47 UTC (permalink / raw) To: Rick Jones Cc: Stephen Hemminger, Nikolay Aleksandrov, netdev, roopa, davem, Nikolay Aleksandrov, linux-api On Wed, Feb 03, 2016 at 03:49:04PM -0800, Rick Jones wrote: > On 02/03/2016 03:32 PM, Stephen Hemminger wrote: > > >But why check for valid value at all. At some point in the > >future, there will be yet another speed adopted by some standard body > >and the switch statement would need another value. > > > >Why not accept any value? This is a virtual device. > > > > And even for not-quite-virtual devices - such as a VC/FlexNIC in an HPE > blade server there can be just about any speed set. I think we went down a > path of patching some things to address that many years ago. It would be a > shame to undo that. > > rick I'm not sure I understand. The question is in defining the UAPI. We currently have: * @speed: Low bits of the speed * @speed_hi: Hi bits of the speed with the assumption that all values come from the defines. So if we allow any value here we need to define what it means. If the following is acceptable, then we can drop most of validation: ---> ethtool: future-proof interface for speed extensions Many virtual and not quite virtual devices allow any speed to be set through ethtool. Document this fact to make sure people don't assume the enum lists all possible values. Reserve values greater than INT_MAX for future extension and to avoid conflict with SPEED_UNKNOWN. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> ---- diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 57fa390..9462844 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -31,7 +31,7 @@ * physical connectors and other link features that are * advertised through autonegotiation or enabled for * auto-detection. - * @speed: Low bits of the speed + * @speed: Low bits of the speed, 1Mb units, 0 to INT_MAX or SPEED_UNKNOWN * @duplex: Duplex mode; one of %DUPLEX_* * @port: Physical connector type; one of %PORT_* * @phy_address: MDIO address of PHY (transceiver); 0 or 255 if not @@ -47,7 +47,7 @@ * obsoleted by &struct ethtool_coalesce. Read-only; deprecated. * @maxrxpkt: Historically used to report RX IRQ coalescing; now * obsoleted by &struct ethtool_coalesce. Read-only; deprecated. - * @speed_hi: High bits of the speed + * @speed_hi: High bits of the speed, 1Mb units, 0 to INT_MAX or SPEED_UNKNOWN * @eth_tp_mdix: Ethernet twisted-pair MDI(-X) status; one of * %ETH_TP_MDI_*. If the status is unknown or not applicable, the * value will be %ETH_TP_MDI_INVALID. Read-only. @@ -1303,7 +1303,7 @@ enum ethtool_sfeatures_retval_bits { * it was forced up into this mode or autonegotiated. */ -/* The forced speed, 10Mb, 100Mb, gigabit, [2.5|5|10|20|25|40|50|56|100]GbE. */ +/* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. */ #define SPEED_10 10 #define SPEED_100 100 #define SPEED_1000 1000 -- MST ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-04 12:47 ` Michael S. Tsirkin @ 2016-02-04 17:30 ` Rick Jones 0 siblings, 0 replies; 15+ messages in thread From: Rick Jones @ 2016-02-04 17:30 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Stephen Hemminger, Nikolay Aleksandrov, netdev, roopa, davem, Nikolay Aleksandrov, linux-api On 02/04/2016 04:47 AM, Michael S. Tsirkin wrote: > On Wed, Feb 03, 2016 at 03:49:04PM -0800, Rick Jones wrote: >> And even for not-quite-virtual devices - such as a VC/FlexNIC in an HPE >> blade server there can be just about any speed set. I think we went down a >> path of patching some things to address that many years ago. It would be a >> shame to undo that. >> >> rick > > I'm not sure I understand. The question is in defining the UAPI. > We currently have: > > * @speed: Low bits of the speed > * @speed_hi: Hi bits of the speed > > with the assumption that all values come from the defines. > > So if we allow any value here we need to define what it means. I may be mixing apples and kiwis. Many years ago when HP came-out with their blades and VirtualConnect, they included the ability to create "flex NICs" - "sub-NICs" out of a given interface port on a blade, and to assign each a specific bitrate in increments (IIRC) of 100 Mbit/s. This was reported up through the driver and it became necessary to make ethtool (again, IIRC) not so picky about "valid" speed values. rick ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-03 23:32 ` Stephen Hemminger 2016-02-03 23:49 ` Rick Jones @ 2016-02-04 11:02 ` Nikolay Aleksandrov 2016-02-04 12:18 ` Michael S. Tsirkin 2 siblings, 0 replies; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-04 11:02 UTC (permalink / raw) To: Stephen Hemminger, Nikolay Aleksandrov; +Cc: netdev, mst, roopa, davem On 02/04/2016 12:32 AM, Stephen Hemminger wrote: > On Wed, 3 Feb 2016 04:04:36 +0100 > Nikolay Aleksandrov <razor@blackwall.org> wrote: > >> >> +static inline int ethtool_validate_speed(__u32 speed) >> +{ > > > No need for inline. > This is defined in a header, if it's not inline you start getting "defined but not used" warnings. > But why check for valid value at all. At some point in the > future, there will be yet another speed adopted by some standard body > and the switch statement would need another value. > > Why not accept any value? This is a virtual device. > It was moved near the defined values so everyone adding a new speed would remember to update the validation function as well. That being said, I don't object to being able to set any custom speed to the virtio_net device especially when there're physical devices that can have speeds outside of these defines. Michael do you have any objections if I respin without the speed validation ? Thanks, Nik ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions 2016-02-03 23:32 ` Stephen Hemminger 2016-02-03 23:49 ` Rick Jones 2016-02-04 11:02 ` Nikolay Aleksandrov @ 2016-02-04 12:18 ` Michael S. Tsirkin 2 siblings, 0 replies; 15+ messages in thread From: Michael S. Tsirkin @ 2016-02-04 12:18 UTC (permalink / raw) To: Stephen Hemminger Cc: Nikolay Aleksandrov, netdev, roopa, davem, Nikolay Aleksandrov On Thu, Feb 04, 2016 at 10:32:26AM +1100, Stephen Hemminger wrote: > On Wed, 3 Feb 2016 04:04:36 +0100 > Nikolay Aleksandrov <razor@blackwall.org> wrote: > > > > > +static inline int ethtool_validate_speed(__u32 speed) > > +{ > > > No need for inline. > > But why check for valid value at all. At some point in the > future, there will be yet another speed adopted by some standard body > and the switch statement would need another value. > > Why not accept any value? This is a virtual device. It's virtual but often there's a physical backend behind it. In the future we will likely forward the values to and from that physical device. And if guest passes an unexpected value, host is unlikely to be able to support it. -- MST ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings 2016-02-03 3:04 [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support Nikolay Aleksandrov 2016-02-03 3:04 ` [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions Nikolay Aleksandrov @ 2016-02-03 3:04 ` Nikolay Aleksandrov 2016-02-03 9:19 ` Nikolay Aleksandrov 2016-02-04 12:21 ` Michael S. Tsirkin 2016-02-07 19:31 ` [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support David Miller 2 siblings, 2 replies; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-03 3:04 UTC (permalink / raw) To: netdev; +Cc: mst, roopa, davem, Nikolay Aleksandrov From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> This patch allows the user to set and retrieve speed and duplex of the virtio_net device via ethtool. Having this functionality is very helpful for simulating different environments and also enables the virtio_net device to participate in operations where proper speed and duplex are required (e.g. currently bonding lacp mode requires full duplex). Custom speed and duplex are not allowed, the user-supplied settings are validated before applying. Example: $ ethtool eth1 Settings for eth1: ... Speed: Unknown! Duplex: Unknown! (255) $ ethtool -s eth1 speed 1000 duplex full $ ethtool eth1 Settings for eth1: ... Speed: 1000Mb/s Duplex: Full Based on a patch by Roopa Prabhu. Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- v2: use the new ethtool speed/duplex validation functions and allow half duplex to be set v3: return error if the user tries to change anything besides speed/duplex as per Michael's comment We have to zero-out advertising as it gets set automatically by ethtool if setting speed and duplex together. v4: Set port type to PORT_OTHER v5: null diff1.port because we set cmd->port now and ethtool returns it in the set request, retested all cases drivers/net/virtio_net.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 767ab11a6e9f..c9fd52a8e6ec 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -146,6 +146,10 @@ struct virtnet_info { virtio_net_ctrl_ack ctrl_status; u8 ctrl_promisc; u8 ctrl_allmulti; + + /* Ethtool settings */ + u8 duplex; + u32 speed; }; struct padded_vnet_hdr { @@ -1376,6 +1380,58 @@ static void virtnet_get_channels(struct net_device *dev, channels->other_count = 0; } +/* Check if the user is trying to change anything besides speed/duplex */ +static bool virtnet_validate_ethtool_cmd(const struct ethtool_cmd *cmd) +{ + struct ethtool_cmd diff1 = *cmd; + struct ethtool_cmd diff2 = {}; + + /* advertising and cmd are usually set, ignore port because we set it */ + ethtool_cmd_speed_set(&diff1, 0); + diff1.advertising = 0; + diff1.duplex = 0; + diff1.port = 0; + diff1.cmd = 0; + + return !memcmp(&diff1, &diff2, sizeof(diff1)); +} + +static int virtnet_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) +{ + struct virtnet_info *vi = netdev_priv(dev); + u32 speed; + + speed = ethtool_cmd_speed(cmd); + /* don't allow custom speed and duplex */ + if (!ethtool_validate_speed(speed) || + !ethtool_validate_duplex(cmd->duplex) || + !virtnet_validate_ethtool_cmd(cmd)) + return -EINVAL; + vi->speed = speed; + vi->duplex = cmd->duplex; + + return 0; +} + +static int virtnet_get_settings(struct net_device *dev, struct ethtool_cmd *cmd) +{ + struct virtnet_info *vi = netdev_priv(dev); + + ethtool_cmd_speed_set(cmd, vi->speed); + cmd->duplex = vi->duplex; + cmd->port = PORT_OTHER; + + return 0; +} + +static void virtnet_init_settings(struct net_device *dev) +{ + struct virtnet_info *vi = netdev_priv(dev); + + vi->speed = SPEED_UNKNOWN; + vi->duplex = DUPLEX_UNKNOWN; +} + static const struct ethtool_ops virtnet_ethtool_ops = { .get_drvinfo = virtnet_get_drvinfo, .get_link = ethtool_op_get_link, @@ -1383,6 +1439,8 @@ static const struct ethtool_ops virtnet_ethtool_ops = { .set_channels = virtnet_set_channels, .get_channels = virtnet_get_channels, .get_ts_info = ethtool_op_get_ts_info, + .get_settings = virtnet_get_settings, + .set_settings = virtnet_set_settings, }; #define MIN_MTU 68 @@ -1855,6 +1913,8 @@ static int virtnet_probe(struct virtio_device *vdev) netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs); netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs); + virtnet_init_settings(dev); + err = register_netdev(dev); if (err) { pr_debug("virtio_net: registering device failed\n"); -- 2.4.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings 2016-02-03 3:04 ` [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings Nikolay Aleksandrov @ 2016-02-03 9:19 ` Nikolay Aleksandrov 2016-02-04 12:23 ` Michael S. Tsirkin 2016-02-04 12:21 ` Michael S. Tsirkin 1 sibling, 1 reply; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-03 9:19 UTC (permalink / raw) To: Nikolay Aleksandrov, netdev; +Cc: mst, roopa, davem On 02/03/2016 04:04 AM, Nikolay Aleksandrov wrote: > From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > This patch allows the user to set and retrieve speed and duplex of the > virtio_net device via ethtool. Having this functionality is very helpful > for simulating different environments and also enables the virtio_net > device to participate in operations where proper speed and duplex are > required (e.g. currently bonding lacp mode requires full duplex). Custom > speed and duplex are not allowed, the user-supplied settings are validated > before applying. > > Example: > $ ethtool eth1 > Settings for eth1: > ... > Speed: Unknown! > Duplex: Unknown! (255) > $ ethtool -s eth1 speed 1000 duplex full > $ ethtool eth1 > Settings for eth1: > ... > Speed: 1000Mb/s > Duplex: Full > > Based on a patch by Roopa Prabhu. > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > v2: use the new ethtool speed/duplex validation functions and allow half > duplex to be set > v3: return error if the user tries to change anything besides speed/duplex > as per Michael's comment > We have to zero-out advertising as it gets set automatically by ethtool if > setting speed and duplex together. > v4: Set port type to PORT_OTHER > v5: null diff1.port because we set cmd->port now and ethtool returns it in > the set request, retested all cases > Hmm, nulling the advertising and ->port completely ignores them, i.e. won't produce an error if the user actually specified a different value for either of them. We can check if the ->port matches what we returned, but there's no fix for advertising. I'm leaving both ignored for now, please let me know if you'd prefer otherwise. Thanks, Nik ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings 2016-02-03 9:19 ` Nikolay Aleksandrov @ 2016-02-04 12:23 ` Michael S. Tsirkin 0 siblings, 0 replies; 15+ messages in thread From: Michael S. Tsirkin @ 2016-02-04 12:23 UTC (permalink / raw) To: Nikolay Aleksandrov; +Cc: Nikolay Aleksandrov, netdev, roopa, davem On Wed, Feb 03, 2016 at 10:19:04AM +0100, Nikolay Aleksandrov wrote: > On 02/03/2016 04:04 AM, Nikolay Aleksandrov wrote: > > From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > > > This patch allows the user to set and retrieve speed and duplex of the > > virtio_net device via ethtool. Having this functionality is very helpful > > for simulating different environments and also enables the virtio_net > > device to participate in operations where proper speed and duplex are > > required (e.g. currently bonding lacp mode requires full duplex). Custom > > speed and duplex are not allowed, the user-supplied settings are validated > > before applying. > > > > Example: > > $ ethtool eth1 > > Settings for eth1: > > ... > > Speed: Unknown! > > Duplex: Unknown! (255) > > $ ethtool -s eth1 speed 1000 duplex full > > $ ethtool eth1 > > Settings for eth1: > > ... > > Speed: 1000Mb/s > > Duplex: Full > > > > Based on a patch by Roopa Prabhu. > > > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > --- > > v2: use the new ethtool speed/duplex validation functions and allow half > > duplex to be set > > v3: return error if the user tries to change anything besides speed/duplex > > as per Michael's comment > > We have to zero-out advertising as it gets set automatically by ethtool if > > setting speed and duplex together. > > v4: Set port type to PORT_OTHER > > v5: null diff1.port because we set cmd->port now and ethtool returns it in > > the set request, retested all cases > > > > Hmm, nulling the advertising and ->port completely ignores them, i.e. won't produce > an error if the user actually specified a different value for either of them. > We can check if the ->port matches what we returned, but there's no fix for > advertising. I'm leaving both ignored for now, please let me know if you'd > prefer otherwise. > > Thanks, > Nik I think I prefer validating port. For advertising we don't allow enabling autonegotiation so ignoring these is fine I think. -- MST ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings 2016-02-03 3:04 ` [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings Nikolay Aleksandrov 2016-02-03 9:19 ` Nikolay Aleksandrov @ 2016-02-04 12:21 ` Michael S. Tsirkin 2016-02-04 12:26 ` Nikolay Aleksandrov 1 sibling, 1 reply; 15+ messages in thread From: Michael S. Tsirkin @ 2016-02-04 12:21 UTC (permalink / raw) To: Nikolay Aleksandrov; +Cc: netdev, roopa, davem, Nikolay Aleksandrov On Wed, Feb 03, 2016 at 04:04:37AM +0100, Nikolay Aleksandrov wrote: > From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > This patch allows the user to set and retrieve speed and duplex of the > virtio_net device via ethtool. Having this functionality is very helpful > for simulating different environments and also enables the virtio_net > device to participate in operations where proper speed and duplex are > required (e.g. currently bonding lacp mode requires full duplex). Custom > speed and duplex are not allowed, the user-supplied settings are validated > before applying. > > Example: > $ ethtool eth1 > Settings for eth1: > ... > Speed: Unknown! > Duplex: Unknown! (255) > $ ethtool -s eth1 speed 1000 duplex full > $ ethtool eth1 > Settings for eth1: > ... > Speed: 1000Mb/s > Duplex: Full > > Based on a patch by Roopa Prabhu. > > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > --- > v2: use the new ethtool speed/duplex validation functions and allow half > duplex to be set > v3: return error if the user tries to change anything besides speed/duplex > as per Michael's comment > We have to zero-out advertising as it gets set automatically by ethtool if > setting speed and duplex together. > v4: Set port type to PORT_OTHER > v5: null diff1.port because we set cmd->port now and ethtool returns it in > the set request, retested all cases > > drivers/net/virtio_net.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 767ab11a6e9f..c9fd52a8e6ec 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -146,6 +146,10 @@ struct virtnet_info { > virtio_net_ctrl_ack ctrl_status; > u8 ctrl_promisc; > u8 ctrl_allmulti; > + > + /* Ethtool settings */ > + u8 duplex; > + u32 speed; > }; > > struct padded_vnet_hdr { > @@ -1376,6 +1380,58 @@ static void virtnet_get_channels(struct net_device *dev, > channels->other_count = 0; > } > > +/* Check if the user is trying to change anything besides speed/duplex */ > +static bool virtnet_validate_ethtool_cmd(const struct ethtool_cmd *cmd) > +{ > + struct ethtool_cmd diff1 = *cmd; > + struct ethtool_cmd diff2 = {}; > + > + /* advertising and cmd are usually set, ignore port because we set it */ We set it where? Instead of this, should not we set diff2.port to PORT_OTHER? > + ethtool_cmd_speed_set(&diff1, 0); > + diff1.advertising = 0; > + diff1.duplex = 0; > + diff1.port = 0; > + diff1.cmd = 0; > + > + return !memcmp(&diff1, &diff2, sizeof(diff1)); > +} > + > +static int virtnet_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) > +{ > + struct virtnet_info *vi = netdev_priv(dev); > + u32 speed; > + > + speed = ethtool_cmd_speed(cmd); > + /* don't allow custom speed and duplex */ > + if (!ethtool_validate_speed(speed) || > + !ethtool_validate_duplex(cmd->duplex) || > + !virtnet_validate_ethtool_cmd(cmd)) > + return -EINVAL; > + vi->speed = speed; > + vi->duplex = cmd->duplex; > + > + return 0; > +} > + > +static int virtnet_get_settings(struct net_device *dev, struct ethtool_cmd *cmd) > +{ > + struct virtnet_info *vi = netdev_priv(dev); > + > + ethtool_cmd_speed_set(cmd, vi->speed); > + cmd->duplex = vi->duplex; > + cmd->port = PORT_OTHER; > + > + return 0; > +} > + > +static void virtnet_init_settings(struct net_device *dev) > +{ > + struct virtnet_info *vi = netdev_priv(dev); > + > + vi->speed = SPEED_UNKNOWN; > + vi->duplex = DUPLEX_UNKNOWN; > +} > + > static const struct ethtool_ops virtnet_ethtool_ops = { > .get_drvinfo = virtnet_get_drvinfo, > .get_link = ethtool_op_get_link, > @@ -1383,6 +1439,8 @@ static const struct ethtool_ops virtnet_ethtool_ops = { > .set_channels = virtnet_set_channels, > .get_channels = virtnet_get_channels, > .get_ts_info = ethtool_op_get_ts_info, > + .get_settings = virtnet_get_settings, > + .set_settings = virtnet_set_settings, > }; > > #define MIN_MTU 68 > @@ -1855,6 +1913,8 @@ static int virtnet_probe(struct virtio_device *vdev) > netif_set_real_num_tx_queues(dev, vi->curr_queue_pairs); > netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs); > > + virtnet_init_settings(dev); > + > err = register_netdev(dev); > if (err) { > pr_debug("virtio_net: registering device failed\n"); > -- > 2.4.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings 2016-02-04 12:21 ` Michael S. Tsirkin @ 2016-02-04 12:26 ` Nikolay Aleksandrov 0 siblings, 0 replies; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-04 12:26 UTC (permalink / raw) To: Michael S. Tsirkin, Nikolay Aleksandrov; +Cc: netdev, roopa, davem On 02/04/2016 01:21 PM, Michael S. Tsirkin wrote: > On Wed, Feb 03, 2016 at 04:04:37AM +0100, Nikolay Aleksandrov wrote: >> From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> [snip] >> struct padded_vnet_hdr { >> @@ -1376,6 +1380,58 @@ static void virtnet_get_channels(struct net_device *dev, >> channels->other_count = 0; >> } >> >> +/* Check if the user is trying to change anything besides speed/duplex */ >> +static bool virtnet_validate_ethtool_cmd(const struct ethtool_cmd *cmd) >> +{ >> + struct ethtool_cmd diff1 = *cmd; >> + struct ethtool_cmd diff2 = {}; >> + >> + /* advertising and cmd are usually set, ignore port because we set it */ > > We set it where? If you're asking about advertising - ethtool sets it automatically when the user tries to set both speed and duplex together. > Instead of this, should not we set diff2.port to PORT_OTHER? > Yes, that will validate it too. >> + ethtool_cmd_speed_set(&diff1, 0); >> + diff1.advertising = 0; >> + diff1.duplex = 0; >> + diff1.port = 0; >> + diff1.cmd = 0; >> + >> + return !memcmp(&diff1, &diff2, sizeof(diff1)); >> +} >> + [snip] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support 2016-02-03 3:04 [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support Nikolay Aleksandrov 2016-02-03 3:04 ` [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions Nikolay Aleksandrov 2016-02-03 3:04 ` [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings Nikolay Aleksandrov @ 2016-02-07 19:31 ` David Miller 2016-02-07 19:35 ` Nikolay Aleksandrov 2 siblings, 1 reply; 15+ messages in thread From: David Miller @ 2016-02-07 19:31 UTC (permalink / raw) To: razor; +Cc: netdev, mst, roopa, nikolay From: Nikolay Aleksandrov <razor@blackwall.org> Date: Wed, 3 Feb 2016 04:04:35 +0100 > Patch 1 adds ethtool speed/duplex validation functions which check if the > value is defined. Patch 2 adds support for ethtool (get|set)_settings and > uses the validation functions to check the user-supplied values. > > v2: split in 2 patches to allow everyone to make use of the validation > functions and allow virtio_net devices to be half duplex > v3: added a check to return error if the user tries to change anything else > besides duplex/speed as per Michael's comment > v4: Set port type to PORT_OTHER > v5: clear diff1.port (ignore port) when checking for changes since we set > it now and ethtool uses it in the set request > > Sorry about the pointless iterations, should've all covered now. Series applied, thanks Nik. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support 2016-02-07 19:31 ` [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support David Miller @ 2016-02-07 19:35 ` Nikolay Aleksandrov 0 siblings, 0 replies; 15+ messages in thread From: Nikolay Aleksandrov @ 2016-02-07 19:35 UTC (permalink / raw) To: David Miller, razor; +Cc: netdev, mst, roopa On 02/07/2016 08:31 PM, David Miller wrote: > From: Nikolay Aleksandrov <razor@blackwall.org> > Date: Wed, 3 Feb 2016 04:04:35 +0100 > >> Patch 1 adds ethtool speed/duplex validation functions which check if the >> value is defined. Patch 2 adds support for ethtool (get|set)_settings and >> uses the validation functions to check the user-supplied values. >> >> v2: split in 2 patches to allow everyone to make use of the validation >> functions and allow virtio_net devices to be half duplex >> v3: added a check to return error if the user tries to change anything else >> besides duplex/speed as per Michael's comment >> v4: Set port type to PORT_OTHER >> v5: clear diff1.port (ignore port) when checking for changes since we set >> it now and ethtool uses it in the set request >> >> Sorry about the pointless iterations, should've all covered now. > > Series applied, thanks Nik. > Thank you Dave, I was actually waiting for Michael's ethtool patch to go in. I'll send a follow-up to make sure the port is what we set it to and also to allow setting any speed (as people commented, devices can have any speed these days, so we don't want to limit them to the defined ones). ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-02-07 19:35 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-03 3:04 [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support Nikolay Aleksandrov 2016-02-03 3:04 ` [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions Nikolay Aleksandrov 2016-02-03 23:32 ` Stephen Hemminger 2016-02-03 23:49 ` Rick Jones 2016-02-04 12:47 ` Michael S. Tsirkin 2016-02-04 17:30 ` Rick Jones 2016-02-04 11:02 ` Nikolay Aleksandrov 2016-02-04 12:18 ` Michael S. Tsirkin 2016-02-03 3:04 ` [PATCH net-next v5 2/2] virtio_net: add ethtool support for set and get of settings Nikolay Aleksandrov 2016-02-03 9:19 ` Nikolay Aleksandrov 2016-02-04 12:23 ` Michael S. Tsirkin 2016-02-04 12:21 ` Michael S. Tsirkin 2016-02-04 12:26 ` Nikolay Aleksandrov 2016-02-07 19:31 ` [PATCH net-next v5 0/2] virtio_net: add ethtool get/set settings support David Miller 2016-02-07 19:35 ` Nikolay Aleksandrov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.