[PATCH v2 0/3] ethtool: fix use of SPEED_UNKNOWN constant

[PATCH v2 0/3] ethtool: fix use of SPEED_UNKNOWN constant

[Michael Zhivich]

This patch series addresses 2 related issues:

1. ethtool_validate_speed() triggers a "signed-unsigned comparison"
warning due to type difference of SPEED_UNKNOWN constant (int)
and argument to ethtool_validate_speed (__u32).

2. some drivers use u16 storage for SPEED_UNKNOWN constant, 
resulting in value truncation and thus failure to test against
SPEED_UNKNOWN correctly.

This revised series addresses several feedback comments:
- split up the patch in to series
- do not unnecessarily change drivers that use "int" storage
  for speed values

Michael Zhivich (3):
  ethtool: avoid signed-unsigned comparison in ethtool_validate_speed()
  broadcom: tg3: fix use of SPEED_UNKNOWN ethtool constant
  qlogic: qlcnic: fix use of SPEED_UNKNOWN ethtool constant

 drivers/net/ethernet/broadcom/tg3.c         | 8 ++++----
 drivers/net/ethernet/broadcom/tg3.h         | 4 ++--
 drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 2 +-
 include/uapi/linux/ethtool.h                | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] ethtool: avoid signed-unsigned comparison in ethtool_validate_speed()

[Michael Zhivich]

When building C++ userspace code that includes ethtool.h
with "-Werror -Wall", g++ complains about signed-unsigned comparison in
ethtool_validate_speed() due to definition of SPEED_UNKNOWN as -1.

Explicitly cast SPEED_UNKNOWN to __u32 to match type of
ethtool_validate_speed() argument.

Signed-off-by: Michael Zhivich <mzhivich@akamai.com>
---
 include/uapi/linux/ethtool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 3652b239..d473e5e 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1591,7 +1591,7 @@ enum ethtool_link_mode_bit_indices {
 
 static inline int ethtool_validate_speed(__u32 speed)
 {
-	return speed <= INT_MAX || speed == SPEED_UNKNOWN;
+	return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN;
 }
 
 /* Duplex, half or full. */
-- 
2.7.4

[PATCH v2 2/3] broadcom: tg3: fix use of SPEED_UNKNOWN ethtool constant

[Michael Zhivich]

tg3 driver uses u16 to store SPEED_UKNOWN ethtool constant,
which is defined as -1, resulting in value truncation and
thus incorrect test results against SPEED_UNKNOWN.

For example, the following test will print "False":

	u16 speed = SPEED_UNKNOWN;

	if (speed == SPEED_UNKNOWN)
	    printf("True");
	else
	    printf("False");

Change storage of speed to use u32 to avoid this issue.

Signed-off-by: Michael Zhivich <mzhivich@akamai.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 8 ++++----
 drivers/net/ethernet/broadcom/tg3.h | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 328373e..060a6f3 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -4283,7 +4283,7 @@ static void tg3_power_down(struct tg3 *tp)
 	pci_set_power_state(tp->pdev, PCI_D3hot);
 }
 
