Patchwork tg3: fix return value check in tg3_read_vpd()

login
register
mail settings
Submitter David Sterba
Date Dec. 27, 2010, 5:31 p.m.
Message ID <1293471094-28976-1-git-send-email-dsterba@suse.cz>
Download mbox | patch
Permalink /patch/76797/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

David Sterba - Dec. 27, 2010, 5:31 p.m.
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(-)
Dan Carpenter - Dec. 27, 2010, 8:19 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba - Dec. 29, 2010, 1:17 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;