[PATCH 1/4] net/e1000: Map custom error codes to more appropriate errno values

[PATCH 1/4] net/e1000: Map custom error codes to more appropriate errno values

[Andrey Smirnov]

A number of custom error codes used by e1000 driver will be propagated
all the way up to generic networking code and will end up being fed to
strerror(). As a result of that, some of the current error codes will
result in not very helpful failure messages. For example, trying to
ping a host on a system where access to i210's EEPROM fails results in
the following message:

  barebox@ZII RDU2 Board:/ ping 192.168.53.7
  ping failed: Operation not permitted

In order to make message like that one a little bit more helpful,
change definitions of various E1000_ERR_* constants to map to a bit
more appropriate error codes.

While at it, remove E1000_ERR_MASTER_REQUESTS_PENDING and
E1000_ERR_HOST_INTERFACE_COMMAND that are not referenced anywhere in
the codebase.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/e1000/e1000.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 4a1a1aa33..0a9e107c0 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -95,19 +95,17 @@ typedef enum {
 
 /* Error Codes */
 #define E1000_SUCCESS				0
-#define E1000_ERR_EEPROM			1
-#define E1000_ERR_PHY				2
-#define E1000_ERR_CONFIG			3
-#define E1000_ERR_PARAM				4
-#define E1000_ERR_MAC_TYPE			5
-#define E1000_ERR_PHY_TYPE			6
-#define E1000_ERR_NOLINK			7
-#define E1000_ERR_TIMEOUT			8
-#define E1000_ERR_RESET				9
-#define E1000_ERR_MASTER_REQUESTS_PENDING	10
-#define E1000_ERR_HOST_INTERFACE_COMMAND	11
-#define E1000_BLK_PHY_RESET			12
-#define E1000_ERR_SWFW_SYNC 			13
+#define E1000_ERR_EEPROM			EIO
+#define E1000_ERR_PHY				EIO
+#define E1000_ERR_CONFIG			EINVAL
+#define E1000_ERR_PARAM				EINVAL
+#define E1000_ERR_MAC_TYPE			EINVAL
+#define E1000_ERR_PHY_TYPE			EINVAL
+#define E1000_ERR_NOLINK			ENETDOWN
+#define E1000_ERR_TIMEOUT			ETIMEDOUT
+#define E1000_ERR_RESET				EIO
+#define E1000_BLK_PHY_RESET			EWOULDBLOCK
+#define E1000_ERR_SWFW_SYNC 			EBUSY
 
 /* PCI Device IDs */
 #define E1000_DEV_ID_82542			0x1000
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/4] net/e1000: Do not discard EEPROM error code in e1000_setup_link()

[Andrey Smirnov]

E1000_read_eeprom() returns a number of different error codes, so
propagate them up the caller chain instead of reducing it to
E1000_READ_EEPROM.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/e1000/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/e1000/main.c b/drivers/net/e1000/main.c
index caa7274a8..d631e44b6 100644
--- a/drivers/net/e1000/main.c
+++ b/drivers/net/e1000/main.c
@@ -840,10 +840,11 @@ static int e1000_setup_link(struct e1000_hw *hw)
 	 * control setting, then the variable hw->fc will
 	 * be initialized based on a value in the EEPROM.
 	 */
