> > +int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
> > +{
> > + struct netc_timer *priv = NULL;
> > + struct netc_timer *tmp;
> > + unsigned long flags;
> > + int err = 0;
> > +
> > + /* Serialize against driver unbind, so holding it here ensures that
> > + * priv remains valid for the entire duration of the register read.
> > + */
> > + spin_lock_bh(&netc_timer_list_lock);
>
> [Severity: Medium]
> Is it necessary to hold the module-global netc_timer_list_lock across the
> MMIO access, rather than only across the lookup?
>
> priv->lock is taken from hardirq context with a plain spin_lock() in
> netc_timer_isr(), and that handler can spin in the register drain loop in
> netc_timer_handle_etts_event():
>
> drivers/ptp/ptp_netc.c:netc_timer_handle_etts_event() {
> while (netc_timer_rd(priv, NETC_TMR_STAT) & TMR_STAT_ETS_VLD(index))
> etts = netc_timer_rd64(priv, NETC_TMR_ETTS_L(index));
> }
>
> So while CPU A sits in netc_timer_get_current_time() waiting on
> spin_lock_irqsave(&priv->lock) with softirqs disabled and the global list lock
> held, CPU B can be inside netc_timer_isr() holding priv->lock for a period
> bounded only by hardware. During that window every other Timer's probe(),
> remove() and every other time query is serialized behind
> netc_timer_list_lock. The same applies if the target function is unreachable
> and the non-posted read stalls until the PCIe completion timeout.
>
> Would an RCU-protected lookup (with the priv free deferred in remove()), or
> taking a reference on the Timer, give the same lifetime guarantee without
> holding a global lock across the register access?
>
> Related to that: the BH-only requirement makes the exported symbol illegal
> from hardirq or with interrupts disabled, but that constraint appears only in
> the kerneldoc and is not visible at the prototype in
> include/linux/fsl/netc_global.h, and nothing but an eventual WARN from
> __local_bh_enable_ip() catches a violation. Is there a reason not to drop the
> context restriction by using a lookup scheme that does not disable BH?
>
There is effectively no contention on the global lock. NETC has at most 3 Timer
instances, and the Switch can only use Timer0. netc_timer_get_current_time()
is used solely by the Switch driver - the ENETC driver never calls it. At
runtime,
only the Switch driver ever takes netc_timer_list_lock; the other Timer
instances
take it only during their own probe()/remove(). So in practice the lock has no
competing runtime users. On top of that, the drain loop in
netc_timer_handle_etts_event() is bounded - the hardware time-stamp FIFO
holds at most 16 entries, so the ISR holds priv->lock for at most 16 register
reads,
not an unbounded time.
With no real contender for the lock, switching to RCU would only add complexity
without a meaningful gain.
> > +
> > + list_for_each_entry(tmp, &netc_timer_list, node) {
> > + if (tmp->pdev == pdev) {
> > + priv = tmp;
> > + break;
> > + }
> > + }
>
> [Severity: Low]
> This isn't a bug, but the lookup is a bare pointer comparison on a
> caller-supplied struct pci_dev *, so its soundness depends on the caller
> holding a reference on @pdev. The in-tree consumer does hold one
> (netc_get_ptp_timer() obtains it with pci_get_domain_bus_and_slot() and
> drops it with pci_dev_put()), so nothing is broken today.
>
> Could the kerneldoc and the exported prototype state that the caller must
> hold a reference on @pdev? Without it, a future caller that caches a raw
> pci_dev pointer has no hint that a freed and recycled pointer could match a
> different Timer's entry.
The only caller is the NETC switch driver, I do not think we need to update
the kernel-doc.
>
> > +
> > + if (!priv) {
> > + err = -ENODEV;
> > + goto netc_timer_list_unlock;
> > + }
> > +
> > + spin_lock_irqsave(&priv->lock, flags);
> > + *ns = netc_timer_cur_time_read(priv);
> > + spin_unlock_irqrestore(&priv->lock, flags);
>
> [Severity: Medium]
> Can this return 0 together with a meaningless timestamp when the Timer
> function is not accessible?
>
> List membership only proves the pci_dev is bound to this driver: the entry is
> added at the end of netc_timer_probe() and removed only in
> netc_timer_remove(). netc_timer_driver registers no PM callbacks, so system
> suspend takes the PCI core default path and the function is disabled and put
> into D3 while the list entry stays live, and nothing re-programs
> TMR_CTRL[TE], CK_SEL, TMR_ADD or TMR_CNT on resume. The driver already
> documents the constraint in netc_timer_init():
>
> drivers/ptp/ptp_netc.c:netc_timer_init() {
> /* Software must enable timer first and the clock selected must be
> * active, otherwise, the registers which are in the timer clock
> * domain are not accessible.
> */
> }
>
> The same holds for a surprise-removed function or one contained by DPC/AER,
> where the read returns all-ones. In those cases the helper still reports
> success, and the switch side consumes the value verbatim as the one-step Sync
> originTimestamp in netc_port_onestep_work():
>
> drivers/net/dsa/netc/netc_ptp.c:netc_port_onestep_work() {
> NETC_SKB_CB(skb)->tstamp = tstamp;
> }
>
> Would it make sense to gate the read on pdev->current_state == PCI_D0 and
> !pci_channel_offline(pdev), or on TMR_CTRL[TE], and return an error instead
> of a successful-looking value? The missing PM support is not introduced by
> this patch, but the new export is what turns an inaccessible-device read into
> a timestamp handed to another driver.
>
NETC is a Root Complex integrated Endpoint (RCiEP). The Switch, ENETC and
Timer are all on-chip integrated functions with no external PCIe link and no
physical connector. So surprise removal and AER/DPC containment simply
cannot occur here.
Suspend/resume is not supported anywhere in the NETC subsystem - the Switch,
ENETC and Timer drivers have no PM callbacks. When PM support is added later,
it will guarantee that the Timer suspends after the Switch and resumes before
it,
so the Timer is always programmed and accessible whenever the Switch may
query it.
