| Message ID | xr93vcw9s9zc.fsf@gthelen.mtv.corp.google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 13 Jun 2011 14:21:59 -0700 Greg Thelen <gthelen@google.com> 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 <gthelen@google.com> > 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 <gthelen@google.com> Shouldn't the callers be changed to check rather than just returning 0 and masking the problem. -- 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
On Mon, Jun 13, 2011 at 3:12 PM, Stephen Hemminger <shemminger@vyatta.com> wrote: > On Mon, 13 Jun 2011 14:21:59 -0700 > Greg Thelen <gthelen@google.com> 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 <gthelen@google.com> >> 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 <gthelen@google.com> > > 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. -- 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
On Mon, 13 Jun 2011 17:34:00 -0700 Greg Thelen <gthelen@google.com> wrote: > On Mon, Jun 13, 2011 at 3:12 PM, Stephen Hemminger > <shemminger@vyatta.com> wrote: > > On Mon, 13 Jun 2011 14:21:59 -0700 > > Greg Thelen <gthelen@google.com> 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 <gthelen@google.com> > >> 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 <gthelen@google.com> > > > > 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. -- 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
From: Stephen Hemminger <shemminger@vyatta.com> 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. -- 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
On Thu, 2011-06-16 at 23:10 -0400, David Miller wrote: > From: Stephen Hemminger <shemminger@vyatta.com> > 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. 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.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Fri, 17 Jun 2011 04:51:35 +0100 > On Thu, 2011-06-16 at 23:10 -0400, David Miller wrote: >> From: Stephen Hemminger <shemminger@vyatta.com> >> 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. > > 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. I totally agree. -- 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
diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c index 3ee41da..eba1ac4 100644 --- a/drivers/net/sky2.c +++ b/drivers/net/sky2.c @@ -206,7 +206,8 @@ io_error: static inline u16 gm_phy_read(struct sky2_hw *hw, unsigned port, u16 reg) { u16 v; - __gm_phy_read(hw, port, reg, &v); + if (__gm_phy_read(hw, port, reg, &v) < 0) + return 0; return v; }