-	if (e1000_read_eeprom(hw, EEPROM_INIT_CONTROL2_REG, 1,
-				&eeprom_data) < 0) {
+	ret_val = e1000_read_eeprom(hw, EEPROM_INIT_CONTROL2_REG, 1,
+				    &eeprom_data);
+	if (ret_val < 0) {
 		dev_dbg(hw->dev, "EEPROM Read Error\n");
-		return -E1000_ERR_EEPROM;
+		return ret_val;
 	}
 
 	switch (hw->mac_type) {
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/4] net/e1000: Use dev_err to report error

[Andrey Smirnov]

Those messages shouldn't be reported in normal use scenarious and in
the case of error using dev_err saves user the trouble of having to
run a custom build of Barebox with e1000 debugging enabled to see some
initial diagnostic messages.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/e1000/main.c | 70 ++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/net/e1000/main.c b/drivers/net/e1000/main.c
index d631e44b6..87ee46094 100644
--- a/drivers/net/e1000/main.c
+++ b/drivers/net/e1000/main.c
@@ -231,7 +231,7 @@ static int32_t e1000_get_hw_eeprom_semaphore(struct e1000_hw *hw)
 	if (!timeout) {
 		/* Release semaphores */
 		e1000_put_hw_eeprom_semaphore(hw);
-		dev_dbg(hw->dev, "Driver can't access the Eeprom - "
+		dev_err(hw->dev, "Driver can't access the Eeprom - "
 				"SWESMBI bit is set.\n");
 		return -E1000_ERR_EEPROM;
 	}
@@ -262,7 +262,7 @@ int32_t e1000_swfw_sync_acquire(struct e1000_hw *hw, uint16_t mask)
 	}
 
 	if (!timeout) {
-		dev_dbg(hw->dev, "Driver can't access resource, SW_FW_SYNC timeout.\n");
+		dev_err(hw->dev, "Driver can't access resource, SW_FW_SYNC timeout.\n");
 		return -E1000_ERR_SWFW_SYNC;
 	}
 
@@ -332,7 +332,7 @@ static int e1000_get_ethaddr(struct eth_device *edev, unsigned char *adr)
 
 	for (i = 0; i < NODE_ADDRESS_SIZE; i += 2) {
 		if (e1000_read_eeprom(hw, i >> 1, 1, &eeprom_data) < 0) {
-			dev_dbg(hw->dev, "EEPROM Read Error\n");
+			dev_err(hw->dev, "EEPROM Read Error\n");
 			return -E1000_ERR_EEPROM;
 		}
 		adr[i] = eeprom_data & 0xff;
@@ -843,7 +843,7 @@ static int e1000_setup_link(struct e1000_hw *hw)
 	ret_val = e1000_read_eeprom(hw, EEPROM_INIT_CONTROL2_REG, 1,
 				    &eeprom_data);
 	if (ret_val < 0) {
-		dev_dbg(hw->dev, "EEPROM Read Error\n");
+		dev_err(hw->dev, "EEPROM Read Error\n");
 		return ret_val;
 	}
 
@@ -1007,7 +1007,7 @@ static int e1000_setup_fiber_link(struct e1000_hw *hw)
 		txcw = E1000_TXCW_ANE | E1000_TXCW_FD | E1000_TXCW_PAUSE_MASK;
 		break;
 	default:
-		dev_dbg(hw->dev, "Flow control param set incorrectly\n");
+		dev_err(hw->dev, "Flow control param set incorrectly\n");
 		return -E1000_ERR_CONFIG;
 		break;
 	}
@@ -1045,7 +1045,7 @@ static int e1000_setup_fiber_link(struct e1000_hw *hw)
 			 * detect a signal. This will allow us to communicate with
 			 * non-autonegotiating link partners.
 			 */
-			dev_dbg(hw->dev, "Never got a valid link from auto-neg!!!\n");
+			dev_err(hw->dev, "Never got a valid link from auto-neg!!!\n");
 			hw->autoneg_failed = 1;
 			return -E1000_ERR_NOLINK;
 		} else {
@@ -1053,7 +1053,7 @@ static int e1000_setup_fiber_link(struct e1000_hw *hw)
 			dev_dbg(hw->dev, "Valid Link Found\n");
 		}
 	} else {
-		dev_dbg(hw->dev, "No Signal Detected\n");
+		dev_err(hw->dev, "No Signal Detected\n");
 		return -E1000_ERR_NOLINK;
 	}
 	return 0;
@@ -1093,7 +1093,7 @@ static int32_t e1000_copper_link_preconfig(struct e1000_hw *hw)
 	/* Make sure we have a valid PHY */
 	ret_val = e1000_detect_gig_phy(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error, did not detect valid phy.\n");
+		dev_err(hw->dev, "Error, did not detect valid phy.\n");
 		return ret_val;
 	}
 	dev_dbg(hw->dev, "Phy ID = %x \n", hw->phy_id);
@@ -1237,7 +1237,7 @@ static int32_t e1000_copper_link_igp_setup(struct e1000_hw *hw)
 
 	ret_val = e1000_phy_reset(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error Resetting the PHY\n");
+		dev_err(hw->dev, "Error Resetting the PHY\n");
 		return ret_val;
 	}
 
