Re: [PATCH net-next 1/4] igb: refactor and simplify time sync interrupt handling

["Vick, Matthew" <matthew.vick@intel.com>]

On 5/27/13 2:21 AM, "Richard Cochran" <richardcochran@gmail.com> wrote:

>The code that handles the time sync interrupt is repeated in three
>different places. This patch refactors the identical code blocks into
>a single helper function. Also, reading the TSICR register already
>acknowledges the time sync interrupts, and so there is no need to
>write that register at all.
>
>Signed-off-by: Richard Cochran <richardcochran@gmail.com>
>---
> drivers/net/ethernet/intel/igb/igb_main.c |   47
>+++++++++++------------------
> 1 file changed, 17 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>b/drivers/net/ethernet/intel/igb/igb_main.c
>index 6a0c1b6..d25a965 100644
>--- a/drivers/net/ethernet/intel/igb/igb_main.c
>+++ b/drivers/net/ethernet/intel/igb/igb_main.c
>@@ -5035,6 +5035,17 @@ void igb_update_stats(struct igb_adapter *adapter,
> 	}
> }
> 
>+static void igb_tsync_interrupt(struct igb_adapter *adapter)
>+{
>+	struct e1000_hw *hw = &adapter->hw;
>+	u32 tsicr = rd32(E1000_TSICR);
>+
>+	if (tsicr & E1000_TSICR_TXTS) {
>+		/* retrieve hardware timestamp */
>+		schedule_work(&adapter->ptp_tx_work);
>+	}
>+}
>+
> static irqreturn_t igb_msix_other(int irq, void *data)
> {
> 	struct igb_adapter *adapter = data;
>@@ -5066,16 +5077,8 @@ static irqreturn_t igb_msix_other(int irq, void
>*data)
> 			mod_timer(&adapter->watchdog_timer, jiffies + 1);
> 	}
> 
>-	if (icr & E1000_ICR_TS) {
>-		u32 tsicr = rd32(E1000_TSICR);
>-
>-		if (tsicr & E1000_TSICR_TXTS) {
>-			/* acknowledge the interrupt */
>-			wr32(E1000_TSICR, E1000_TSICR_TXTS);
>-			/* retrieve hardware timestamp */
>-			schedule_work(&adapter->ptp_tx_work);
>-		}
>-	}
>+	if (icr & E1000_ICR_TS)
>+		igb_tsync_interrupt(adapter);
> 
> 	wr32(E1000_EIMS, adapter->eims_other);
> 
>@@ -5850,16 +5853,8 @@ static irqreturn_t igb_intr_msi(int irq, void
>*data)
> 			mod_timer(&adapter->watchdog_timer, jiffies + 1);
> 	}
> 
>-	if (icr & E1000_ICR_TS) {
>-		u32 tsicr = rd32(E1000_TSICR);
>-
>-		if (tsicr & E1000_TSICR_TXTS) {
>-			/* acknowledge the interrupt */
>-			wr32(E1000_TSICR, E1000_TSICR_TXTS);
>-			/* retrieve hardware timestamp */
>-			schedule_work(&adapter->ptp_tx_work);
>-		}
>-	}
>+	if (icr & E1000_ICR_TS)
>+		igb_tsync_interrupt(adapter);
> 
> 	napi_schedule(&q_vector->napi);
> 
>@@ -5904,16 +5899,8 @@ static irqreturn_t igb_intr(int irq, void *data)
> 			mod_timer(&adapter->watchdog_timer, jiffies + 1);
> 	}
> 
>-	if (icr & E1000_ICR_TS) {
>-		u32 tsicr = rd32(E1000_TSICR);
>-
>-		if (tsicr & E1000_TSICR_TXTS) {
>-			/* acknowledge the interrupt */
>-			wr32(E1000_TSICR, E1000_TSICR_TXTS);
>-			/* retrieve hardware timestamp */
>-			schedule_work(&adapter->ptp_tx_work);
>-		}
>-	}
>+	if (icr & E1000_ICR_TS)
>+		igb_tsync_interrupt(adapter);
> 
> 	napi_schedule(&q_vector->napi);
> 
>-- 
>1.7.10.4

NAK due to the question you raised in the other thread--this will break
1588 on the 82580, but there was really no way for you to know that. As I
mentioned before, I'm getting the internal discussion going to confirm
that.

Matthew