Message ID | 4999704A.9030004@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 16-02-2009 14:55, Roel Kluin wrote: > Only half the timeout was waited, due to the double increment, and an error > occurred one too early. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > --- > diff --git a/drivers/net/netxen/netxen_nic_niu.c b/drivers/net/netxen/netxen_nic_niu.c > index c3b9c83..df3aa08 100644 > --- a/drivers/net/netxen/netxen_nic_niu.c > +++ b/drivers/net/netxen/netxen_nic_niu.c > @@ -147,12 +147,11 @@ int netxen_niu_gbe_phy_read(struct netxen_adapter *adapter, long reg, > NETXEN_NIU_GB_MII_MGMT_INDICATE(0), > &status, 4)) > return -EIO; > - timeout++; > } while ((netxen_get_gb_mii_mgmt_busy(status) > || netxen_get_gb_mii_mgmt_notvalid(status)) > && (timeout++ < NETXEN_NIU_PHY_WAITMAX)); > > - if (timeout < NETXEN_NIU_PHY_WAITMAX) { > + if (timeout <= NETXEN_NIU_PHY_WAITMAX) { Roel, very good caches, but IMHO in all such cases changing to ++timeout with 'if (timeout == NETXEN_NIU_PHY_WAITMAX)' where possible, should be really more readable. At least wrt. next similar patches. Thanks, Jarek P. > if (adapter->hw_read_wx(adapter, > NETXEN_NIU_GB_MII_MGMT_STATUS(0), > readval, 4)) > @@ -240,11 +239,10 @@ int netxen_niu_gbe_phy_write(struct netxen_adapter *adapter, long reg, > NETXEN_NIU_GB_MII_MGMT_INDICATE(0), > &status, 4)) > return -EIO; > - timeout++; > } while ((netxen_get_gb_mii_mgmt_busy(status)) > && (timeout++ < NETXEN_NIU_PHY_WAITMAX)); > > - if (timeout < NETXEN_NIU_PHY_WAITMAX) > + if (timeout <= NETXEN_NIU_PHY_WAITMAX) > result = 0; > else > result = -EIO; -- 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 Tue, Feb 17, 2009 at 07:33:12AM +0000, Jarek Poplawski wrote: > On 16-02-2009 14:55, Roel Kluin wrote: > > Only half the timeout was waited, due to the double increment, and an error > > occurred one too early. > > > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> > > --- > > diff --git a/drivers/net/netxen/netxen_nic_niu.c b/drivers/net/netxen/netxen_nic_niu.c > > index c3b9c83..df3aa08 100644 > > --- a/drivers/net/netxen/netxen_nic_niu.c > > +++ b/drivers/net/netxen/netxen_nic_niu.c > > @@ -147,12 +147,11 @@ int netxen_niu_gbe_phy_read(struct netxen_adapter *adapter, long reg, > > NETXEN_NIU_GB_MII_MGMT_INDICATE(0), > > &status, 4)) > > return -EIO; > > - timeout++; > > } while ((netxen_get_gb_mii_mgmt_busy(status) > > || netxen_get_gb_mii_mgmt_notvalid(status)) > > && (timeout++ < NETXEN_NIU_PHY_WAITMAX)); > > > > - if (timeout < NETXEN_NIU_PHY_WAITMAX) { > > + if (timeout <= NETXEN_NIU_PHY_WAITMAX) { > > Roel, very good caches, but IMHO in all such cases changing to > ++timeout with 'if (timeout == NETXEN_NIU_PHY_WAITMAX)' where Hmm... of course 'if (timeout == NETXEN_NIU_PHY_WAITMAX)' for errcode; in this particular case: 'if (timeout < NETXEN_NIU_PHY_WAITMAX)' would be OK. Jarek P. > possible, should be really more readable. At least wrt. next similar > patches. > > Thanks, > Jarek P. > > > if (adapter->hw_read_wx(adapter, > > NETXEN_NIU_GB_MII_MGMT_STATUS(0), > > readval, 4)) > > @@ -240,11 +239,10 @@ int netxen_niu_gbe_phy_write(struct netxen_adapter *adapter, long reg, > > NETXEN_NIU_GB_MII_MGMT_INDICATE(0), > > &status, 4)) > > return -EIO; > > - timeout++; > > } while ((netxen_get_gb_mii_mgmt_busy(status)) > > && (timeout++ < NETXEN_NIU_PHY_WAITMAX)); > > > > - if (timeout < NETXEN_NIU_PHY_WAITMAX) > > + if (timeout <= NETXEN_NIU_PHY_WAITMAX) > > result = 0; > > else > > result = -EIO; -- 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/netxen/netxen_nic_niu.c b/drivers/net/netxen/netxen_nic_niu.c index c3b9c83..df3aa08 100644 --- a/drivers/net/netxen/netxen_nic_niu.c +++ b/drivers/net/netxen/netxen_nic_niu.c @@ -147,12 +147,11 @@ int netxen_niu_gbe_phy_read(struct netxen_adapter *adapter, long reg, NETXEN_NIU_GB_MII_MGMT_INDICATE(0), &status, 4)) return -EIO; - timeout++; } while ((netxen_get_gb_mii_mgmt_busy(status) || netxen_get_gb_mii_mgmt_notvalid(status)) && (timeout++ < NETXEN_NIU_PHY_WAITMAX)); - if (timeout < NETXEN_NIU_PHY_WAITMAX) { + if (timeout <= NETXEN_NIU_PHY_WAITMAX) { if (adapter->hw_read_wx(adapter, NETXEN_NIU_GB_MII_MGMT_STATUS(0), readval, 4)) @@ -240,11 +239,10 @@ int netxen_niu_gbe_phy_write(struct netxen_adapter *adapter, long reg, NETXEN_NIU_GB_MII_MGMT_INDICATE(0), &status, 4)) return -EIO; - timeout++; } while ((netxen_get_gb_mii_mgmt_busy(status)) && (timeout++ < NETXEN_NIU_PHY_WAITMAX)); - if (timeout < NETXEN_NIU_PHY_WAITMAX) + if (timeout <= NETXEN_NIU_PHY_WAITMAX) result = 0; else result = -EIO;
Only half the timeout was waited, due to the double increment, and an error occurred one too early. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- -- 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