@@ -1256,7 +1256,7 @@ static int32_t e1000_copper_link_igp_setup(struct e1000_hw *hw)
 		/* disable lplu d3 during driver init */
 		ret_val = e1000_set_d3_lplu_state_off(hw);
 		if (ret_val) {
-			dev_dbg(hw->dev, "Error Disabling LPLU D3\n");
+			dev_err(hw->dev, "Error Disabling LPLU D3\n");
 			return ret_val;
 		}
 	}
@@ -1264,7 +1264,7 @@ static int32_t e1000_copper_link_igp_setup(struct e1000_hw *hw)
 	/* disable lplu d0 during driver init */
 	ret_val = e1000_set_d0_lplu_state_off(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error Disabling LPLU D0\n");
+		dev_err(hw->dev, "Error Disabling LPLU D0\n");
 		return ret_val;
 	}
 
@@ -1458,7 +1458,7 @@ static int32_t e1000_copper_link_ggp_setup(struct e1000_hw *hw)
 	/* SW Reset the PHY so all changes take effect */
 	ret_val = e1000_phy_reset(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error Resetting the PHY\n");
+		dev_err(hw->dev, "Error Resetting the PHY\n");
 		return ret_val;
 	}
 
@@ -1587,7 +1587,7 @@ static int32_t e1000_copper_link_mgp_setup(struct e1000_hw *hw)
 	/* SW Reset the PHY so all changes take effect */
 	ret_val = e1000_phy_reset(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error Resetting the PHY\n");
+		dev_err(hw->dev, "Error Resetting the PHY\n");
 		return ret_val;
 	}
 
@@ -1616,7 +1616,7 @@ static int32_t e1000_copper_link_autoneg(struct e1000_hw *hw)
 	dev_dbg(hw->dev, "Reconfiguring auto-neg advertisement params\n");
 	ret_val = e1000_phy_setup_autoneg(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error Setting up Auto-Negotiation\n");
+		dev_err(hw->dev, "Error Setting up Auto-Negotiation\n");
 		return ret_val;
 	}
 	dev_dbg(hw->dev, "Restarting Auto-Neg\n");
@@ -1635,7 +1635,7 @@ static int32_t e1000_copper_link_autoneg(struct e1000_hw *hw)
 
 	ret_val = e1000_wait_autoneg(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error while waiting for autoneg to complete\n");
+		dev_err(hw->dev, "Error while waiting for autoneg to complete\n");
 		return ret_val;
 	}
 
@@ -1664,14 +1664,14 @@ static int32_t e1000_copper_link_postconfig(struct e1000_hw *hw)
 	} else {
 		ret_val = e1000_config_mac_to_phy(hw);
 		if (ret_val) {
-			dev_dbg(hw->dev, "Error configuring MAC to PHY settings\n");
+			dev_err(hw->dev, "Error configuring MAC to PHY settings\n");
 			return ret_val;
 		}
 	}
 
 	ret_val = e1000_config_fc_after_link_up(hw);
 	if (ret_val) {
-		dev_dbg(hw->dev, "Error Configuring Flow Control\n");
+		dev_err(hw->dev, "Error Configuring Flow Control\n");
 		return ret_val;
 	}
 
@@ -1984,7 +1984,7 @@ static int e1000_config_mac_to_phy(struct e1000_hw *hw)
 	 * registers depending on negotiated values.
 	 */
 	if (e1000_read_phy_reg(hw, M88E1000_PHY_SPEC_STATUS, &phy_data) < 0) {
-		dev_dbg(hw->dev, "PHY Read Error\n");
+		dev_err(hw->dev, "PHY Read Error\n");
 		return -E1000_ERR_PHY;
 	}
 	if (phy_data & M88E1000_PSSR_DPLX)
@@ -2060,7 +2060,7 @@ static int e1000_force_mac_fc(struct e1000_hw *hw)
 		ctrl |= (E1000_CTRL_TFCE | E1000_CTRL_RFCE);
 		break;
 	default:
-		dev_dbg(hw->dev, "Flow control param set incorrectly\n");
+		dev_err(hw->dev, "Flow control param set incorrectly\n");
 		return -E1000_ERR_CONFIG;
 	}
 
