Re: [PATCH net-next v6 05/11] net: wangxun: move reusable PCI driver ops functions into libwx

[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: wangxun: move reusable PCI driver ops functions into libwx

This commit adds a function pointer for close_suspend and adjusts do_reset.
These changes allow PCI driver operations such as shutdown to be called
directly from the common libwx library.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index 746623fa59b40..c65240ca3c283 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c

[ ... ]

> @@ -3342,5 +3342,90 @@ void wx_service_timer(struct timer_list *t)
>  }
>  EXPORT_SYMBOL(wx_service_timer);
>  
> +static void wx_dev_shutdown(struct pci_dev *pdev, bool *enable_wake)
> +{
> +	struct wx *wx = pci_get_drvdata(pdev);
> +	struct net_device *netdev;
> +	u32 wufc = wx->wol;
> +
> +	netdev = wx->netdev;
> +	rtnl_lock();
> +	netif_device_detach(netdev);
> +
> +	if (netif_running(netdev))
> +		wx->close_suspend(wx);
> +
> +	wx_clear_interrupt_scheme(wx);
> +	rtnl_unlock();
> +
> +	if (wufc) {
> +		wx_set_rx_mode(netdev);
> +		wx_configure_rx(wx);

Does this sequence result in a use-after-free of the DMA rings?

When the interface is running, wx->close_suspend() is called, which frees
all receive and transmit descriptor rings via dma_free_coherent() in
wx_free_resources().

If Wake-on-LAN is enabled (wufc != 0), the code then calls
wx_configure_rx(). This programs the freed DMA physical addresses from
ring->dma (which is not cleared upon free) into the MACs Receive Descriptor
Base Address registers.

If the MAC fetches from these freed pages, could it interpret garbage data as
buffer addresses and DMA incoming network packets into random physical memory?

> +		wr32(wx, WX_PSR_WKUP_CTL, wufc);
> +	} else {
> +		wr32(wx, WX_PSR_WKUP_CTL, 0);
> +	}
> +	pci_wake_from_d3(pdev, !!wufc);
> +	*enable_wake = !!wufc;
> +	wx_control_hw(wx, false);
> +
> +	pci_disable_device(pdev);
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 50a13dcf4b3c8..baffcadb22690 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c

[ ... ]

> @@ -604,11 +570,11 @@ static void ngbe_reinit_locked(struct wx *wx)
>  	clear_bit(WX_STATE_RESETTING, wx->state);
>  }
>  
> -void ngbe_do_reset(struct net_device *netdev)
> +void ngbe_do_reset(struct net_device *netdev, bool reinit)
>  {
>  	struct wx *wx = netdev_priv(netdev);
>  
> -	if (netif_running(netdev))
> +	if (netif_running(netdev) && reinit)
>  		ngbe_reinit_locked(wx);
>  }

Is it intentional that ngbe_do_reset() does nothing when reinit is false?

Before this patch, ngbe_resume() explicitly called ngbe_reset_hw().
Now that it uses the shared wx_resume(), it calls wx->do_reset(netdev, false).

Since ngbe_do_reset() lacks an else branch for when reinit is false
(unlike txgbe_do_reset()), the hardware reset is skipped entirely during
resume. Will this leave the MAC/PHY uninitialized and break the interface
after a suspend/resume cycle?