| Message ID | 1454468677-12280-2-git-send-email-razor@blackwall.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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
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.
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