@@ -2099,17 +2099,17 @@ static int32_t e1000_config_fc_after_link_up(struct e1000_hw *hw)
 	 * some "sticky" (latched) bits.
 	 */
 	if (e1000_read_phy_reg(hw, PHY_STATUS, &mii_status_reg) < 0) {
-		dev_dbg(hw->dev, "PHY Read Error \n");
+		dev_err(hw->dev, "PHY Read Error \n");
 		return -E1000_ERR_PHY;
 	}
 
 	if (e1000_read_phy_reg(hw, PHY_STATUS, &mii_status_reg) < 0) {
-		dev_dbg(hw->dev, "PHY Read Error \n");
+		dev_err(hw->dev, "PHY Read Error \n");
 		return -E1000_ERR_PHY;
 	}
 
 	if (!(mii_status_reg & MII_SR_AUTONEG_COMPLETE)) {
-		dev_dbg(hw->dev, "Copper PHY and Auto Neg has not completed.\n");
+		dev_err(hw->dev, "Copper PHY and Auto Neg has not completed.\n");
 		return 0;
 	}
 
@@ -2120,12 +2120,12 @@ static int32_t e1000_config_fc_after_link_up(struct e1000_hw *hw)
 	 * negotiated.
 	 */
 	if (e1000_read_phy_reg(hw, PHY_AUTONEG_ADV, &mii_nway_adv_reg) < 0) {
-		dev_dbg(hw->dev, "PHY Read Error\n");
+		dev_err(hw->dev, "PHY Read Error\n");
 		return -E1000_ERR_PHY;
 	}
 
 	if (e1000_read_phy_reg(hw, PHY_LP_ABILITY, &mii_nway_lp_ability_reg) < 0) {
-		dev_dbg(hw->dev, "PHY Read Error\n");
+		dev_err(hw->dev, "PHY Read Error\n");
 		return -E1000_ERR_PHY;
 	}
 
@@ -2251,7 +2251,7 @@ static int32_t e1000_config_fc_after_link_up(struct e1000_hw *hw)
 	 */
 	ret_val = e1000_force_mac_fc(hw);
 	if (ret_val < 0) {
-		dev_dbg(hw->dev, "Error forcing flow control settings\n");
+		dev_err(hw->dev, "Error forcing flow control settings\n");
 		return ret_val;
 	}
 
@@ -2400,11 +2400,11 @@ static int e1000_wait_autoneg(struct e1000_hw *hw)
 		 * Complete bit to be set.
 		 */
 		if (e1000_read_phy_reg(hw, PHY_STATUS, &phy_data) < 0) {
-			dev_dbg(hw->dev, "PHY Read Error\n");
+			dev_err(hw->dev, "PHY Read Error\n");
 			return -E1000_ERR_PHY;
 		}
 		if (e1000_read_phy_reg(hw, PHY_STATUS, &phy_data) < 0) {
-			dev_dbg(hw->dev, "PHY Read Error\n");
+			dev_err(hw->dev, "PHY Read Error\n");
 			return -E1000_ERR_PHY;
 		}
 		if (phy_data & MII_SR_AUTONEG_COMPLETE) {
@@ -2413,7 +2413,7 @@ static int e1000_wait_autoneg(struct e1000_hw *hw)
 		}
 		mdelay(100);
 	}
-	dev_dbg(hw->dev, "Auto-Neg timedout.\n");
+	dev_err(hw->dev, "Auto-Neg timedout.\n");
 	return -E1000_ERR_TIMEOUT;
 }
 
@@ -2579,11 +2579,11 @@ static int e1000_phy_read(struct mii_bus *bus, int phy_addr, int reg_addr)
 				break;
 		}
 		if (!(mdic & E1000_MDIC_READY)) {
-			dev_dbg(hw->dev, "MDI Read did not complete\n");
+			dev_err(hw->dev, "MDI Read did not complete\n");
 			return -E1000_ERR_PHY;
 		}
 		if (mdic & E1000_MDIC_ERROR) {
-			dev_dbg(hw->dev, "MDI Error\n");
+			dev_err(hw->dev, "MDI Error\n");
 			return -E1000_ERR_PHY;
 		}
 		return mdic;
