[PATCH] e1000: avoid potential deadlock in e1000_do_[read|write]_eeprom()

[PATCH] e1000: avoid potential deadlock in e1000_do_[read|write]_eeprom()

[Alexey Khoroshilov]

If eeprom->word_size is zero, e1000_do_[read|write]_eeprom() invoke
e1000_init_eeprom_params() to reinit eeprom params.
That is not a good idea since e1000_init_eeprom_params() calls
e1000_read_eeprom() if eeprom->type is e1000_eeprom_spi.
That means a deadlock on e1000_eeprom_lock.

At the same time it is unclear if the reinit is needed at all.
e1000_init_eeprom_params() is called from probe, so
it should succeed before any activities of the module start.

The patch suggests to remove the try to reinit eeprom params.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 drivers/net/ethernet/intel/e1000/e1000_hw.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index 2879b9631e15..a0317348ebde 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -3982,10 +3982,6 @@ static s32 e1000_do_read_eeprom(struct e1000_hw *hw, u16 offset, u16 words,
 		return E1000_SUCCESS;
 	}
 
-	/* If eeprom is not yet detected, do so now */
-	if (eeprom->word_size == 0)
-		e1000_init_eeprom_params(hw);
-
 	/* A check for invalid values:  offset too large, too many words, and
 	 * not enough words.
 	 */
@@ -4162,10 +4158,6 @@ static s32 e1000_do_write_eeprom(struct e1000_hw *hw, u16 offset, u16 words,
 		return E1000_SUCCESS;
 	}
 
-	/* If eeprom is not yet detected, do so now */
-	if (eeprom->word_size == 0)
-		e1000_init_eeprom_params(hw);
-
 	/* A check for invalid values:  offset too large, too many words, and
 	 * not enough words.
 	 */
-- 
1.8.3.2

Re: [PATCH] e1000: avoid potential deadlock in e1000_do_[read|write]_eeprom()

[Jeff Kirsher]

[-- Attachment #1.1: Type: text/plain, Size: 985 bytes --]

On Sat, 2013-12-21 at 02:25 +0400, Alexey Khoroshilov wrote:
> If eeprom->word_size is zero, e1000_do_[read|write]_eeprom() invoke
> e1000_init_eeprom_params() to reinit eeprom params.
> That is not a good idea since e1000_init_eeprom_params() calls
> e1000_read_eeprom() if eeprom->type is e1000_eeprom_spi.
> That means a deadlock on e1000_eeprom_lock.
> 
> At the same time it is unclear if the reinit is needed at all.
> e1000_init_eeprom_params() is called from probe, so
> it should succeed before any activities of the module start.
> 
> The patch suggests to remove the try to reinit eeprom params.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
>  drivers/net/ethernet/intel/e1000/e1000_hw.c | 8 --------
>  1 file changed, 8 deletions(-)

Sorry Alexey for the late reply, I thought I had responded to you
earlier.

I have added your patch to my queue.  Thanks!

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 455 bytes --]

------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: avoid potential deadlock in e1000_do_[read|write]_eeprom()

[Brown, Aaron F]

> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Alexey Khoroshilov
> Sent: Friday, December 20, 2013 2:25 PM
> To: Kirsher, Jeffrey T
> Cc: Alexey Khoroshilov; Brandeburg, Jesse; Allan, Bruce W; Wyborny,
> Carolyn; Skidmore, Donald C; Rose, Gregory V; Duyck, Alexander H; Ronciak,
> John; e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; ldv-project@linuxtesting.org
> Subject: [PATCH] e1000: avoid potential deadlock in
> e1000_do_[read|write]_eeprom()
> 
> If eeprom->word_size is zero, e1000_do_[read|write]_eeprom() invoke
> e1000_init_eeprom_params() to reinit eeprom params.
> That is not a good idea since e1000_init_eeprom_params() calls
> e1000_read_eeprom() if eeprom->type is e1000_eeprom_spi.
> That means a deadlock on e1000_eeprom_lock.
> 
> At the same time it is unclear if the reinit is needed at all.
> e1000_init_eeprom_params() is called from probe, so it should succeed
> before any activities of the module start.
> 
> The patch suggests to remove the try to reinit eeprom params.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>

Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
Tested by: Aaron Brown <aaron.f.brown@intel.com>

> ---
>  drivers/net/ethernet/intel/e1000/e1000_hw.c | 8 --------
>  1 file changed, 8 deletions(-)


------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today. 
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired