Re: [PATCH v3 net-next 3/8] ptp: netc: export netc_timer_get_current_time() for cross-driver use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using device_lock() in an exported API to protect against driver unbind is an architectural anti-pattern that creates a circular lock dependency (lockdep warning) and limits caller contexts.
--

commit 9efea393693780854f3e912aa851d7d32115845a
Author: Wei Fang <wei.fang@nxp.com>

ptp: netc: export netc_timer_get_current_time() for cross-driver use

The NETC Switch does not have its own time registers and must obtain
the current PTP time from the NETC Timer bound to it. Since the two
are separate PCIe functions with independent drivers, add
netc_timer_get_current_time() to the Timer driver and export it via
EXPORT_SYMBOL_GPL().

> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> index aa9be8e2a6304..854a206b6c5bc 100644
> --- a/drivers/ptp/ptp_netc.c
> +++ b/drivers/ptp/ptp_netc.c
[ ... ]
> @@ -1021,5 +1021,55 @@ static struct pci_driver netc_timer_driver = {
>  };
>  module_pci_driver(netc_timer_driver);
>  
> +/**
> + * netc_timer_get_current_time - read the current PTP time from the NETC Timer
> + * @pdev: PCI device of the NETC Timer
> + * @ns: The current PTP clock time in nanoseconds, returned to the caller
> + *
> + * Reads the 64-bit current time register (TMR_CUR_TIME) from the NETC Timer
> + * device associated with @pdev. Returns an error if the Timer driver has not
> + * yet probed or has already been removed.
> + *
> + * Context: Process context only. Acquires the device mutex via device_lock(),
> + *          which may sleep. Must not be called from atomic context, softirq,
> + *          BH, or while holding a spinlock.
> + *
> + * Return: 0 on success, otherwise a negative error code.
> + */
> +int netc_timer_get_current_time(struct pci_dev *pdev, u64 *ns)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct netc_timer *priv;
> +	unsigned long flags;
> +	int err = 0;
> +
> +	/* Serialize against driver unbind: the remove() callback runs under
> +	 * the device lock, so holding it here ensures that priv remains valid
> +	 * for the entire duration of the register read.
> +	 */
> +	device_lock(dev);

[Severity: High]
Does acquiring the timer's device lock here create a circular lock dependency
with the switch's remove path?

When the switch driver unbinds, it acquires the switch device's dev->mutex,
then rtnl_lock() via dsa_unregister_switch(), and then onestep->work_lock
via netc_port_disable().

In the datapath workqueue, netc_onestep_work() acquires onestep->work_lock,
and calls this function, which acquires the timer's dev->mutex.

Because lockdep tracks all dev->mutex locks initialized by device_initialize()
as a single lock class, wouldn't this form a dev->mutex -> work_lock ->
dev->mutex circular dependency?

Could we prevent the timer from unbinding while the switch is active by
creating a device link with device_link_add() instead, which would allow
lock-free reads?

> +
> +	if (pci_dev_driver(pdev) != &netc_timer_driver) {
> +		err = -EINVAL;
> +		goto unlock_device;
> +	}
> +
> +	priv = pci_get_drvdata(pdev);
> +	if (!priv) {
> +		err = -ENOMEM;
> +		goto unlock_device;
> +	}
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	*ns = netc_timer_cur_time_read(priv);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +unlock_device:
> +	device_unlock(dev);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(netc_timer_get_current_time);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831082335.1184605-1-wei.fang@oss.nxp.com?part=3