Re: [PATCH net V4 2/2] igb: offer a PTP Hardware Clock instead of the timecompare method

On Thu, Mar 22, 2012 at 07:13:42PM -0400, chetan loke wrote:

/* users can keep re-trying - dont really care */

[Sigh] No, they do in fact care...

igb_gettime_locking (...) {
     unsigned int seq;
     u64 ns;
     do {
     seq = read_seqbegin( &pigb->tmreg_seq_lock);
     ns = timecounter_read(&pigb->tc);

And what happens here?

timecounter_read()
 timecounter_read_delta()
  tc->cc->read() == igb_825xx_systim_read()

   lo = rd32(E1000_SYSTIML); /* this read latches the time value */
   hi = rd32(E1000_SYSTIMH); /* and here is your race */

If two readers enter this code at nearly the same time, then they can
corrupt each other's hi/lo values.

     } while (read_seqretry(&pigb->tmreg_seq_lock, seq));
     // process ns
}


copyright 2012 - Chetan Loke [off-list ref]

// trip cnt will ensure/enforce - evil adjtime user-space code can
still not block us.
// called from igb_tx[rx]_hwtstamp
driver_rx_tx_path_locking( ... ) {
     unsigned int seq, trip_cnt = 0;
     u64 ns;
     do {
     seq = read_seqbegin( &pigb->tmreg_seq_lock);
     trip_cnt++;
     ns = timecounter_read(&pigb->tc);

Same here.

Thanks,
Richard