Hi Jakub,
Quoting Jakub Kicinski (2020-06-26 21:30:35)
> On Fri, 26 Jun 2020 18:54:23 -0700 Jeff Kirsher wrote:
> > From: Andre Guedes <andre.gue...@intel.com>
> >
> > The __IGC_PTP_TX_IN_PROGRESS flag indicates we have a pending Tx
> > timestamp. In some places, instead of checking that flag, we check
> > adapter->ptp_tx_skb. This patch fixes those places to use the flag.
> >
> > Quick note about igc_ptp_tx_hwtstamp() change: when that function is
> > called, adapter->ptp_tx_skb is expected to be valid always so we
> > WARN_ON_ONCE() in case it is not.
> >
> > Quick note about igc_ptp_suspend() change: when suspending, we don't
> > really need to check if there is a pending timestamp. We can simply
> > clear it unconditionally.
> >
> > Signed-off-by: Andre Guedes <andre.gue...@intel.com>
> > Tested-by: Aaron Brown <aaron.f.br...@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
> > ---
> > drivers/net/ethernet/intel/igc/igc_ptp.c | 16 +++++++---------
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c
> > b/drivers/net/ethernet/intel/igc/igc_ptp.c
> > index b1b23c6bf689..e65fdcf966b2 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> > @@ -404,9 +404,6 @@ void igc_ptp_tx_hang(struct igc_adapter *adapter)
> > bool timeout = time_is_before_jiffies(adapter->ptp_tx_start +
> > IGC_PTP_TX_TIMEOUT);
> >
> > - if (!adapter->ptp_tx_skb)
> > - return;
> > -
> > if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
> > return;
> >
> > @@ -435,6 +432,9 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter
> > *adapter)
> > struct igc_hw *hw = &adapter->hw;
> > u64 regval;
> >
> > + if (WARN_ON_ONCE(!skb))
> > + return;
> > +
> > regval = rd32(IGC_TXSTMPL);
> > regval |= (u64)rd32(IGC_TXSTMPH) << 32;
> > igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
> > @@ -466,7 +466,7 @@ static void igc_ptp_tx_work(struct work_struct *work)
> > struct igc_hw *hw = &adapter->hw;
> > u32 tsynctxctl;
> >
> > - if (!adapter->ptp_tx_skb)
> > + if (!test_bit(__IGC_PTP_TX_IN_PROGRESS, &adapter->state))
> > return;
>
> Not that reading ptp_tx_skb is particularly correct here, but I think
> it's better. See how they get set:
>
> if (adapter->tstamp_config.tx_type == HWTSTAMP_TX_ON &&
> !test_and_set_bit_lock(__IGC_PTP_TX_IN_PROGRESS,
> &adapter->state)) {
> skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> tx_flags |= IGC_TX_FLAGS_TSTAMP;
>
> adapter->ptp_tx_skb = skb_get(skb);
> adapter->ptp_tx_start = jiffies;
>
> bit is set first and other fields after. Since there is no locking here
> we may just see the bit but none of the fields set.
I see your point, but note that the code within the if-block and the code in
igc_ptp_tx_work() don't execute concurrently. adapter->ptp_tx_work is scheduled
only on a time-sync interrupt, which is triggered if IGC_TX_FLAGS_TSTAMP is
set (so adapter->ptp_tx_skb is valid).
- Andre
