> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 36df742c326c..a00fbce6337f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -382,6 +382,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx,
> u8 idx)
> struct ice_ptp_port *ptp_port;
> unsigned long flags;
> struct sk_buff *skb;
> + struct device *dev;
> struct ice_pf *pf;
>
> if (!tx->init)
> @@ -389,6 +390,7 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx,
> u8 idx)
>
> ptp_port = container_of(tx, struct ice_ptp_port, tx);
> pf = ptp_port_to_pf(ptp_port);
> + dev = ice_pf_to_dev(pf);
> params = &pf->hw.ptp.phy.e810;
>
> /* Drop packets which have waited for more than 2 seconds */
Sashiko says:
>
> This is a pre-existing issue, but does the early return in the timeout
> path of ice_ptp_req_tx_single_tstamp() abort the hardware interrupt chain?
> If a request times out, the function drops the packet and returns early:
> if (time_is_before_jiffies(tx->tstamps[idx].start + 2 * HZ)) {
> ...
> dev_kfree_skb_any(skb);
> return;
> }
> Returning early without initiating the next firmware read seems like it
> would stall the chain and strand any remaining timestamps.
I think this is plausible, if unlikely to actually have happened in
practice. The 2-second drop was copied from the normal interface for Tx
timestamps, and not the low latency interrupt.
I think this should probably be moved out of
ice_ptp_req_tx_single_tstamp() and placed in the caller which determines
the index and drops stale timestamps until it finds a valid index.
> @@ -408,7 +410,13 @@ void ice_ptp_req_tx_single_tstamp(struct ice_ptp_tx *tx,
> u8 idx)
>
> spin_lock_irqsave(¶ms->atqbal_wq.lock, flags);
>
> - params->atqbal_flags |= ATQBAL_FLAGS_INTR_IN_PROGRESS;
> + if (test_and_set_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS,
> + params->atqbal_flags)) {
> + dev_dbg(dev, "%s: low latency interrupt request already in
> progress\n",
> + __func__);
> + spin_unlock_irqrestore(¶ms->atqbal_wq.lock, flags);
> + return;
> + }
>
Sashiko says:
>
> If ATQBAL_FLAGS_INTR_IN_PROGRESS is leaked and sticks permanently, does
> this new check prevent the system from ever recovering?
> Previously, calling this function would overwrite the registers and
> effectively self-heal. Now, if the flag is leaked (for example, if
> firmware drops a timestamp or during teardown), the function will return
> early. This could lead to a permanent deadlock of all Tx timestamp and
> clock operations.
I don't know of a real case where the firmware would drop a timestamp.
However, if it were to happen this could now deadlock the timestamp
interface.
> /* Write TS index to read to the PF register so the FW can read it */
> wr32(&pf->hw, REG_LL_PROXY_H,
> @@ -449,7 +457,8 @@ void ice_ptp_complete_tx_single_tstamp(struct ice_ptp_tx
> *tx)
>
> spin_lock_irqsave(¶ms->atqbal_wq.lock, flags);
>
> - if (!(params->atqbal_flags & ATQBAL_FLAGS_INTR_IN_PROGRESS))
> + if (!test_and_clear_bit(ATQBAL_FLAGS_INTR_IN_PROGRESS,
> + params->atqbal_flags))
> dev_dbg(dev, "%s: low latency interrupt request not in
> progress?\n",
> __func__);
>
Sashiko says:
> Can the ATQBAL_FLAGS_INTR_IN_PROGRESS flag be leaked if the completion
> routine bails out early?
> For example, in ice_ptp_complete_tx_single_tstamp(), there is an early
> return before this flag is cleared:
> if (!tx->init || tx->last_ll_ts_idx_read < 0)
> return;
> If this early return is hit, won't we leak the
> ATQBAL_FLAGS_INTR_IN_PROGRESS flag? Since the new test_and_set_bit()
> check in ice_ptp_req_tx_single_tstamp() relies on this flag being
> cleared to proceed, leaving it set here would cause all future timestamp
> requests to permanently fail.
This is ultimately effectively the same concern as the previous comment.
If the timestamp doesn't complete or if we get an invalid index etc,
this will potentially get stuck.
I think both of these concerns should be addressed by adding a mechanism
to clear the flag after some time if no timestamp has completed. I think
this fits well with our existing PTP watchdog thread. Thus I plan to do
the following:
1) move the 2-second drop out of ice_ptp_req_tx_single_tstamp(), and
have its caller perform the check while picking the next available
timestamp. Depending on size, this may be done in a separate patch, I'm
not yet certain.
2) Add a mechanism to the watchdog to check and clear the flag if we've
been waiting for too long for the timestamp to arrive, and we should
therefore assume firmware won't respond to that request.
Regards,
Jake