@@ -2668,7 +2668,7 @@ static int e1000_phy_write(struct mii_bus *bus, int phy_addr,
 				break;
 		}
 		if (!(mdic & E1000_MDIC_READY)) {
-			dev_dbg(hw->dev, "MDI Write did not complete\n");
+			dev_err(hw->dev, "MDI Write did not complete\n");
 			return -E1000_ERR_PHY;
 		}
 	} else {
@@ -2775,7 +2775,7 @@ static int32_t e1000_get_phy_cfg_done(struct e1000_hw *hw)
 			timeout--;
 		}
 		if (!timeout) {
-			dev_dbg(hw->dev, "MNG configuration cycle has not completed.\n");
+			dev_err(hw->dev, "MNG configuration cycle has not completed.\n");
 			return -E1000_ERR_RESET;
 		}
 		break;
@@ -2811,7 +2811,7 @@ static int32_t e1000_phy_hw_reset(struct e1000_hw *hw)
 			swfw = E1000_SWFW_PHY1_SM;
 
 		if (e1000_swfw_sync_acquire(hw, swfw)) {
-			dev_dbg(hw->dev, "Unable to acquire swfw sync\n");
+			dev_err(hw->dev, "Unable to acquire swfw sync\n");
 			return -E1000_ERR_SWFW_SYNC;
 		}
 
@@ -3110,12 +3110,12 @@ static int32_t e1000_detect_gig_phy(struct e1000_hw *hw)
 			phy_type = e1000_phy_igb;
 		break;
 	default:
-		dev_dbg(hw->dev, "Invalid MAC type %d\n", hw->mac_type);
+		dev_err(hw->dev, "Invalid MAC type %d\n", hw->mac_type);
 		return -E1000_ERR_CONFIG;
 	}
 
 	if (phy_type == e1000_phy_undefined) {
-		dev_dbg(hw->dev, "Invalid PHY ID 0x%X\n", hw->phy_id);
+		dev_err(hw->dev, "Invalid PHY ID 0x%X\n", hw->phy_id);
 		return -EINVAL;
 	}
 
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/4] net/e1000: Only read EEPROM_INIT_CONTROL2_REG if it is needed

[Andrey Smirnov]

E1000_ich8lan, e1000_82573, e1000_82574 and e1000_igb
devices (hw->mac_type) do not use data read from
EEPROM_INIT_CONTROL2_REG in e1000_setup_link(), so there's no reason
for it to bail out when EEPROM read fails. An examlpe use-case would
be a i210 adapter initialized from iNVM with no valid EEPROM attached.

Change the code to only call e1000_read_eeprom() for devices that do
need it.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/net/e1000/main.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/net/e1000/main.c b/drivers/net/e1000/main.c
index 87ee46094..0ef8fd623 100644
--- a/drivers/net/e1000/main.c
+++ b/drivers/net/e1000/main.c
@@ -832,21 +832,6 @@ static int e1000_setup_link(struct e1000_hw *hw)
 	if (e1000_check_phy_reset_block(hw))
 		return E1000_SUCCESS;
 
-	/* Read and store word 0x0F of the EEPROM. This word contains bits
-	 * that determine the hardware's default PAUSE (flow control) mode,
-	 * a bit that determines whether the HW defaults to enabling or
-	 * disabling auto-negotiation, and the direction of the
-	 * SW defined pins. If there is no SW over-ride of the flow
-	 * control setting, then the variable hw->fc will
-	 * be initialized based on a value in the EEPROM.
-	 */
-	ret_val = e1000_read_eeprom(hw, EEPROM_INIT_CONTROL2_REG, 1,
-				    &eeprom_data);
-	if (ret_val < 0) {
-		dev_err(hw->dev, "EEPROM Read Error\n");
-		return ret_val;
-	}
-
 	switch (hw->mac_type) {
 	case e1000_ich8lan:
 	case e1000_82573:
@@ -855,6 +840,22 @@ static int e1000_setup_link(struct e1000_hw *hw)
 		hw->fc = e1000_fc_full;
 		break;
 	default:
+		/* Read and store word 0x0F of the EEPROM. This word
+		 * contains bits that determine the hardware's default
+		 * PAUSE (flow control) mode, a bit that determines
+		 * whether the HW defaults to enabling or disabling
+		 * auto-negotiation, and the direction of the SW
+		 * defined pins. If there is no SW over-ride of the
+		 * flow control setting, then the variable hw->fc will
+		 * be initialized based on a value in the EEPROM.
+		 */
+		ret_val = e1000_read_eeprom(hw, EEPROM_INIT_CONTROL2_REG, 1,
+					    &eeprom_data);
+		if (ret_val < 0) {
+			dev_err(hw->dev, "EEPROM Read Error\n");
+			return ret_val;
+		}
+
 		if ((eeprom_data & EEPROM_WORD0F_PAUSE_MASK) == 0)
 			hw->fc = e1000_fc_none;
 		else if ((eeprom_data & EEPROM_WORD0F_PAUSE_MASK) == EEPROM_WORD0F_ASM_DIR)
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] net/e1000: Map custom error codes to more appropriate errno values

