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:
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> 
> ...
> 
> > @@ -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