Re: [PATCH net-next v8 1/4] net: rnpgbe: Add interrupt handling

Hi Simon:

On Wed, Aug 05, 2026 at 10:34:28AM +0100, Simon Horman wrote:

On Fri, Jul 31, 2026 at 08:03:19PM +0800, Dong Yibo wrote:

...

quoted

diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c

...

quoted

@@ -50,6 +68,11 @@ static int rnpgbe_open(struct net_device *netdev)
  **/
 static int rnpgbe_close(struct net_device *netdev)
 {
+	struct mucse *mucse = netdev_priv(netdev);
+
+	rnpgbe_down(mucse);
+	rnpgbe_free_irq(mucse);
+
 	return 0;
 }

Hi Dong,

There is an AI-generated review of this patch-set available at
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260731120322.895955-1-dong100%40mucse.com
I would appreciate it if you could take a look over it.

In particular, the following issue does seem to be of concern.

I take the last portion regarding ixgbe to be a template for a solution to
the problem raised, not that the __IXGBE_DOWN bit should litteraly be used
in the rnpgbe driver.

  Is ndo_stop now safe to call twice?

  rnpgbe_dev_shutdown() in this same file calls rnpgbe_close() directly rather
  than going through dev_close():

	  rtnl_lock();
	  netif_device_detach(netdev);
	  if (netif_running(netdev))
		  rnpgbe_close(netdev);
	  rtnl_unlock();

  netif_device_detach() only clears __LINK_STATE_PRESENT, so IFF_UP and
  __LINK_STATE_START stay set and the core still believes the interface is up.

  A later dev_close() (device_shutdown() runs before migrate_to_reboot_cpu(),
  so an "ip link set dev X down" on another CPU can still get through), or an
  unbind/unregister_netdev() after a shutdown, would call ndo_stop again:

    dev_close() -> __dev_close_many() -> ops->ndo_stop() -> rnpgbe_close()

  On that second pass rnpgbe_down() runs again:

	  void rnpgbe_down(struct mucse *mucse)
	  {
		  set_bit(__MUCSE_DOWN, &mucse->state);
		  rnpgbe_napi_disable_all(mucse);
		  rnpgbe_irq_disable(mucse);
	  }

  With plain set_bit() there is nothing to detect the re-entry, so
  napi_disable() runs on an already-disabled NAPI.  In napi_disable_locked():

	  while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		  usleep_range(20, 200);
		  val = READ_ONCE(n->state);
	  }

  The irq is already freed and NAPI was never re-enabled, so can anything
  clear those bits?  That loop has no timeout, and it runs under rtnl_lock().

  The second rnpgbe_free_irq() would also call free_irq() on vectors already
  released, giving the "Trying to free already-free IRQ" warning.

  Before this patch rnpgbe_close() was just "return 0;", so the double call
  was harmless.

  Would the ixgbe guard fit here?

	  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_down() {
		  /* signal that we are down to the interrupt handler */
		  if (test_and_set_bit(__IXGBE_DOWN, &adapter->state))
			return; /* do nothing if already down */
		...
	}

  Alternatively, should rnpgbe_dev_shutdown() call dev_close() so the core
  state stays consistent?

Yes, it is an error. I will use rnpgbe's own __MUCSE_DOWN bit to make the
teardown idempotent. rnpgbe_down() will atomically report whether it performed
the transition, and rnpgbe_close() will skip IRQ and resource teardown when
the device is already down. This covers a later ndo_stop after the shutdown
path has detached and stopped the device.

...

Thanks for your feedback