[Sascha Hauer]

On Wed, Dec 12, 2018 at 11:03:33PM -0800, Andrey Smirnov wrote:
> A number of custom error codes used by e1000 driver will be propagated
> all the way up to generic networking code and will end up being fed to
> strerror(). As a result of that, some of the current error codes will
> result in not very helpful failure messages. For example, trying to
> ping a host on a system where access to i210's EEPROM fails results in
> the following message:
> 
>   barebox@ZII RDU2 Board:/ ping 192.168.53.7
>   ping failed: Operation not permitted
> 
> In order to make message like that one a little bit more helpful,
> change definitions of various E1000_ERR_* constants to map to a bit
> more appropriate error codes.
> 
> While at it, remove E1000_ERR_MASTER_REQUESTS_PENDING and
> E1000_ERR_HOST_INTERFACE_COMMAND that are not referenced anywhere in
> the codebase.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/net/e1000/e1000.h | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> index 4a1a1aa33..0a9e107c0 100644
> --- a/drivers/net/e1000/e1000.h
> +++ b/drivers/net/e1000/e1000.h
> @@ -95,19 +95,17 @@ typedef enum {
>  
>  /* Error Codes */
>  #define E1000_SUCCESS				0
> -#define E1000_ERR_EEPROM			1
> -#define E1000_ERR_PHY				2
> -#define E1000_ERR_CONFIG			3
> -#define E1000_ERR_PARAM				4
> -#define E1000_ERR_MAC_TYPE			5
> -#define E1000_ERR_PHY_TYPE			6
> -#define E1000_ERR_NOLINK			7
> -#define E1000_ERR_TIMEOUT			8
> -#define E1000_ERR_RESET				9
> -#define E1000_ERR_MASTER_REQUESTS_PENDING	10
> -#define E1000_ERR_HOST_INTERFACE_COMMAND	11
> -#define E1000_BLK_PHY_RESET			12
> -#define E1000_ERR_SWFW_SYNC 			13
> +#define E1000_ERR_EEPROM			EIO
> +#define E1000_ERR_PHY				EIO
> +#define E1000_ERR_CONFIG			EINVAL
> +#define E1000_ERR_PARAM				EINVAL
> +#define E1000_ERR_MAC_TYPE			EINVAL
> +#define E1000_ERR_PHY_TYPE			EINVAL
> +#define E1000_ERR_NOLINK			ENETDOWN
> +#define E1000_ERR_TIMEOUT			ETIMEDOUT
> +#define E1000_ERR_RESET				EIO
> +#define E1000_BLK_PHY_RESET			EWOULDBLOCK
> +#define E1000_ERR_SWFW_SYNC 			EBUSY
>  
>  /* PCI Device IDs */
>  #define E1000_DEV_ID_82542			0x1000
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] net/e1000: Map custom error codes to more appropriate errno values

[Roland Hieber]

