Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Nikolay Aleksandrov]

On 02/04/2016 12:32 AM, Stephen Hemminger wrote:
> On Wed,  3 Feb 2016 04:04:36 +0100
> Nikolay Aleksandrov  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

Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Michael S. Tsirkin]

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 



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  ethtool_coalesce.  Read-only; deprecated.
  * @maxrxpkt: Historically used to report RX IRQ coalescing; now
  * obsoleted by  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

Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Michael S. Tsirkin]

On Thu, Feb 04, 2016 at 10:32:26AM +1100, Stephen Hemminger wrote:
> On Wed,  3 Feb 2016 04:04:36 +0100
> Nikolay Aleksandrov  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

Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Rick Jones]

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

Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Rick Jones]

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

Re: [PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Stephen Hemminger]

On Wed,  3 Feb 2016 04:04:36 +0100
Nikolay Aleksandrov  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.

[PATCH net-next v5 1/2] ethtool: add speed/duplex validation functions

[Nikolay Aleksandrov]

From: Nikolay Aleksandrov 

Add functions which check if the speed/duplex are defined.

Signed-off-by: Nikolay Aleksandrov 
Acked-by: Michael S. Tsirkin 
---
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_1:
+   case SPEED_2:
+   case SPEED_25000:
+   case SPEED_4:
+   case SPEED_5:
+   case SPEED_56000:
+   case SPEED_10:
+   case SPEED_UNKNOWN:
+   return 1;
+   }
+
+   return 0;
+}
+
 /* Duplex, half or full. */
 #define DUPLEX_HALF0x00
 #define DUPLEX_FULL0x01
 #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_TP0x00
 #define PORT_AUI   0x01
-- 
2.4.3