I am not sure if 0 or ~0 would be a better choice in the gm_phy_read()
error case. I used 0. A more complete solution might be to plumb up
error handling to the callers of gm_phy_read().
==
From 37486219a3d93881f3b2619a4b2bb21be62db7d4 Mon Sep 17 00:00:00 2001
From: Greg Thelen <redacted>
Date: Mon, 13 Jun 2011 14:09:07 -0700
Subject: [PATCH] sky2: avoid using uninitialized variable
Prior to this change gm_phy_read() could return an uninitialized
variable if __gm_phy_read() failed.
This change returns zero in the failure case.
Signed-off-by: Greg Thelen <redacted>
---
drivers/net/sky2.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
From: Stephen Hemminger <hidden> Date: 2011-06-13 22:12:14
On Mon, 13 Jun 2011 14:21:59 -0700
Greg Thelen [off-list ref] wrote:
I am not sure if 0 or ~0 would be a better choice in the gm_phy_read()
error case. I used 0. A more complete solution might be to plumb up
error handling to the callers of gm_phy_read().
==
From 37486219a3d93881f3b2619a4b2bb21be62db7d4 Mon Sep 17 00:00:00 2001
From: Greg Thelen <redacted>
Date: Mon, 13 Jun 2011 14:09:07 -0700
Subject: [PATCH] sky2: avoid using uninitialized variable
Prior to this change gm_phy_read() could return an uninitialized
variable if __gm_phy_read() failed.
This change returns zero in the failure case.
Signed-off-by: Greg Thelen <redacted>
Shouldn't the callers be changed to check rather than just returning
0 and masking the problem.
On Mon, Jun 13, 2011 at 3:12 PM, Stephen Hemminger
[off-list ref] wrote:
On Mon, 13 Jun 2011 14:21:59 -0700
Greg Thelen [off-list ref] wrote:
quoted
I am not sure if 0 or ~0 would be a better choice in the gm_phy_read()
error case. I used 0. A more complete solution might be to plumb up
error handling to the callers of gm_phy_read().
==
From 37486219a3d93881f3b2619a4b2bb21be62db7d4 Mon Sep 17 00:00:00 2001
From: Greg Thelen <redacted>
Date: Mon, 13 Jun 2011 14:09:07 -0700
Subject: [PATCH] sky2: avoid using uninitialized variable
Prior to this change gm_phy_read() could return an uninitialized
variable if __gm_phy_read() failed.
This change returns zero in the failure case.
Signed-off-by: Greg Thelen <redacted>
Shouldn't the callers be changed to check rather than just returning
0 and masking the problem.
I agree that the right long term solution is to plumb the error
handling up through the callers. This would involve deleting the
error-free gm_phy_read() routine and replacing all calls to it with
calls to error-capable __gm_phy_read(). This would require converting
several routines from returning void to returning int allowing errors
to propagate upwards. Notable affected routines include:
sky2_phy_power_up(), sky2_wol_init(), sky2_phy_power_down(),
sky2_hw_down(), sky2_mac_init(), sky2_hw_up(), sky2_phy_intr(),
sky2_led(). sky2_restart() would likely have to re-queue the work
item in the case of failure. Presumably sky2_poll() would return 0 if
error is seen. On a related note, it also seems that gm_phy_write()
callers should be checking its return value to also handle the same
class of I/O errors. Testing these changes would involve injecting
error values or access to certain kinds of broken hardware.
For the short term I figured that not consuming random data was a step
in right direction. But I understand if you prefer to hold out for
the complete solution. Unfortunately, I do not currently have time to
contribute to the complete solution.
From: Stephen Hemminger <hidden> Date: 2011-06-14 04:02:32
On Mon, 13 Jun 2011 17:34:00 -0700
Greg Thelen [off-list ref] wrote:
On Mon, Jun 13, 2011 at 3:12 PM, Stephen Hemminger
[off-list ref] wrote:
quoted
On Mon, 13 Jun 2011 14:21:59 -0700
Greg Thelen [off-list ref] wrote:
quoted
I am not sure if 0 or ~0 would be a better choice in the gm_phy_read()
error case. I used 0. A more complete solution might be to plumb up
error handling to the callers of gm_phy_read().
==
From 37486219a3d93881f3b2619a4b2bb21be62db7d4 Mon Sep 17 00:00:00 2001
From: Greg Thelen <redacted>
Date: Mon, 13 Jun 2011 14:09:07 -0700
Subject: [PATCH] sky2: avoid using uninitialized variable
Prior to this change gm_phy_read() could return an uninitialized
variable if __gm_phy_read() failed.
This change returns zero in the failure case.
Signed-off-by: Greg Thelen <redacted>
Shouldn't the callers be changed to check rather than just returning
0 and masking the problem.
I agree that the right long term solution is to plumb the error
handling up through the callers. This would involve deleting the
error-free gm_phy_read() routine and replacing all calls to it with
calls to error-capable __gm_phy_read(). This would require converting
several routines from returning void to returning int allowing errors
to propagate upwards. Notable affected routines include:
sky2_phy_power_up(), sky2_wol_init(), sky2_phy_power_down(),
sky2_hw_down(), sky2_mac_init(), sky2_hw_up(), sky2_phy_intr(),
sky2_led(). sky2_restart() would likely have to re-queue the work
item in the case of failure. Presumably sky2_poll() would return 0 if
error is seen. On a related note, it also seems that gm_phy_write()
callers should be checking its return value to also handle the same
class of I/O errors. Testing these changes would involve injecting
error values or access to certain kinds of broken hardware.
In my experience if phy reads once successfully, it is going
to read every time. If there is a problem it only happens on
the first access (powered off, bad timing, etc).
For the short term I figured that not consuming random data was a step
in right direction. But I understand if you prefer to hold out for
the complete solution. Unfortunately, I do not currently have time to
contribute to the complete solution.
From: David Miller <davem@davemloft.net> Date: 2011-06-17 03:10:56
From: Stephen Hemminger <redacted>
Date: Tue, 14 Jun 2011 00:02:30 -0400
In my experience if phy reads once successfully, it is going
to read every time. If there is a problem it only happens on
the first access (powered off, bad timing, etc).
It also happens when the PHY can't get a response for a certain
register, for whatever reason, before internal hw timeouts trigger.
Please, check all MII accesses. That's what I do in every driver
I've written.
From: Ben Hutchings <hidden> Date: 2011-06-17 03:51:41
On Thu, 2011-06-16 at 23:10 -0400, David Miller wrote:
From: Stephen Hemminger <redacted>
Date: Tue, 14 Jun 2011 00:02:30 -0400
quoted
In my experience if phy reads once successfully, it is going
to read every time. If there is a problem it only happens on
the first access (powered off, bad timing, etc).
It also happens when the PHY can't get a response for a certain
register, for whatever reason, before internal hw timeouts trigger.
Please, check all MII accesses. That's what I do in every driver
I've written.
It doesn't help that the mii_if_info operations are defined to never
return errors. This doesn't prevent drivers from doing so internally,
but it does set a bad example.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: David Miller <davem@davemloft.net> Date: 2011-06-17 03:58:57
From: Ben Hutchings <redacted>
Date: Fri, 17 Jun 2011 04:51:35 +0100
On Thu, 2011-06-16 at 23:10 -0400, David Miller wrote:
quoted
From: Stephen Hemminger <redacted>
Date: Tue, 14 Jun 2011 00:02:30 -0400
quoted
In my experience if phy reads once successfully, it is going
to read every time. If there is a problem it only happens on
the first access (powered off, bad timing, etc).
It also happens when the PHY can't get a response for a certain
register, for whatever reason, before internal hw timeouts trigger.
Please, check all MII accesses. That's what I do in every driver
I've written.
It doesn't help that the mii_if_info operations are defined to never
return errors. This doesn't prevent drivers from doing so internally,
but it does set a bad example.