Re: [PATCH net-next v6 06/11] net: txgbe: add power management support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: txgbe: add power management support

This commit adds power management support to the txgbe driver, including
.suspend, .resume, and Wake-on-LAN functionality. It updates the pci_driver
struct to use libwx suspend, resume, and shutdown callbacks.

[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index 30f66507809bd..ddc7cea9d7c28 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
[ ... ]
> @@ -975,7 +960,9 @@ static struct pci_driver txgbe_driver = {
>  	.id_table = txgbe_pci_tbl,
>  	.probe    = txgbe_probe,
>  	.remove   = txgbe_remove,
> -	.shutdown = txgbe_shutdown,
> +	.suspend  = wx_suspend,
> +	.resume   = wx_resume,
> +	.shutdown = wx_shutdown,
>  	.sriov_configure = wx_pci_sriov_configure,
>  };

When Wake-on-LAN is enabled, do wx_shutdown() or wx_suspend() cause a
NULL pointer dereference regression?

If the interface is running, wx_dev_shutdown() calls txgbe_close_suspend(),
which frees the RX and TX rings via wx_free_resources().

wx_free_resources() calls kvfree(rx_ring->rx_buffer_info) and sets it to
NULL. It also calls dma_free_coherent() for rx_ring->desc but leaves
rx_ring->dma holding a stale IOVA.

Later in wx_dev_shutdown(), since Wake-on-LAN is enabled, it calls
wx_configure_rx() which loops over the rings and calls
wx_configure_rx_ring().

wx_configure_rx_ring() explicitly programs the hardware with the stale
DMA address ring->dma, which might lead to memory corruption. Furthermore,
it calls memset(ring->rx_buffer_info, 0, ...), which causes an unconditional
NULL pointer dereference.

Is there also a potential data race regression when configuring Wake-on-LAN
during suspend/shutdown?

In wx_dev_shutdown(), rtnl_lock() is explicitly unlocked right before
checking if Wake-on-LAN is enabled. If it is enabled, the driver calls
wx_set_rx_mode(netdev).

Inside wx_set_rx_mode(), the driver accesses the netdev's unicast and
multicast address lists. Since neither rtnl_lock() nor netif_addr_lock()
is held here, could concurrent list modifications cause list corruption?

Could the wx->service_task workqueue remain uncancelled during suspend and
shutdown?

The driver initializes a work item wx->service_task that runs periodically
via wx->service_timer. During suspend or shutdown, txgbe_disable_device()
deletes the timer with timer_delete_sync(), but
cancel_work_sync(&wx->service_task) is never called.

If the timer fired just before being deleted, the work item could execute
txgbe_service_task() in the background while the PCI device is placed into
D3hot or its PCI master bit is disabled, which might trigger PCI target
aborts or lockups.

Lastly, is the device missing a transition to PCI_D3hot during system
suspend?

The patch adds the .suspend = wx_suspend callback using the legacy PCI
power management API. Legacy PCI PM callbacks do not automatically invoke
pci_prepare_to_sleep() or pci_set_power_state() to place the device into
a low-power state.

While the wx_shutdown() path explicitly calls
pci_set_power_state(pdev, PCI_D3hot), wx_suspend() does not. Consequently,
the device appears to be left in PCI_D0 during system suspend.