On Wed, Dec 12, 2018 at 11:03:33PM -0800, Andrey Smirnov wrote:
> A number of custom error codes used by e1000 driver will be propagated
> all the way up to generic networking code and will end up being fed to
> strerror(). As a result of that, some of the current error codes will
> result in not very helpful failure messages. For example, trying to
> ping a host on a system where access to i210's EEPROM fails results in
> the following message:
> 
>   barebox@ZII RDU2 Board:/ ping 192.168.53.7
>   ping failed: Operation not permitted
> 
> In order to make message like that one a little bit more helpful,
> change definitions of various E1000_ERR_* constants to map to a bit
> more appropriate error codes.
> 
> While at it, remove E1000_ERR_MASTER_REQUESTS_PENDING and
> E1000_ERR_HOST_INTERFACE_COMMAND that are not referenced anywhere in
> the codebase.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/net/e1000/e1000.h | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> index 4a1a1aa33..0a9e107c0 100644
> --- a/drivers/net/e1000/e1000.h
> +++ b/drivers/net/e1000/e1000.h
> @@ -95,19 +95,17 @@ typedef enum {
>  
>  /* Error Codes */
>  #define E1000_SUCCESS				0
> -#define E1000_ERR_EEPROM			1
> -#define E1000_ERR_PHY				2
> -#define E1000_ERR_CONFIG			3
> -#define E1000_ERR_PARAM				4
> -#define E1000_ERR_MAC_TYPE			5
> -#define E1000_ERR_PHY_TYPE			6
> -#define E1000_ERR_NOLINK			7
> -#define E1000_ERR_TIMEOUT			8
> -#define E1000_ERR_RESET				9
> -#define E1000_ERR_MASTER_REQUESTS_PENDING	10
> -#define E1000_ERR_HOST_INTERFACE_COMMAND	11
> -#define E1000_BLK_PHY_RESET			12
> -#define E1000_ERR_SWFW_SYNC 			13
> +#define E1000_ERR_EEPROM			EIO
> +#define E1000_ERR_PHY				EIO
> +#define E1000_ERR_CONFIG			EINVAL
> +#define E1000_ERR_PARAM				EINVAL
> +#define E1000_ERR_MAC_TYPE			EINVAL
> +#define E1000_ERR_PHY_TYPE			EINVAL
> +#define E1000_ERR_NOLINK			ENETDOWN
> +#define E1000_ERR_TIMEOUT			ETIMEDOUT
> +#define E1000_ERR_RESET				EIO
> +#define E1000_BLK_PHY_RESET			EWOULDBLOCK
> +#define E1000_ERR_SWFW_SYNC 			EBUSY

Just a small nitpick, and I'm not sure it's relevant somewhere in the
code: previously the mapping of symbols to numbers was bijective, but
now it is no longer surjective. This may result in confusion when
checking return codes, for example:

    int ret = e1000_set_phy_mode(...;
    if (ret == E1000_ERR_EEPROM) { 
        printf("could not read from eeprom\n");
    } else if (ret == E1000_ERR_PHY) {
        printf("could not write to phy register\n");
    }

In this case, when the e1000_set_phy_mode() code path returns
E1000_ERR_PHY, it is interpreted as E1000_ERR_EEPROM because both are
defined to EIO.

 - Roland

>  
>  /* PCI Device IDs */
>  #define E1000_DEV_ID_82542			0x1000
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] net/e1000: Map custom error codes to more appropriate errno values

[Andrey Smirnov]

On Thu, Jan 3, 2019 at 7:31 AM Roland Hieber <rhi@pengutronix.de> wrote:
>
> On Wed, Dec 12, 2018 at 11:03:33PM -0800, Andrey Smirnov wrote:
> > A number of custom error codes used by e1000 driver will be propagated
> > all the way up to generic networking code and will end up being fed to
> > strerror(). As a result of that, some of the current error codes will
> > result in not very helpful failure messages. For example, trying to
> > ping a host on a system where access to i210's EEPROM fails results in
> > the following message:
> >
> >   barebox@ZII RDU2 Board:/ ping 192.168.53.7
> >   ping failed: Operation not permitted
> >
> > In order to make message like that one a little bit more helpful,
> > change definitions of various E1000_ERR_* constants to map to a bit
> > more appropriate error codes.
> >
> > While at it, remove E1000_ERR_MASTER_REQUESTS_PENDING and
> > E1000_ERR_HOST_INTERFACE_COMMAND that are not referenced anywhere in
> > the codebase.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  drivers/net/e1000/e1000.h | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> > index 4a1a1aa33..0a9e107c0 100644
> > --- a/drivers/net/e1000/e1000.h
> > +++ b/drivers/net/e1000/e1000.h
> > @@ -95,19 +95,17 @@ typedef enum {
> >
> >  /* Error Codes */
> >  #define E1000_SUCCESS                                0
> > -#define E1000_ERR_EEPROM                     1
> > -#define E1000_ERR_PHY                                2
> > -#define E1000_ERR_CONFIG                     3
> > -#define E1000_ERR_PARAM                              4
> > -#define E1000_ERR_MAC_TYPE                   5
> > -#define E1000_ERR_PHY_TYPE                   6
> > -#define E1000_ERR_NOLINK                     7
> > -#define E1000_ERR_TIMEOUT                    8
> > -#define E1000_ERR_RESET                              9
> > -#define E1000_ERR_MASTER_REQUESTS_PENDING    10
> > -#define E1000_ERR_HOST_INTERFACE_COMMAND     11
> > -#define E1000_BLK_PHY_RESET                  12
> > -#define E1000_ERR_SWFW_SYNC                  13
> > +#define E1000_ERR_EEPROM                     EIO
> > +#define E1000_ERR_PHY                                EIO
> > +#define E1000_ERR_CONFIG                     EINVAL
> > +#define E1000_ERR_PARAM                              EINVAL
> > +#define E1000_ERR_MAC_TYPE                   EINVAL
> > +#define E1000_ERR_PHY_TYPE                   EINVAL
> > +#define E1000_ERR_NOLINK                     ENETDOWN
> > +#define E1000_ERR_TIMEOUT                    ETIMEDOUT
> > +#define E1000_ERR_RESET                              EIO
> > +#define E1000_BLK_PHY_RESET                  EWOULDBLOCK
> > +#define E1000_ERR_SWFW_SYNC                  EBUSY
>
> Just a small nitpick, and I'm not sure it's relevant somewhere in the
> code: previously the mapping of symbols to numbers was bijective, but
> now it is no longer surjective. This may result in confusion when
> checking return codes, for example:
>
>     int ret = e1000_set_phy_mode(...;
>     if (ret == E1000_ERR_EEPROM) {
>         printf("could not read from eeprom\n");
>     } else if (ret == E1000_ERR_PHY) {
>         printf("could not write to phy register\n");
>     }
>
> In this case, when the e1000_set_phy_mode() code path returns
> E1000_ERR_PHY, it is interpreted as E1000_ERR_EEPROM because both are
> defined to EIO.
>

That's definitely true. When writing this patch I looked for usages
similar to what you describe in the actual code of the driver, but
didn't see anything that should cause a problem. The driver has a
pretty sizable set of error codes, but AFAICT none of them are really
used as anything more than a negative number passed up the call chain.
Grepping for "E1000_ERR_*" in drivers/net/e1000 doesn't seem to show
any usages in any comparison statements. Finding all of the points in
the driver where error codes cross over into generic codebase and
doing errno re-mapping there seemed like a more invasive/complicated
alternative, so I did go for it.

That's how my thinking went, anyway. I can re-do the patch if we
decide that maintaining unique ID for each E1000_ERR_* code is
desired.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/4] net/e1000: Map custom error codes to more appropriate errno values

[Roland Hieber]

On Thu, Jan 03, 2019 at 05:47:09PM -0800, Andrey Smirnov wrote:
> That's definitely true. When writing this patch I looked for usages
> similar to what you describe in the actual code of the driver, but
> didn't see anything that should cause a problem. The driver has a
> pretty sizable set of error codes, but AFAICT none of them are really
> used as anything more than a negative number passed up the call chain.
> Grepping for "E1000_ERR_*" in drivers/net/e1000 doesn't seem to show
> any usages in any comparison statements. Finding all of the points in
> the driver where error codes cross over into generic codebase and
> doing errno re-mapping there seemed like a more invasive/complicated
> alternative, so I did go for it.
> 
> That's how my thinking went, anyway. I can re-do the patch if we
> decide that maintaining unique ID for each E1000_ERR_* code is
> desired.

Yeah, I also did a few quick greps through the code, and didn't find any
such usages. I also noticed that sometimes the e1000 driver returns
-EIO etc. directly instead of the E1000_ERR_* codes. I don't think your
patch needs a respin though, it was only a small nitpick from my side,
all of that can still be cleaned up in additional patches, if someone
wants to do it. ;-)

 - Roland

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox