[PATCH] tg3: fix return value check in tg3_read_vpd()

[PATCH] tg3: fix return value check in tg3_read_vpd()

[David Sterba]

CC: Matt Carlson <mcarlson@broadcom.com>
CC: Michael Chan <mchan@broadcom.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
 drivers/net/tg3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 30ccbb6..6f97b7b 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12658,7 +12658,7 @@ static void __devinit tg3_read_vpd(struct tg3 *tp)
 			cnt = pci_read_vpd(tp->pdev, pos,
 					   TG3_NVM_VPD_LEN - pos,
 					   &vpd_data[pos]);
-			if (cnt == -ETIMEDOUT || -EINTR)
+			if (cnt == -ETIMEDOUT || cnt == -EINTR)
 				cnt = 0;
 			else if (cnt < 0)
 				goto out_not_found;
-- 
1.7.3.4.626.g73e7b

Re: [PATCH] tg3: fix return value check in tg3_read_vpd()

[Dan Carpenter]

On Mon, Dec 27, 2010 at 06:31:34PM +0100, David Sterba wrote:
> CC: Matt Carlson <mcarlson@broadcom.com>
> CC: Michael Chan <mchan@broadcom.com>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---

Your fix is obviously correct, but could you describe the symptoms
in your changelog instead of leaving it blank?  In the original code,
negative error values are ignored so we never goto out_not_found.  Can
pci_read_vpd() return any errors we care about besides -ENODEV?  What I
mean is, did you find this through analysing the code or did it cause a
bug at runtime?

regards,
dan carpenter

Re: [PATCH] tg3: fix return value check in tg3_read_vpd()

[David Sterba]

Hi,

On Monday 27 December 2010 21:19:27 Dan Carpenter wrote:
> Your fix is obviously correct, but could you describe the symptoms
> in your changelog instead of leaving it blank?  In the original code,
> negative error values are ignored so we never goto out_not_found.  Can
> pci_read_vpd() return any errors we care about besides -ENODEV?  What I
> mean is, did you find this through analysing the code or did it cause a
> bug at runtime?

Although pci_read_vpd returns ENODEV directly, there is a call to vpd->read
function which may return other error values and is eg. set to
pci_vpd_pci22_read() :

drivers/pci/access.c::pci_vpd_pci22_read()
  may return -EINVAL or -EINTR directly
  or -ETIMEDOUT or -EINTR via pci_vpd_pci22_wait()

Yes, other negative values besides ETIMEDOUT and EINTR are ignored and after
3 attempts the pos != TG3_NVM_VPD_LEN check goes outwards. The fixed version
allows to jump out immediately. So this does not manifest itself as a 
misbehaviour at runtime. It was found by code analysis.

I'll post updated patch.

dave

[PATCH v2] tg3: fix return value check in tg3_read_vpd()

[David Sterba]

Besides -ETIMEDOUT and -EINTR, pci_read_vpd may return other error
values like -ENODEV or -EINVAL which are ignored due to the buggy
check, but the data are not read from VPD anyway and this is checked
subsequently with at most 3 needless loop iterations. This does not
show up as a runtime bug.

CC: Matt Carlson <mcarlson@broadcom.com>
CC: Michael Chan <mchan@broadcom.com>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
 drivers/net/tg3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 30ccbb6..6f97b7b 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12658,7 +12658,7 @@ static void __devinit tg3_read_vpd(struct tg3 *tp)
 			cnt = pci_read_vpd(tp->pdev, pos,
 					   TG3_NVM_VPD_LEN - pos,
 					   &vpd_data[pos]);
-			if (cnt == -ETIMEDOUT || -EINTR)
+			if (cnt == -ETIMEDOUT || cnt == -EINTR)
 				cnt = 0;
 			else if (cnt < 0)
 				goto out_not_found;
-- 
1.7.4.rc0

Re: [PATCH v2] tg3: fix return value check in tg3_read_vpd()

[David Miller]

From: David Sterba <dsterba@suse.cz>
Date: Wed, 29 Dec 2010 14:40:31 +0100

> Besides -ETIMEDOUT and -EINTR, pci_read_vpd may return other error
> values like -ENODEV or -EINVAL which are ignored due to the buggy
> check, but the data are not read from VPD anyway and this is checked
> subsequently with at most 3 needless loop iterations. This does not
> show up as a runtime bug.
> 
> CC: Matt Carlson <mcarlson@broadcom.com>
> CC: Michael Chan <mchan@broadcom.com>
> Signed-off-by: David Sterba <dsterba@suse.cz>

Applied, thanks.