| Message ID | f254bc583cbe1f623bd6ebb4ff375e440696d8b2.1363703896.git.jbenc@redhat.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On 3/19/13 7:42 AM, "Jiri Benc" <jbenc@redhat.com> wrote: >For 82576 MAC type, max_adj is reported as 1000000000 ppb. However, if >this value is passed to igb_ptp_adjfreq_82576, incvalue overflows out of >INCVALUE_82576_MASK, resulting in setting of zero TIMINCA.incvalue, >stopping >the PHC (instead of going at twice the nominal speed). > >Fix the advertised max_adj value to the largest value hardware can handle. >As there is no min_adj value available (-max_adj is used instead), this >will >also prevent stopping the clock intentionally. It's probably not a big >deal, >other igb MAC types don't support stopping the clock, either. > >Signed-off-by: Jiri Benc <jbenc@redhat.com> >--- > drivers/net/ethernet/intel/igb/igb_ptp.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > >diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c >b/drivers/net/ethernet/intel/igb/igb_ptp.c >index 0987822..0a23750 100644 >--- a/drivers/net/ethernet/intel/igb/igb_ptp.c >+++ b/drivers/net/ethernet/intel/igb/igb_ptp.c >@@ -740,7 +740,7 @@ void igb_ptp_init(struct igb_adapter *adapter) > case e1000_82576: > snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); > adapter->ptp_caps.owner = THIS_MODULE; >- adapter->ptp_caps.max_adj = 1000000000; >+ adapter->ptp_caps.max_adj = 999999881; > adapter->ptp_caps.n_ext_ts = 0; > adapter->ptp_caps.pps = 0; > adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576; >-- >1.7.6.5 Good catch on this, Jiri! I know the math works out the same, but I'd prefer it if you changed the max_adj value to 999999999, since that is technically what we can accept before we have any issues. If you re-submit with this change, I'll add my ACK and we can run it through our internal testing. Thanks! Matthew Matthew Vick Linux Development Networking Division Intel Corporation -- 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, 2013-03-19 at 15:42 +0100, Jiri Benc wrote: > For 82576 MAC type, max_adj is reported as 1000000000 ppb. However, if > this value is passed to igb_ptp_adjfreq_82576, incvalue overflows out > of > INCVALUE_82576_MASK, resulting in setting of zero TIMINCA.incvalue, > stopping > the PHC (instead of going at twice the nominal speed). > > Fix the advertised max_adj value to the largest value hardware can > handle. > As there is no min_adj value available (-max_adj is used instead), > this will > also prevent stopping the clock intentionally. It's probably not a big > deal, > other igb MAC types don't support stopping the clock, either. > > Signed-off-by: Jiri Benc <jbenc@redhat.com> > --- > drivers/net/ethernet/intel/igb/igb_ptp.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) Thanks Jiri, I have added the patch to my igb queue
On Tue, 19 Mar 2013 21:17:25 +0000, Vick, Matthew wrote: > Good catch on this, Jiri! I know the math works out the same, but I'd > prefer it if you changed the max_adj value to 999999999, since that is > technically what we can accept before we have any issues. If you re-submit > with this change, I'll add my ACK and we can run it through our internal > testing. Thanks! But the real maximum value is actually 999999881, as anything higher than that would be capped to 999999881 by the driver. I don't think the driver should advertise higher max_adj than it is able to fulfill, otherwise there would be no need for the field. Jiri
On 3/20/13 12:11 PM, "Jiri Benc" <jbenc@redhat.com> wrote: >On Tue, 19 Mar 2013 21:17:25 +0000, Vick, Matthew wrote: >> Good catch on this, Jiri! I know the math works out the same, but I'd >> prefer it if you changed the max_adj value to 999999999, since that is >> technically what we can accept before we have any issues. If you >>re-submit >> with this change, I'll add my ACK and we can run it through our internal >> testing. Thanks! > >But the real maximum value is actually 999999881, as anything higher >than that would be capped to 999999881 by the driver. I don't think the >driver should advertise higher max_adj than it is able to fulfill, >otherwise there would be no need for the field. I prefer 999999999 as it's something that looks slightly less "magic number"-y (plus looks like the other devices in igb) and is still technically something that can be passed down without error. Ultimately not a big deal and I can understand your argument, so I'm okay putting my personal preference aside on this one. -- 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, Mar 19, 2013 at 05:49:12PM -0700, Jeff Kirsher wrote: > > Thanks Jiri, I have added the patch to my igb queue Can this patch also go into stable, since v3.5, please? Thanks, Richard -- 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/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index 0987822..0a23750 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -740,7 +740,7 @@ void igb_ptp_init(struct igb_adapter *adapter) case e1000_82576: snprintf(adapter->ptp_caps.name, 16, "%pm", netdev->dev_addr); adapter->ptp_caps.owner = THIS_MODULE; - adapter->ptp_caps.max_adj = 1000000000; + adapter->ptp_caps.max_adj = 999999881; adapter->ptp_caps.n_ext_ts = 0; adapter->ptp_caps.pps = 0; adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82576;
For 82576 MAC type, max_adj is reported as 1000000000 ppb. However, if this value is passed to igb_ptp_adjfreq_82576, incvalue overflows out of INCVALUE_82576_MASK, resulting in setting of zero TIMINCA.incvalue, stopping the PHC (instead of going at twice the nominal speed). Fix the advertised max_adj value to the largest value hardware can handle. As there is no min_adj value available (-max_adj is used instead), this will also prevent stopping the clock intentionally. It's probably not a big deal, other igb MAC types don't support stopping the clock, either. Signed-off-by: Jiri Benc <jbenc@redhat.com> --- drivers/net/ethernet/intel/igb/igb_ptp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)