[PATCH v1] net/mlx5: Use generic power management

[PATCH v1] net/mlx5: Use generic power management

[Vaibhav Gupta]

Switch to the generic power management and remove the usage of legacy
(pci_driver) hooks.

Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 5f28d906c35b..d8dba3328bb6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -2204,8 +2204,9 @@ static void shutdown(struct pci_dev *pdev)
 	mlx5_pci_disable_device(dev);
 }
 
-static int mlx5_suspend(struct pci_dev *pdev, pm_message_t state)
+static int mlx5_suspend(struct device *device)
 {
+	struct pci_dev *pdev = to_pci_dev(device);
 	struct mlx5_core_dev *dev = pci_get_drvdata(pdev);
 
 	mlx5_unload_one(dev, true);
@@ -2213,13 +2214,16 @@ static int mlx5_suspend(struct pci_dev *pdev, pm_message_t state)
 	return 0;
 }
 
-static int mlx5_resume(struct pci_dev *pdev)
+static int mlx5_resume(struct device *device)
 {
+	struct pci_dev *pdev = to_pci_dev(device);
 	struct mlx5_core_dev *dev = pci_get_drvdata(pdev);
 
 	return mlx5_load_one(dev, false);
 }
 
+static DEFINE_SIMPLE_DEV_PM_OPS(mlx5_pm_ops, mlx5_suspend, mlx5_resume);
+
 static const struct pci_device_id mlx5_core_pci_table[] = {
 	{ PCI_VDEVICE(MELLANOX, PCI_DEVICE_ID_MELLANOX_CONNECTIB) },
 	{ PCI_VDEVICE(MELLANOX, 0x1012),
@@ -2281,8 +2285,7 @@ static struct pci_driver mlx5_core_driver = {
 	.id_table       = mlx5_core_pci_table,
 	.probe          = probe_one,
 	.remove         = remove_one,
-	.suspend        = mlx5_suspend,
-	.resume         = mlx5_resume,
+	.driver.pm      = pm_sleep_ptr(&mlx5_pm_ops),
 	.shutdown	= shutdown,
 	.err_handler	= &mlx5_err_handler,
 	.sriov_configure   = mlx5_core_sriov_configure,
-- 
2.56.0

Re: [PATCH v1] net/mlx5: Use generic power management

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, the PCI core puts the mlx5 function into
  D3hot/D3cold in pci_pm_suspend_noirq() once mlx5_suspend() returns.

--

net/mlx5: Use generic power management

This patch moves mlx5_core from the legacy pci_driver suspend and resume
hooks to dev_pm_ops. It converts mlx5_suspend() and mlx5_resume() to take
a struct device and registers them with DEFINE_SIMPLE_DEV_PM_OPS() and
pm_sleep_ptr().

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 5f28d906c35b6..d8dba3328bb6a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c

[ ... ]

> @@ -2281,8 +2285,7 @@ static struct pci_driver mlx5_core_driver = {
>  	.id_table       = mlx5_core_pci_table,
>  	.probe          = probe_one,
>  	.remove         = remove_one,
> -	.suspend        = mlx5_suspend,
> -	.resume         = mlx5_resume,
> +	.driver.pm      = pm_sleep_ptr(&mlx5_pm_ops),

[Severity: Medium]
With dev_pm_ops in use, the PCI core now calls pci_prepare_to_sleep() from
pci_pm_suspend_noirq() after mlx5_suspend() returns. That puts the function
into D3hot or D3cold. The legacy path did not do this.

Can the PTP overflow worker still touch the device after that point? The
suspend path is:

mlx5_suspend()->mlx5_unload_one(dev, true)->mlx5_unload()->mlx5_clock_unload()

mlx5_clock_unload() unregisters the PPS notifier and cancels out_work:

	mlx5_eq_notifier_unregister(mdev, &mdev->clock_state->pps_nb);
	cancel_work_sync(&mdev->clock_state->out_work);

It does not stop the PTP aux worker. That worker is started in
mlx5_init_clock_dev():

	if (clock->ptp)
		ptp_schedule_worker(clock->ptp, 0);

It only stops when ptp_clock_unregister() runs from
mlx5_destroy_clock_dev() on the cleanup_once path, and suspend never takes
that path. The worker runs on a kthread worker from kthread_run_worker(),
which is not freezable.

mlx5_timestamp_overflow() re-arms itself every overflow_period. Its only
guard is:

	if (mdev->state == MLX5_DEVICE_STATE_INTERNAL_ERROR)
		goto out;

mlx5_unload_one_devl_locked() leaves dev->state at MLX5_DEVICE_STATE_UP, so
the worker goes on to timecounter_read()->read_internal_timer()->
mlx5_read_time(), which reads BAR0:

	timer_h = ioread32be(real_time ? &dev->iseg->real_time_h :
			     &dev->iseg->internal_timer_h);

In s2idle, timekeeping comes back when s2idle_loop exits. A delayed work
that expired during sleep then fires right away. That can happen before
dpm_resume_noirq() returns mlx5 to D0. It can also happen in the window
between mlx5's suspend_noirq and sleep entry.

Would those reads then reach a function in D3hot, complete as Unsupported
Request and return all-ones? They may also log AER errors.

If so, timecounter_read() would take the all-ones value (masked by
cc->mask) as a real counter value. It would move tc->nsec and cycle_last
forward, and mlx5_update_clock_info_page() would publish the bad values to
the clock_info page that RDMA userspace maps.

Nothing on the resume path (mlx5_load()->mlx5_clock_load()) appears to
re-initialize the timecounter.

Should the overflow worker be stopped in the unload path when suspending
(for example with ptp_cancel_worker_sync()) and rescheduled on load?

>  	.shutdown	= shutdown,
>  	.err_handler	= &mlx5_err_handler,
>  	.sriov_configure   = mlx5_core_sriov_configure,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017194104.2684-1-vaibhavgupta40%40gmail.com