-static void tg3_aux_stat_to_speed_duplex(struct tg3 *tp, u32 val, u16 *speed, u8 *duplex)
+static void tg3_aux_stat_to_speed_duplex(struct tg3 *tp, u32 val, u32 *speed, u8 *duplex)
 {
 	switch (val & MII_TG3_AUX_STAT_SPDMASK) {
 	case MII_TG3_AUX_STAT_10HALF:
@@ -4787,7 +4787,7 @@ static int tg3_setup_copper_phy(struct tg3 *tp, bool force_reset)
 	bool current_link_up;
 	u32 bmsr, val;
 	u32 lcl_adv, rmt_adv;
-	u16 current_speed;
+	u32 current_speed;
 	u8 current_duplex;
 	int i, err;
 
@@ -5719,7 +5719,7 @@ static bool tg3_setup_fiber_by_hand(struct tg3 *tp, u32 mac_status)
 static int tg3_setup_fiber_phy(struct tg3 *tp, bool force_reset)
 {
 	u32 orig_pause_cfg;
-	u16 orig_active_speed;
+	u32 orig_active_speed;
 	u8 orig_active_duplex;
 	u32 mac_status;
 	bool current_link_up;
@@ -5823,7 +5823,7 @@ static int tg3_setup_fiber_mii_phy(struct tg3 *tp, bool force_reset)
 {
 	int err = 0;
 	u32 bmsr, bmcr;
-	u16 current_speed = SPEED_UNKNOWN;
+	u32 current_speed = SPEED_UNKNOWN;
 	u8 current_duplex = DUPLEX_UNKNOWN;
 	bool current_link_up = false;
 	u32 local_adv, remote_adv, sgsr;
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index a772a33..6953d05 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -2873,7 +2873,7 @@ struct tg3_tx_ring_info {
 struct tg3_link_config {
 	/* Describes what we're trying to get. */
 	u32				advertising;
-	u16				speed;
+	u32				speed;
 	u8				duplex;
 	u8				autoneg;
 	u8				flowctrl;
@@ -2882,7 +2882,7 @@ struct tg3_link_config {
 	u8				active_flowctrl;
 
 	u8				active_duplex;
-	u16				active_speed;
+	u32				active_speed;
 	u32				rmt_adv;
 };
 
-- 
2.7.4

Re: [PATCH v2 2/3] broadcom: tg3: fix use of SPEED_UNKNOWN ethtool constant

[Andrew Lunn]

On Mon, Apr 08, 2019 at 10:48:46AM -0400, Michael Zhivich wrote:
> tg3 driver uses u16 to store SPEED_UKNOWN ethtool constant,
> which is defined as -1, resulting in value truncation and
> thus incorrect test results against SPEED_UNKNOWN.
> 
> For example, the following test will print "False":
> 
> 	u16 speed = SPEED_UNKNOWN;
> 
> 	if (speed == SPEED_UNKNOWN)
> 	    printf("True");
> 	else
> 	    printf("False");
> 
> Change storage of speed to use u32 to avoid this issue.
> 
> Signed-off-by: Michael Zhivich <mzhivich@akamai.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 3/3] qlogic: qlcnic: fix use of SPEED_UNKNOWN ethtool constant

[Michael Zhivich]

qlcnic driver uses u16 to store SPEED_UKNOWN ethtool constant,
which is defined as -1, resulting in value truncation and
thus incorrect test results against SPEED_UNKNOWN.

For example, the following test will print "False":

    u16 speed = SPEED_UNKNOWN;

    if (speed == SPEED_UNKNOWN)
        printf("True");
    else
        printf("False");

Change storage of speed to use u32 to avoid this issue.

Signed-off-by: Michael Zhivich <mzhivich@akamai.com>
---
 drivers/net/ethernet/qlogic/qlcnic/qlcnic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
index 0c443ea..374a4d4 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic.h
@@ -497,7 +497,7 @@ struct qlcnic_hardware_context {
 	u16 board_type;
 	u16 supported_type;
 
-	u16 link_speed;
+	u32 link_speed;
 	u16 link_duplex;
 	u16 link_autoneg;
 	u16 module_type;
-- 
2.7.4

Re: [PATCH v2 3/3] qlogic: qlcnic: fix use of SPEED_UNKNOWN ethtool constant

[Andrew Lunn]

On Mon, Apr 08, 2019 at 10:48:47AM -0400, Michael Zhivich wrote:
> qlcnic driver uses u16 to store SPEED_UKNOWN ethtool constant,
> which is defined as -1, resulting in value truncation and
> thus incorrect test results against SPEED_UNKNOWN.
> 
> For example, the following test will print "False":
> 
>     u16 speed = SPEED_UNKNOWN;
> 
>     if (speed == SPEED_UNKNOWN)
>         printf("True");
>     else
>         printf("False");
> 
> Change storage of speed to use u32 to avoid this issue.
> 
> Signed-off-by: Michael Zhivich <mzhivich@akamai.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 0/3] ethtool: fix use of SPEED_UNKNOWN constant

[Andrew Lunn]

On Mon, Apr 08, 2019 at 10:48:44AM -0400, Michael Zhivich wrote:
> This patch series addresses 2 related issues:
> 
> 1. ethtool_validate_speed() triggers a "signed-unsigned comparison"
> warning due to type difference of SPEED_UNKNOWN constant (int)
> and argument to ethtool_validate_speed (__u32).
> 
> 2. some drivers use u16 storage for SPEED_UNKNOWN constant, 
> resulting in value truncation and thus failure to test against
> SPEED_UNKNOWN correctly.
> 
> This revised series addresses several feedback comments:
> - split up the patch in to series
> - do not unnecessarily change drivers that use "int" storage
>   for speed values

Hi Michael

Your v1 had some changes which changed plain int value speeds to using
the SPEED_X macros. Those changes appear to of disappeared. You could
submit them as well.

       Thanks
	Andrew

Re: [PATCH v2 0/3] ethtool: fix use of SPEED_UNKNOWN constant

[Zhivich, Michael]

On 4/8/19, 1:55 PM, "Andrew Lunn" <andrew@lunn.ch> wrote:

>On Mon, Apr 08, 2019 at 10:48:44AM -0400, Michael Zhivich wrote:
>> This patch series addresses 2 related issues:
>> 
>> 1. ethtool_validate_speed() triggers a "signed-unsigned comparison"
>> warning due to type difference of SPEED_UNKNOWN constant (int)
>> and argument to ethtool_validate_speed (__u32).
>> 
>> 2. some drivers use u16 storage for SPEED_UNKNOWN constant, 
>> resulting in value truncation and thus failure to test against
>> SPEED_UNKNOWN correctly.
>> 
>> This revised series addresses several feedback comments:
>> - split up the patch in to series
>> - do not unnecessarily change drivers that use "int" storage
>>   for speed values
>
>Hi Michael
>
>Your v1 had some changes which changed plain int value speeds to using
>the SPEED_X macros. Those changes appear to of disappeared. You could
>submit them as well.
>
>       Thanks
>	Andrew

Hi Andrew,

I dropped those changes since they were purely cosmetic and not 
required for this patch series.  

I'll resubmit that set of changes as a separate patch.

Thanks,
~ Michael

Re: [PATCH v2 0/3] ethtool: fix use of SPEED_UNKNOWN constant

[David Miller]

From: Michael Zhivich <mzhivich@akamai.com>
Date: Mon, 8 Apr 2019 10:48:44 -0400

> This patch series addresses 2 related issues:
> 
> 1. ethtool_validate_speed() triggers a "signed-unsigned comparison"
> warning due to type difference of SPEED_UNKNOWN constant (int)
> and argument to ethtool_validate_speed (__u32).
> 
> 2. some drivers use u16 storage for SPEED_UNKNOWN constant, 
> resulting in value truncation and thus failure to test against
> SPEED_UNKNOWN correctly.
> 
> This revised series addresses several feedback comments:
> - split up the patch in to series
> - do not unnecessarily change drivers that use "int" storage
>   for speed values

Series applied.