| Message ID | e5b4ef7c56119e2d86e42f5680b1b82c9ec1a38e.1416265321.git.richardcochran@gmail.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2014-11-18 at 00:06 +0100, Richard Cochran wrote: > The i210 device can produce an interrupt on the full second. This > patch allows using this interrupt to generate an internal PPS event > for adjusting the kernel system time. > > Signed-off-by: Richard Cochran <richardcochran@gmail.com> > --- > drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++++ > drivers/net/ethernet/intel/igb/igb_ptp.c | 32 +++++++++++++++++++++++++++-- > 2 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index 7183a56..9412661 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter, > static void igb_tsync_interrupt(struct igb_adapter *adapter) > { > struct e1000_hw *hw = &adapter->hw; > + struct ptp_clock_event event; > u32 tsicr = rd32(E1000_TSICR); > > + if (tsicr & TSINTR_SYS_WRAP) { > + event.type = PTP_CLOCK_PPS; > + ptp_clock_event(adapter->ptp_clock, &event); > + } > + Nice! I hadn't thought of using the SYS_WRAP around interrupt for pps. Much simpler than having to setup a SDP pin trigger. Regards, Jake
On Tue, Nov 18, 2014 at 12:06:24AM +0100, Richard Cochran wrote: > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter, > static void igb_tsync_interrupt(struct igb_adapter *adapter) > { > struct e1000_hw *hw = &adapter->hw; > + struct ptp_clock_event event; > u32 tsicr = rd32(E1000_TSICR); > > + if (tsicr & TSINTR_SYS_WRAP) { > + event.type = PTP_CLOCK_PPS; > + ptp_clock_event(adapter->ptp_clock, &event); This causes a BUG for the 82580. When you enable E1000_TSICR_TXTS, it seems that TSINTR_SYS_WRAP is also automatically enabled. Because the 82580 variant has no pps device enabled, the driver then dereferences a null pointer. So we need to make the call to ptp_clock_event() conditional based on whether pps is enabled. I'll fix this in V2. > + } > + 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
On Wed, 2014-11-19 at 07:37 +0100, Richard Cochran wrote: > On Tue, Nov 18, 2014 at 12:06:24AM +0100, Richard Cochran wrote: > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter, > > static void igb_tsync_interrupt(struct igb_adapter *adapter) > > { > > struct e1000_hw *hw = &adapter->hw; > > + struct ptp_clock_event event; > > u32 tsicr = rd32(E1000_TSICR); > > > > + if (tsicr & TSINTR_SYS_WRAP) { > > + event.type = PTP_CLOCK_PPS; > > + ptp_clock_event(adapter->ptp_clock, &event); > > This causes a BUG for the 82580. When you enable E1000_TSICR_TXTS, it > seems that TSINTR_SYS_WRAP is also automatically enabled. Because the > 82580 variant has no pps device enabled, the driver then dereferences > a null pointer. > > So we need to make the call to ptp_clock_event() conditional based on > whether pps is enabled. I'll fix this in V2. > > > + } > > + > > Thanks, > Richard Good catch :) Did you see my concern about the reset path needing to fully restore the state since it is called after a hardware MAC reset which has cleared all these registers? Regards, Jake
On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote: > Good catch :) (Well, my X session suddenly disappeared, and a kernel oops appeared in the console... hard to overlook ;^) > Did you see my concern about the reset path needing to fully restore the > state since it is called after a hardware MAC reset which has cleared > all these registers? Yes, and that bit I copied from the first series a year ago. I don't remember why, but IIRC that was necessary to let the SDP stuff work at all. Maybe the reset function was called under different circumstances back then. I'll take another look. I find it a bit weird that the auxiliary functions don't work when the interface or the link is down. 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
On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote: > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote: > > Good catch :) > > (Well, my X session suddenly disappeared, and a kernel oops appeared in > the console... hard to overlook ;^) > > > Did you see my concern about the reset path needing to fully restore the > > state since it is called after a hardware MAC reset which has cleared > > all these registers? > > Yes, and that bit I copied from the first series a year ago. I don't > remember why, but IIRC that was necessary to let the SDP stuff work at > all. Maybe the reset function was called under different circumstances > back then. I'll take another look. > > I find it a bit weird that the auxiliary functions don't work when the > interface or the link is down. > > Thanks, > Richard I think you need something here, but it should be clearing that register after a MAC reset, so it needs to be re-initialized. I'm not sure if that reset path was used in the same place in the past. Well, I think igb and ixgbe destroy the ptp device when the interface goes down, and restore it when it comes up. Probably we should instead handle some things in reset path and allow the ptp device to remain. It's partly due to the clock speed changing based on link speed. Regards, Jake
On Wed, Nov 19, 2014 at 09:06:19PM +0000, Keller, Jacob E wrote: > On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote: > > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote: > > > Good catch :) I have not been able to reproduce the crash, and so the cause is not what I thought it was. Maybe it was my patch that preserved the enabled interrupts in igb_ptp_reset(). I didn't notice that the driver frees and reallocates the ptp_clock. I would never do that, myself. > I think you need something here, but it should be clearing that register > after a MAC reset, so it needs to be re-initialized. I'm not sure if > that reset path was used in the same place in the past. Okay, lets figure this out. Why is there a PTP reset function at all? I don't know, lets see who calls it... Finding functions calling: igb_ptp_reset ---------------------------------------- *** drivers/net/ethernet/intel/igb/igb_main.c: igb_reset[2033] igb_ptp_reset(adapter); Easy enough to understand. But who is calling igb_reset? Finding functions calling: igb_reset ------------------------------------ *** drivers/net/ethernet/intel/igb/igb_ethtool.c: igb_set_settings[345] igb_reset(adapter); igb_set_pauseparam[409] igb_reset(adapter); igb_diag_test[2016] igb_reset(adapter); igb_set_eee[2729] igb_reset(adapter); *** drivers/net/ethernet/intel/igb/igb_main.c: igb_down[1814] igb_reset(adapter); igb_set_features[2069] igb_reset(adapter); igb_probe[2526] igb_reset(adapter); __igb_open[3110] igb_reset(adapter); igb_watchdog_task[4231] igb_reset(adapter); igb_change_mtu[5189] igb_reset(adapter); igb_resume[7460] igb_reset(adapter); igb_sriov_reinit[7545] igb_reset(adapter); igb_io_slot_reset[7678] igb_reset(adapter); Wow, that is quite much. So, whenever any random parameter is changed, we reset the PTP clock. Great. Really, wouldn't better to reset the clock functions only when absolutely necessary? 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
On Thu, 2014-11-20 at 10:18 +0100, Richard Cochran wrote: > On Wed, Nov 19, 2014 at 09:06:19PM +0000, Keller, Jacob E wrote: > > On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote: > > > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote: > > > > Good catch :) > > I have not been able to reproduce the crash, and so the cause is not > what I thought it was. Maybe it was my patch that preserved the > enabled interrupts in igb_ptp_reset(). I didn't notice that the driver > frees and reallocates the ptp_clock. I would never do that, myself. > Yea, I think that was a design I tried in the ixgbe driver, but it really isn't good. > > I think you need something here, but it should be clearing that register > > after a MAC reset, so it needs to be re-initialized. I'm not sure if > > that reset path was used in the same place in the past. > > Okay, lets figure this out. Why is there a PTP reset function at all? > I don't know, lets see who calls it... > > Finding functions calling: igb_ptp_reset > ---------------------------------------- > *** drivers/net/ethernet/intel/igb/igb_main.c: > igb_reset[2033] igb_ptp_reset(adapter); > > Easy enough to understand. But who is calling igb_reset? > > Finding functions calling: igb_reset > ------------------------------------ > *** drivers/net/ethernet/intel/igb/igb_ethtool.c: > igb_set_settings[345] igb_reset(adapter); > igb_set_pauseparam[409] igb_reset(adapter); > igb_diag_test[2016] igb_reset(adapter); > igb_set_eee[2729] igb_reset(adapter); > *** drivers/net/ethernet/intel/igb/igb_main.c: > igb_down[1814] igb_reset(adapter); > igb_set_features[2069] igb_reset(adapter); > igb_probe[2526] igb_reset(adapter); > __igb_open[3110] igb_reset(adapter); > igb_watchdog_task[4231] igb_reset(adapter); > igb_change_mtu[5189] igb_reset(adapter); > igb_resume[7460] igb_reset(adapter); > igb_sriov_reinit[7545] igb_reset(adapter); > igb_io_slot_reset[7678] igb_reset(adapter); > > Wow, that is quite much. So, whenever any random parameter is changed, > we reset the PTP clock. Great. Here is the problem. In igb_reset, we call e1000_reset_hw() which will eventually perform a hardware MAC reset. Yes maybe we shouldn't be calling igb_reset everywhere.. I cannot answer that, however... > > Really, wouldn't better to reset the clock functions only when > absolutely necessary? > We *are*. If e1000_reset_hw(hw) is called, the MAC registers will be reset to their initial values, which includes not having SYSTIME setup, and not having the outputs configured. There is not much that can be done to avoid that besides major refactoring of the igb driver to avoid resetting as much as it does. In most of those cases, I think the reset is fine, and we actually aren't going to reset that many times during nominal operation. Some of the ethtool settings maybe could avoid resets, but I am not certain during exactly which path they end up calling resets. Regards, Jake > Thanks, > Richard
On Thu, Nov 20, 2014 at 09:28:26PM +0000, Keller, Jacob E wrote: > Here is the problem. In igb_reset, we call e1000_reset_hw() which will > eventually perform a hardware MAC reset. Yes maybe we shouldn't be > calling igb_reset everywhere.. I cannot answer that, however... It is a typical device driver "code smell". The developer thinks, "Hm, something isn't working in this function. I know - lets reset the device. That _always_ works." > Some of the ethtool settings maybe could avoid resets, but I am not > certain during exactly which path they end up calling resets. Jup. Surely changing the MTU is no reason to reset the PTP clock. But as you said, fixing the reset stuff is a huge amount of work. Instead of trying to do that, I'll just go with the flow and reset the auxiliary functions, too. 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_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 7183a56..9412661 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter, static void igb_tsync_interrupt(struct igb_adapter *adapter) { struct e1000_hw *hw = &adapter->hw; + struct ptp_clock_event event; u32 tsicr = rd32(E1000_TSICR); + if (tsicr & TSINTR_SYS_WRAP) { + event.type = PTP_CLOCK_PPS; + ptp_clock_event(adapter->ptp_clock, &event); + } + if (tsicr & E1000_TSICR_TXTS) { /* acknowledge the interrupt */ wr32(E1000_TSICR, E1000_TSICR_TXTS); diff --git a/drivers/net/ethernet/intel/igb/igb_ptp.c b/drivers/net/ethernet/intel/igb/igb_ptp.c index ce57128..70d3933 100644 --- a/drivers/net/ethernet/intel/igb/igb_ptp.c +++ b/drivers/net/ethernet/intel/igb/igb_ptp.c @@ -360,6 +360,34 @@ static int igb_ptp_settime_i210(struct ptp_clock_info *ptp, return 0; } +static int igb_ptp_feature_enable_i210(struct ptp_clock_info *ptp, + struct ptp_clock_request *rq, int on) +{ + struct igb_adapter *igb = + container_of(ptp, struct igb_adapter, ptp_caps); + struct e1000_hw *hw = &igb->hw; + unsigned long flags; + u32 tsim; + + switch (rq->type) { + case PTP_CLK_REQ_PPS: + spin_lock_irqsave(&igb->tmreg_lock, flags); + tsim = rd32(E1000_TSIM); + if (on) + tsim |= TSINTR_SYS_WRAP; + else + tsim &= ~TSINTR_SYS_WRAP; + wr32(E1000_TSIM, tsim); + spin_unlock_irqrestore(&igb->tmreg_lock, flags); + return 0; + + default: + break; + } + + return -EOPNOTSUPP; +} + static int igb_ptp_feature_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *rq, int on) { @@ -802,12 +830,12 @@ void igb_ptp_init(struct igb_adapter *adapter) adapter->ptp_caps.owner = THIS_MODULE; adapter->ptp_caps.max_adj = 62499999; adapter->ptp_caps.n_ext_ts = 0; - adapter->ptp_caps.pps = 0; + adapter->ptp_caps.pps = 1; adapter->ptp_caps.adjfreq = igb_ptp_adjfreq_82580; adapter->ptp_caps.adjtime = igb_ptp_adjtime_i210; adapter->ptp_caps.gettime = igb_ptp_gettime_i210; adapter->ptp_caps.settime = igb_ptp_settime_i210; - adapter->ptp_caps.enable = igb_ptp_feature_enable; + adapter->ptp_caps.enable = igb_ptp_feature_enable_i210; /* Enable the timer functions by clearing bit 31. */ wr32(E1000_TSAUXC, 0x0); break;
The i210 device can produce an interrupt on the full second. This patch allows using this interrupt to generate an internal PPS event for adjusting the kernel system time. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++++ drivers/net/ethernet/intel/igb/igb_ptp.c | 32 +++++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)