Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] idpf: leaked latches and race on disable in idpf_ptp_set_timestamp_mode() - [High] idpf: NULL cmd_sync dereference after overwrite in idpf_ptp_get_caps() - [Medium] idpf: tx_tstamp_timer not stopped when vport stops in idpf_vport_stop() --
commit dab766921f8ee37b82339faa63fd1af80a2f807b Author: Przemyslaw Korba <[email protected]> idpf: add direct Tx timestamp readout This commit adds direct BAR register access support for reading hardware Tx timestamps in the idpf driver. In direct mode, a periodic 100 us hrtimer polls the readiness bitmap and latch registers, reports completed timestamps to the network stack, and expires unlatched requests after 500 ms. > diff --git a/drivers/net/ethernet/intel/idpf/idpf_ptp.c > b/drivers/net/ethernet/intel/idpf/idpf_ptp.c > index a781b3dbe2d62..adb3f31daa5d9 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_ptp.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_ptp.c [ ... ] > @@ -829,35 +888,258 @@ u64 idpf_ptp_extend_ts(struct idpf_vport *vport, u64 > in_tstamp) [ ... ] > +static enum hrtimer_restart idpf_ptp_tx_tstamp_timer_cb(struct hrtimer > *timer) > +{ > + struct idpf_vport *vport; > + > + vport = container_of(timer, struct idpf_vport, tx_tstamp_timer); > + > + idpf_ptp_get_tx_tstamp_direct(vport); > + hrtimer_forward_now(timer, IDPF_PTP_TX_TSTAMP_PERIOD_NS); > + > + return HRTIMER_RESTART; > +} > + > +/** > + * idpf_ptp_tx_tstamp_timer_init - Set up the Tx timestamp collection timer > + * @vport: Virtual port structure > + */ > +void idpf_ptp_tx_tstamp_timer_init(struct idpf_vport *vport) > +{ > + hrtimer_setup(&vport->tx_tstamp_timer, idpf_ptp_tx_tstamp_timer_cb, > + CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); > +} > + > +/** > + * idpf_ptp_tx_tstamp_timer_set - Arm or disarm the Tx timestamp timer > + * @vport: Virtual port structure > + * @enable: whether the Tx timestamps are to be collected > + * > + * In the direct mode nothing in the Tx path reads the latches, so the > + * collection is driven by a timer that runs for as long as the Tx > + * timestamping is enabled. > + */ > +static void idpf_ptp_tx_tstamp_timer_set(struct idpf_vport *vport, bool > enable) > +{ > + if (!idpf_ptp_is_vport_tx_tstamp_ena(vport) || > + vport->tx_tstamp_caps->access != IDPF_PTP_DIRECT) > + return; > + > + if (enable) > + hrtimer_start(&vport->tx_tstamp_timer, > + IDPF_PTP_TX_TSTAMP_PERIOD_NS, > + HRTIMER_MODE_REL_SOFT); > + else > + hrtimer_cancel(&vport->tx_tstamp_timer); > +} [Severity: Medium] Does vport->tx_tstamp_timer keep running at 10 kHz if the interface is brought down while hardware Tx timestamping is enabled? When HWTSTAMP_TX_ON is active in IDPF_PTP_DIRECT mode and the interface is stopped via idpf_stop() -> idpf_vport_stop(), the queues are torn down and vport->link_up is set to false: drivers/net/ethernet/intel/idpf/idpf_lib.c:idpf_vport_stop() { ... vport->link_up = false; idpf_vport_intr_deinit(vport, rsrc); idpf_xdp_rxq_info_deinit_all(rsrc); idpf_vport_queues_rel(vport, rsrc); idpf_vport_intr_rel(rsrc, &adapter->irq); clear_bit(IDPF_VPORT_UP, np->state); ... } However, idpf_vport_stop() does not cancel vport->tx_tstamp_timer or flush vport->tx_tstamp_caps->latches_in_use, and idpf_ptp_tx_tstamp_timer_cb() unconditionally rearms the timer with HRTIMER_RESTART. Because idpf_hwtstamp_set() rejects changes when !vport->link_up with -EPERM, userspace cannot disable Tx timestamping while the interface is down either. In addition, any skbs with SKBTX_IN_PROGRESS that were cleaned from the Tx ring during idpf_vport_stop() remain on latches_in_use for 500 ms until idpf_ptp_get_tx_tstamp_direct() expires them, logging false-positive warnings and counting them as expired rather than flushed. Should idpf_vport_stop() cancel vport->tx_tstamp_timer and flush latches_in_use (and idpf_vport_open() restart the timer if vport->tstamp_config.tx_type == HWTSTAMP_TX_ON)? > @@ -917,12 +1199,18 @@ int idpf_ptp_set_timestamp_mode(struct idpf_vport > *vport, > case HWTSTAMP_TX_ON: > if (!idpf_ptp_is_vport_tx_tstamp_ena(vport)) > return -EINVAL; > + > break; > default: > return -EINVAL; > } > > - vport->tstamp_config.tx_type = config->tx_type; > + /* Publish the new mode before the collection is disarmed, so that the > + * Tx path stops handing out latches that nothing would collect. > + */ > + WRITE_ONCE(vport->tstamp_config.tx_type, config->tx_type); > + idpf_ptp_tx_tstamp_timer_set(vport, config->tx_type == HWTSTAMP_TX_ON); [Severity: High] Can disabling Tx timestamping in idpf_ptp_set_timestamp_mode() strand in-use latches and leak their skb references? In IDPF_PTP_DIRECT mode, idpf_tx_read_tstamp() is a no-op, so vport->tx_tstamp_timer running idpf_ptp_get_tx_tstamp_direct() is the only place that collects or expires entries from tstamp_caps->latches_in_use. When userspace transitions config->tx_type to HWTSTAMP_TX_OFF via idpf_hwtstamp_set() -> idpf_ptp_set_timestamp_mode(), idpf_ptp_tx_tstamp_timer_set(vport, false) immediately cancels vport->tx_tstamp_timer without draining or flushing entries already queued on tstamp_caps->latches_in_use. Furthermore, the Tx datapath checks tstamp_config.tx_type outside of tstamp_caps->latches_lock: drivers/net/ethernet/intel/idpf/idpf_txrx.c:idpf_tx_tstamp() { ... if (READ_ONCE(idpf_netdev_to_vport(tx_q->netdev)->tstamp_config.tx_type) != HWTSTAMP_TX_ON) return -1; ... err = idpf_ptp_request_ts(tx_q, skb, &idx); } A concurrent Tx thread can pass the READ_ONCE() check in idpf_tx_tstamp() just before WRITE_ONCE() sets HWTSTAMP_TX_OFF, and then call idpf_ptp_request_ts() after hrtimer_cancel() has finished, moving a latch onto latches_in_use and taking an skb_get(skb) reference after the timer is already stopped. Those stranded entries on latches_in_use hold their skb references (with SKBTX_IN_PROGRESS set) and latch indexes until teardown or until HWTSTAMP_TX_ON is re-enabled. If HWTSTAMP_TX_ON is later re-enabled after the 32-bit PHC counter wraps, idpf_ptp_get_tx_tstamp_direct() only checks ptp_tx_tstamp->deadline when !ptp_tx_tstamp->tstamp, so a latch whose readiness and valid bits were set before the timer stopped will have its old 32-bit timestamp extended against the current PHC time and reported via skb_tstamp_tx(). Should idpf_ptp_request_ts() verify tx_type under latches_lock, and should disabling Tx timestamping flush latches_in_use (or let the timer drain remaining entries before stopping)? > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c > b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c > index 427b15281ad0d..6443ce34a0d92 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c > @@ -48,6 +48,7 @@ int idpf_ptp_get_caps(struct idpf_adapter *adapter) > VIRTCHNL2_CAP_PTP_SET_DEVICE_CLK_TIME_MB | > VIRTCHNL2_CAP_PTP_ADJ_DEVICE_CLK | > VIRTCHNL2_CAP_PTP_ADJ_DEVICE_CLK_MB | > + VIRTCHNL2_CAP_PTP_TX_TSTAMPS | > VIRTCHNL2_CAP_PTP_TX_TSTAMPS_MB) > }; [Severity: High] In idpf_ptp_get_caps(), can the cross_tstamp block overwrite a valid ptp->dev_clk_regs.cmd_sync pointer with NULL while leaving get_dev_clk_time_access and adj_dev_clk_time_access set to IDPF_PTP_DIRECT? When ptp->get_dev_clk_time_access is IDPF_PTP_DIRECT, idpf_ptp_get_caps() maps clock_offsets.cmd_sync_trigger into ptp->dev_clk_regs.cmd_sync and validates it in clk_regs[]: drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c:idpf_ptp_get_caps() { ... temp_offset = le32_to_cpu(clock_offsets.cmd_sync_trigger); ptp->dev_clk_regs.cmd_sync = libie_pci_get_mmio_addr(mmio, temp_offset); ... ptp->get_dev_clk_time_access = idpf_ptp_regs_access(ptp, clk_regs, ARRAY_SIZE(clk_regs), VIRTCHNL2_CAP_PTP_GET_DEVICE_CLK_TIME_MB); cross_tstamp: access_type = ptp->get_cross_tstamp_access; if (access_type != IDPF_PTP_DIRECT) goto discipline_clock; cross_tstamp_offsets = recv_ptp_caps_msg->cross_time_offsets; ... temp_offset = le32_to_cpu(cross_tstamp_offsets.cmd_sync_trigger); ptp->dev_clk_regs.cmd_sync = libie_pci_get_mmio_addr(mmio, temp_offset); } If cross_tstamp_offsets.cmd_sync_trigger cannot be mapped, libie_pci_get_mmio_addr() returns NULL and overwrites ptp->dev_clk_regs.cmd_sync. Checking cross_regs[] then downgrades ptp->get_cross_tstamp_access, but ptp->get_dev_clk_time_access, ptp->adj_dev_clk_time_access (whose adj_regs[] does not include cmd_sync), and ptp->set_dev_clk_time_access remain IDPF_PTP_DIRECT, causing subsequent calls to idpf_ptp_tmr_cmd() and idpf_ptp_enable_shtime() to pass a NULL ptp->dev_clk_regs.cmd_sync pointer to writel(). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
