Re: [PATCH net-next v16 1/5] net: rnpgbe: Add interrupt handling

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch is titled and described as "Add interrupt handling",
  but it also changes how the already-merged driver shuts down and is…

--

net: rnpgbe: Add interrupt handling

This commit adds MSI-X interrupt setup and teardown for the rnpgbe driver.
Vector 0 is reserved for the mailbox and the remaining vectors are used for
the data path. Mailbox lock polling now uses a sleepable variant, and a
shared helper sends the firmware powerdown notification from both remove
and shutdown.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 70a2b0082ba86..6022a1a2ddf1e 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c

[ ... ]

> @@ -236,6 +281,26 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	return err;
>  }
>  
> +/**
> + * rnpgbe_notify_powerdown - Notify firmware that the driver is going down
> + * @mucse: pointer to private structure
> + *
> + * Skip the mailbox access if an earlier shutdown has already disabled the
> + * PCI device.
> + **/
> +static void rnpgbe_notify_powerdown(struct mucse *mucse)
> +{
> +	int err;
> +
> +	if (!pci_is_enabled(mucse->pdev))
> +		return;

[ ... ]

> @@ -272,7 +335,8 @@ static void rnpgbe_remove(struct pci_dev *pdev)
>  {
>  	rnpgbe_rm_adapter(pdev);
>  	pci_release_mem_regions(pdev);
> -	pci_disable_device(pdev);
> +	if (pci_is_enabled(pdev))
> +		pci_disable_device(pdev);
>  }
>  
>  /**
> @@ -287,9 +351,14 @@ static void rnpgbe_dev_shutdown(struct pci_dev *pdev)
>  	rtnl_lock();
>  	netif_device_detach(netdev);
>  	if (netif_running(netdev))
> -		rnpgbe_close(netdev);
> +		dev_close(netdev);
>  	rtnl_unlock();
> -	pci_disable_device(pdev);
> +
> +	rnpgbe_free_mbx_irq(mucse);
> +	rnpgbe_notify_powerdown(mucse);
> +	rnpgbe_clear_interrupt_scheme(mucse);
> +	if (pci_is_enabled(pdev))
> +		pci_disable_device(pdev);
>  }

[Severity: Low]
This isn't a bug, but should the commit message cover these teardown
changes? The only related text in the commit message is:

  Notify firmware of driver powerdown from both remove and shutdown
  through a shared helper.

The patch also changes how the existing teardown code behaves, and the
commit message does not describe these changes:

  - rnpgbe_dev_shutdown() now calls dev_close() instead of calling
    rnpgbe_close() directly.

  - rnpgbe_dev_shutdown() and rnpgbe_remove() now call
    pci_disable_device() only when pci_is_enabled() is true.

  - rnpgbe_notify_powerdown() skips the mailbox when the device is
    already disabled. Its kernel-doc says this is for remove running
    after an earlier shutdown.

  - Shutdown now sends a firmware powerdown, which the previous
    rnpgbe_dev_shutdown() never did. The commit message doesn't say why.

The dev_close() switch looks correct. rnpgbe_close() now calls
rnpgbe_down() and rnpgbe_free_irq(). Calling ndo_stop directly leaves
IFF_UP set, so a later unregister_netdev() would call it again and run
napi_disable() and free_irq() a second time.

Could these changes be explained in the commit message? Another option is
to split them into a separate patch, with a Fixes: tag if they address an
observed problem, so reviewers and backporters can follow them.

This code in rnpgbe_main.c is unchanged through the end of the series
("net: rnpgbe: Add link status handling support"), and no later patch
explains it either.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/3B42BE25B3696EAE%2B20261013022920.63529-1-dong100%40mucse.com