[PATCH v3 1/5] net/e1000: remove one level of indentation

[PATCH v3 1/5] net/e1000: remove one level of indentation

[Lucas Stach]

By returning early if the MAC type isn't e1000_igb.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/e1000/eeprom.c | 121 +++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 60 deletions(-)

diff --git a/drivers/net/e1000/eeprom.c b/drivers/net/e1000/eeprom.c
index 748d8afe7922..ddac7de8a0e9 100644
--- a/drivers/net/e1000/eeprom.c
+++ b/drivers/net/e1000/eeprom.c
@@ -1551,79 +1551,80 @@ int e1000_eeprom_valid(struct e1000_hw *hw)
  */
 int e1000_register_eeprom(struct e1000_hw *hw)
 {
-	int ret = E1000_SUCCESS;
-
 	struct e1000_eeprom_info *eeprom = &hw->eeprom;
+	uint32_t eecd;
+	int ret;
 
-	if (hw->mac_type == e1000_igb) {
-		uint32_t eecd = e1000_read_reg(hw, E1000_EECD);
+	if (hw->mac_type != e1000_igb)
+		return E1000_SUCCESS;
 
-		hw->eepromcdev.dev = hw->dev;
-		hw->eepromcdev.ops = &e1000_eeprom_ops;
-		hw->eepromcdev.name = xasprintf("e1000-eeprom%d", hw->dev->id);
-		hw->eepromcdev.size = 0x1000;
+	eecd = e1000_read_reg(hw, E1000_EECD);
 
-		ret = devfs_create(&hw->eepromcdev);
-		if (ret < 0)
-			return ret;
+	hw->eepromcdev.dev = hw->dev;
+	hw->eepromcdev.ops = &e1000_eeprom_ops;
+	hw->eepromcdev.name = xasprintf("e1000-eeprom%d", hw->dev->id);
+	hw->eepromcdev.size = 0x1000;
+
+	ret = devfs_create(&hw->eepromcdev);
+	if (ret < 0)
+		return ret;
 
-		if (eecd & E1000_EECD_AUTO_RD) {
-			if (eecd & E1000_EECD_EE_PRES) {
-				if (eecd & E1000_EECD_FLASH_IN_USE) {
-					uint32_t fla = e1000_read_reg(hw, E1000_FLA);
-					dev_info(hw->dev,
-						 "Hardware programmed from flash (%ssecure)\n",
-						 fla & E1000_FLA_LOCKED ? "" : "un");
-				} else {
-					dev_info(hw->dev, "Hardware programmed from iNVM\n");
-				}
+	if (eecd & E1000_EECD_AUTO_RD) {
+		if (eecd & E1000_EECD_EE_PRES) {
+			if (eecd & E1000_EECD_FLASH_IN_USE) {
+				uint32_t fla = e1000_read_reg(hw, E1000_FLA);
+				dev_info(hw->dev,
+					 "Hardware programmed from flash (%ssecure)\n",
+					 fla & E1000_FLA_LOCKED ? "" : "un");
 			} else {
-				dev_warn(hw->dev, "Shadow RAM invalid\n");
+				dev_info(hw->dev, "Hardware programmed from iNVM\n");
 			}
 		} else {
-			/*
-			 * I never saw this case in practise and I'm unsure how
-			 * to handle that. Maybe just wait until the hardware is
-			 * up enough that this bit is set?
-			 */
-			dev_err(hw->dev, "Flash Auto-Read not done\n");
-		}
-
-		if (eecd & E1000_EECD_I210_FLASH_DETECTED) {
-			hw->mtd.parent = hw->dev;
-			hw->mtd.read = e1000_mtd_read;
-			hw->mtd.write = e1000_mtd_write;
-			hw->mtd.erase = e1000_mtd_erase;
-			hw->mtd.lock = e1000_mtd_lock;
-			hw->mtd.unlock = e1000_mtd_unlock;
-			hw->mtd.size = eeprom->word_size * 2;
-			hw->mtd.writesize = 1;
-			hw->mtd.subpage_sft = 0;
-
-			hw->mtd.eraseregions = xzalloc(sizeof(struct mtd_erase_region_info));
-			hw->mtd.erasesize = SZ_4K;
-			hw->mtd.eraseregions[0].erasesize = SZ_4K;
-			hw->mtd.eraseregions[0].numblocks = hw->mtd.size / SZ_4K;
-			hw->mtd.numeraseregions = 1;
-
-			hw->mtd.flags = MTD_CAP_NORFLASH;
-			hw->mtd.type = MTD_NORFLASH;
-
-			ret = add_mtd_device(&hw->mtd, "e1000-nor",
-					     DEVICE_ID_DYNAMIC);
-			if (ret) {
-				devfs_remove(&hw->eepromcdev);
-				return ret;
-			}
+			dev_warn(hw->dev, "Shadow RAM invalid\n");
 		}
+	} else {
+		/*
+		 * I never saw this case in practise and I'm unsure how
+		 * to handle that. Maybe just wait until the hardware is
+		 * up enough that this bit is set?
+		 */
+		dev_err(hw->dev, "Flash Auto-Read not done\n");
+	}
 
-		ret = e1000_register_invm(hw);
-		if (ret < 0) {
-			if (eecd & E1000_EECD_I210_FLASH_DETECTED)
-				del_mtd_device(&hw->mtd);
+	if (eecd & E1000_EECD_I210_FLASH_DETECTED) {
+		hw->mtd.parent = hw->dev;
+		hw->mtd.read = e1000_mtd_read;
+		hw->mtd.write = e1000_mtd_write;
+		hw->mtd.erase = e1000_mtd_erase;
+		hw->mtd.lock = e1000_mtd_lock;
+		hw->mtd.unlock = e1000_mtd_unlock;
+		hw->mtd.size = eeprom->word_size * 2;
+		hw->mtd.writesize = 1;
+		hw->mtd.subpage_sft = 0;
+
+		hw->mtd.eraseregions = xzalloc(sizeof(struct mtd_erase_region_info));
+		hw->mtd.erasesize = SZ_4K;
+		hw->mtd.eraseregions[0].erasesize = SZ_4K;
+		hw->mtd.eraseregions[0].numblocks = hw->mtd.size / SZ_4K;
+		hw->mtd.numeraseregions = 1;
+
+		hw->mtd.flags = MTD_CAP_NORFLASH;
+		hw->mtd.type = MTD_NORFLASH;
+
+		ret = add_mtd_device(&hw->mtd, "e1000-nor",
+				     DEVICE_ID_DYNAMIC);
+		if (ret) {
 			devfs_remove(&hw->eepromcdev);
+			return ret;
 		}
 	}
 
+	ret = e1000_register_invm(hw);
+	if (ret < 0) {
+		if (eecd & E1000_EECD_I210_FLASH_DETECTED)
+			del_mtd_device(&hw->mtd);
+		devfs_remove(&hw->eepromcdev);
+	}
+
 	return ret;
 }
-- 
2.15.1


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

[PATCH v3 2/5] net/e1000: use correct bit for flash detection

[Lucas Stach]

Fixes: 95c346ccaa6d (net/e1000: don't access the (simulated)
                     eeprom when it is invalid)
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/e1000/e1000.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 1558b3c7f5c7..4a1a1aa33694 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -794,7 +794,7 @@ struct e1000_eeprom_info {
 #ifndef E1000_EEPROM_GRANT_ATTEMPTS
 #define E1000_EEPROM_GRANT_ATTEMPTS 1000 /* EEPROM # attempts to gain grant */
 #endif
-#define E1000_EECD_FLASH_IN_USE     0x00000100  /* Flash is present with a valid signature */
+#define E1000_EECD_FLASH_IN_USE     0x00000040  /* Flash is present with a valid signature */
 #define E1000_EECD_EE_PRES          0x00000100
 #define E1000_EECD_AUTO_RD          0x00000200  /* EEPROM Auto Read done */
 #define E1000_EECD_SIZE_EX_MASK     0x00007800  /* EEprom Size */
-- 
2.15.1


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

[PATCH v3 3/5] net/e1000: EEPROM isn't valid if only iNVM is available

[Lucas Stach]

There is no point in registering the emulated EEPROM device if only the
iNVM is available with no external flash attached to the i210, as in
practice it's only shadowing the iNVM.

When the EEPROM is populated from iNVM, the signature is not valid, which
causes other parts of the driver to fall over. To fix this just ignore
the EEPROM in that case.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/e1000/eeprom.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/e1000/eeprom.c b/drivers/net/e1000/eeprom.c
index ddac7de8a0e9..0a28dfdd8868 100644
--- a/drivers/net/e1000/eeprom.c
+++ b/drivers/net/e1000/eeprom.c
@@ -1525,20 +1525,18 @@ int e1000_register_invm(struct e1000_hw *hw)
 
 int e1000_eeprom_valid(struct e1000_hw *hw)
 {
-	uint32_t eecd;
+	uint32_t valid_mask = E1000_EECD_FLASH_IN_USE |
+			      E1000_EECD_AUTO_RD | E1000_EECD_EE_PRES;
 
 	if (hw->mac_type != e1000_igb)
 		return 1;
 
 	/*
-	 * if AUTO_RD or EE_PRES are not set in EECD, the shadow RAM is invalid
-	 * (and in practise seems to contain the contents of iNVM).
+	 * If there is no flash in use or AUTO_RD or EE_PRES are not set in
+	 * EECD, the shadow RAM is invalid (and in practise seems to contain
+	 * the contents of iNVM).
 	 */
-	eecd = e1000_read_reg(hw, E1000_EECD);
-	if (!(eecd & E1000_EECD_AUTO_RD))
-		return 0;
-
-	if (!(eecd & E1000_EECD_EE_PRES))
+	if ((e1000_read_reg(hw, E1000_EECD) & valid_mask) != valid_mask)
 		return 0;
 
 	return 1;
-- 
2.15.1


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

[PATCH v3 4/5] net/e1000: don't check EEPROM signature if populated from iNVM

[Lucas Stach]

The EEPROM device will contain an invalid signature if it has been
populated from iNVM. Since the iNVM enum type has been removed, the
only way to tell if a signature check makes sense is to look at the
EEPROM valid status.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/e1000/eeprom.c | 10 ++++------
 drivers/net/e1000/main.c   |  2 +-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/e1000/eeprom.c b/drivers/net/e1000/eeprom.c
index 0a28dfdd8868..b0408107bbdd 100644
--- a/drivers/net/e1000/eeprom.c
+++ b/drivers/net/e1000/eeprom.c
@@ -1006,12 +1006,10 @@ int e1000_validate_eeprom_checksum(struct e1000_hw *hw)
 	DEBUGFUNC();
 
 	/*
-	  Only the following three 'types' of EEPROM can be expected
-	  to have correct EEPROM checksum
-	*/
-	if (hw->eeprom.type != e1000_eeprom_spi &&
-	    hw->eeprom.type != e1000_eeprom_microwire &&
-	    hw->eeprom.type != e1000_eeprom_flash)
+	 * If the EEPROM device content isn't valid there is no point in
+	 * checking the signature.
+	 */
+	if (!e1000_eeprom_valid(hw))
 		return 0;
 
 	/* Read the EEPROM */
diff --git a/drivers/net/e1000/main.c b/drivers/net/e1000/main.c
index 0139c4a6d758..bb6ab4eb0360 100644
--- a/drivers/net/e1000/main.c
+++ b/drivers/net/e1000/main.c
@@ -3597,7 +3597,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		}
 	}
 
-	if (!e1000_eeprom_valid(hw) || e1000_validate_eeprom_checksum(hw))
+	if (e1000_validate_eeprom_checksum(hw))
 		return 0;
 
 	e1000_get_ethaddr(edev, edev->ethaddr);
-- 
2.15.1


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

[PATCH v3 5/5] net/e1000: don't register EERPOM device if the content is invalid

[Lucas Stach]

If the EEPROM content isn't valid, there is no point in registering the
EEPROM device, as it will reject any read attempt anyway.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/e1000/eeprom.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/net/e1000/eeprom.c b/drivers/net/e1000/eeprom.c
index b0408107bbdd..319910103eee 100644
--- a/drivers/net/e1000/eeprom.c
+++ b/drivers/net/e1000/eeprom.c
@@ -1556,15 +1556,6 @@ int e1000_register_eeprom(struct e1000_hw *hw)
 
 	eecd = e1000_read_reg(hw, E1000_EECD);
 
-	hw->eepromcdev.dev = hw->dev;
-	hw->eepromcdev.ops = &e1000_eeprom_ops;
-	hw->eepromcdev.name = xasprintf("e1000-eeprom%d", hw->dev->id);
-	hw->eepromcdev.size = 0x1000;
-
-	ret = devfs_create(&hw->eepromcdev);
-	if (ret < 0)
-		return ret;
-
 	if (eecd & E1000_EECD_AUTO_RD) {
 		if (eecd & E1000_EECD_EE_PRES) {
 			if (eecd & E1000_EECD_FLASH_IN_USE) {
@@ -1587,6 +1578,18 @@ int e1000_register_eeprom(struct e1000_hw *hw)
 		dev_err(hw->dev, "Flash Auto-Read not done\n");
 	}
 
+	if (e1000_eeprom_valid(hw)) {
+		hw->eepromcdev.dev = hw->dev;
+		hw->eepromcdev.ops = &e1000_eeprom_ops;
+		hw->eepromcdev.name = xasprintf("e1000-eeprom%d",
+						hw->dev->id);
+		hw->eepromcdev.size = 0x1000;
+
+		ret = devfs_create(&hw->eepromcdev);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (eecd & E1000_EECD_I210_FLASH_DETECTED) {
 		hw->mtd.parent = hw->dev;
 		hw->mtd.read = e1000_mtd_read;
@@ -1609,18 +1612,22 @@ int e1000_register_eeprom(struct e1000_hw *hw)
 
 		ret = add_mtd_device(&hw->mtd, "e1000-nor",
 				     DEVICE_ID_DYNAMIC);
-		if (ret) {
-			devfs_remove(&hw->eepromcdev);
-			return ret;
-		}
+		if (ret)
+			goto out_eeprom;
 	}
 
 	ret = e1000_register_invm(hw);
-	if (ret < 0) {
-		if (eecd & E1000_EECD_I210_FLASH_DETECTED)
-			del_mtd_device(&hw->mtd);
+	if (ret < 0)
+		goto out_mtd;
+
+	return E1000_SUCCESS;
+
+out_mtd:
+	if (eecd & E1000_EECD_I210_FLASH_DETECTED)
+		del_mtd_device(&hw->mtd);
+out_eeprom:
+	if (e1000_eeprom_valid(hw))
 		devfs_remove(&hw->eepromcdev);
-	}
 
 	return ret;
 }
-- 
2.15.1


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

Re: [PATCH v3 1/5] net/e1000: remove one level of indentation

[Sascha Hauer]

On Fri, Jan 26, 2018 at 02:32:28PM +0100, Lucas Stach wrote:
> By returning early if the MAC type isn't e1000_igb.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/net/e1000/eeprom.c | 121 +++++++++++++++++++++++----------------------
>  1 file changed, 61 insertions(+), 60 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/drivers/net/e1000/eeprom.c b/drivers/net/e1000/eeprom.c
> index 748d8afe7922..ddac7de8a0e9 100644
> --- a/drivers/net/e1000/eeprom.c
> +++ b/drivers/net/e1000/eeprom.c
> @@ -1551,79 +1551,80 @@ int e1000_eeprom_valid(struct e1000_hw *hw)
>   */
>  int e1000_register_eeprom(struct e1000_hw *hw)
>  {
> -	int ret = E1000_SUCCESS;
> -
>  	struct e1000_eeprom_info *eeprom = &hw->eeprom;
> +	uint32_t eecd;
> +	int ret;
>  
> -	if (hw->mac_type == e1000_igb) {
> -		uint32_t eecd = e1000_read_reg(hw, E1000_EECD);
> +	if (hw->mac_type != e1000_igb)
> +		return E1000_SUCCESS;
>  
> -		hw->eepromcdev.dev = hw->dev;
> -		hw->eepromcdev.ops = &e1000_eeprom_ops;
> -		hw->eepromcdev.name = xasprintf("e1000-eeprom%d", hw->dev->id);
> -		hw->eepromcdev.size = 0x1000;
> +	eecd = e1000_read_reg(hw, E1000_EECD);
>  
> -		ret = devfs_create(&hw->eepromcdev);
> -		if (ret < 0)
> -			return ret;
> +	hw->eepromcdev.dev = hw->dev;
> +	hw->eepromcdev.ops = &e1000_eeprom_ops;
> +	hw->eepromcdev.name = xasprintf("e1000-eeprom%d", hw->dev->id);
> +	hw->eepromcdev.size = 0x1000;
> +
> +	ret = devfs_create(&hw->eepromcdev);
> +	if (ret < 0)
> +		return ret;
>  
> -		if (eecd & E1000_EECD_AUTO_RD) {
> -			if (eecd & E1000_EECD_EE_PRES) {
> -				if (eecd & E1000_EECD_FLASH_IN_USE) {
> -					uint32_t fla = e1000_read_reg(hw, E1000_FLA);
> -					dev_info(hw->dev,
> -						 "Hardware programmed from flash (%ssecure)\n",
> -						 fla & E1000_FLA_LOCKED ? "" : "un");
> -				} else {
> -					dev_info(hw->dev, "Hardware programmed from iNVM\n");
> -				}
> +	if (eecd & E1000_EECD_AUTO_RD) {
> +		if (eecd & E1000_EECD_EE_PRES) {
> +			if (eecd & E1000_EECD_FLASH_IN_USE) {
> +				uint32_t fla = e1000_read_reg(hw, E1000_FLA);
> +				dev_info(hw->dev,
> +					 "Hardware programmed from flash (%ssecure)\n",
> +					 fla & E1000_FLA_LOCKED ? "" : "un");
>  			} else {
> -				dev_warn(hw->dev, "Shadow RAM invalid\n");
> +				dev_info(hw->dev, "Hardware programmed from iNVM\n");
>  			}
>  		} else {
> -			/*
> -			 * I never saw this case in practise and I'm unsure how
> -			 * to handle that. Maybe just wait until the hardware is
> -			 * up enough that this bit is set?
> -			 */
> -			dev_err(hw->dev, "Flash Auto-Read not done\n");
> -		}
> -
> -		if (eecd & E1000_EECD_I210_FLASH_DETECTED) {
> -			hw->mtd.parent = hw->dev;
> -			hw->mtd.read = e1000_mtd_read;
> -			hw->mtd.write = e1000_mtd_write;
> -			hw->mtd.erase = e1000_mtd_erase;
> -			hw->mtd.lock = e1000_mtd_lock;
> -			hw->mtd.unlock = e1000_mtd_unlock;
> -			hw->mtd.size = eeprom->word_size * 2;
> -			hw->mtd.writesize = 1;
> -			hw->mtd.subpage_sft = 0;
> -
> -			hw->mtd.eraseregions = xzalloc(sizeof(struct mtd_erase_region_info));
> -			hw->mtd.erasesize = SZ_4K;
> -			hw->mtd.eraseregions[0].erasesize = SZ_4K;
> -			hw->mtd.eraseregions[0].numblocks = hw->mtd.size / SZ_4K;
> -			hw->mtd.numeraseregions = 1;
> -
> -			hw->mtd.flags = MTD_CAP_NORFLASH;
> -			hw->mtd.type = MTD_NORFLASH;
> -
> -			ret = add_mtd_device(&hw->mtd, "e1000-nor",
> -					     DEVICE_ID_DYNAMIC);
> -			if (ret) {
> -				devfs_remove(&hw->eepromcdev);
> -				return ret;
> -			}
> +			dev_warn(hw->dev, "Shadow RAM invalid\n");
>  		}
> +	} else {
> +		/*
> +		 * I never saw this case in practise and I'm unsure how
> +		 * to handle that. Maybe just wait until the hardware is
> +		 * up enough that this bit is set?
> +		 */
> +		dev_err(hw->dev, "Flash Auto-Read not done\n");
> +	}
>  
> -		ret = e1000_register_invm(hw);
> -		if (ret < 0) {
> -			if (eecd & E1000_EECD_I210_FLASH_DETECTED)
> -				del_mtd_device(&hw->mtd);
> +	if (eecd & E1000_EECD_I210_FLASH_DETECTED) {
> +		hw->mtd.parent = hw->dev;
> +		hw->mtd.read = e1000_mtd_read;
> +		hw->mtd.write = e1000_mtd_write;
> +		hw->mtd.erase = e1000_mtd_erase;
> +		hw->mtd.lock = e1000_mtd_lock;
> +		hw->mtd.unlock = e1000_mtd_unlock;
> +		hw->mtd.size = eeprom->word_size * 2;
> +		hw->mtd.writesize = 1;
> +		hw->mtd.subpage_sft = 0;
> +
> +		hw->mtd.eraseregions = xzalloc(sizeof(struct mtd_erase_region_info));
> +		hw->mtd.erasesize = SZ_4K;
> +		hw->mtd.eraseregions[0].erasesize = SZ_4K;
> +		hw->mtd.eraseregions[0].numblocks = hw->mtd.size / SZ_4K;
> +		hw->mtd.numeraseregions = 1;
> +
> +		hw->mtd.flags = MTD_CAP_NORFLASH;
> +		hw->mtd.type = MTD_NORFLASH;
> +
> +		ret = add_mtd_device(&hw->mtd, "e1000-nor",
> +				     DEVICE_ID_DYNAMIC);
> +		if (ret) {
>  			devfs_remove(&hw->eepromcdev);
> +			return ret;
>  		}
>  	}
>  
> +	ret = e1000_register_invm(hw);
> +	if (ret < 0) {
> +		if (eecd & E1000_EECD_I210_FLASH_DETECTED)
> +			del_mtd_device(&hw->mtd);
> +		devfs_remove(&hw->eepromcdev);
> +	}
> +
>  	return ret;
>  }
> -- 
> 2.15.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