Re: kern/57645: bridge does not work on raspberry pi

[mlelstv%serpens.de@localhost (Michael van Elst)]

The following reply was made to PR kern/57645; it has been noted by GNATS.

From: mlelstv%serpens.de@localhost (Michael van Elst)
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: kern/57645: bridge does not work on raspberry pi
Date: Sun, 8 Oct 2023 13:34:36 -0000 (UTC)

 sc.dying%gmail.com@localhost writes:
 
 > I don't think this code works as expected,
 > as IFF_CANTCHANGE includes IFF_PROMISC.
 > 
 > 	if ((changed & (~(IFF_CANTCHANGE | IFF_DEBUG) | IFF_PROMISC)) != 0) {
 > 
 > would work.
 
 
 Here is a better patch. The logic is now the same as in e.g. the bge driver
 and still works for me.
 
 
 Index: sys/dev/usb/usbnet.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/usb/usbnet.c,v
 retrieving revision 1.114
 diff -p -u -r1.114 usbnet.c
 --- sys/dev/usb/usbnet.c	15 Jul 2023 21:41:26 -0000	1.114
 +++ sys/dev/usb/usbnet.c	8 Oct 2023 13:27:08 -0000
 @@ -1011,11 +1011,12 @@ usbnet_ifflags_cb(struct ethercom *ec)
  
  	KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname);
  
 +	mutex_enter(&unp->unp_mcastlock);
  	const u_short changed = ifp->if_flags ^ unp->unp_if_flags;
 -	if ((changed & ~(IFF_CANTCHANGE | IFF_DEBUG)) == 0) {
 -		mutex_enter(&unp->unp_mcastlock);
 -		unp->unp_if_flags = ifp->if_flags;
 -		mutex_exit(&unp->unp_mcastlock);
 +
 +	if ((changed & ~(IFF_CANTCHANGE | IFF_DEBUG)) != 0) {
 +		rv = ENETRESET;
 +	} else {
  		/*
  		 * XXX Can we just do uno_mcast synchronously here
  		 * instead of resetting the whole interface?
 @@ -1028,12 +1029,13 @@ usbnet_ifflags_cb(struct ethercom *ec)
  		 * Maybe the logic can be unified, but it will require
  		 * an audit and testing of all the usbnet drivers.
  		 */
 -		if (changed & IFF_PROMISC)
 +		if ((changed & (IFF_PROMISC|IFF_ALLMULTI)) != 0)
  			rv = ENETRESET;
 -	} else {
 -		rv = ENETRESET;
  	}
  
 +	unp->unp_if_flags = ifp->if_flags;
 +	mutex_exit(&unp->unp_mcastlock);
 +
  	return rv;
  }
  
 @@ -1065,23 +1067,23 @@ usbnet_if_ioctl(struct ifnet *ifp, u_lon
  
  	error = ether_ioctl(ifp, cmd, data);
  	if (error == ENETRESET) {
 +		/*
 +		 * If there's a hardware multicast filter, and
 +		 * it has been programmed by usbnet_init_rx_tx
 +		 * and is active, update it now.  Otherwise,
 +		 * drop the update on the floor -- it will be
 +		 * observed by usbnet_init_rx_tx next time we
 +		 * bring the interface up.
 +		 */
 +		if (un->un_ops->uno_mcast) {
 +			mutex_enter(&unp->unp_mcastlock);
 +			if (unp->unp_mcastactive)
 +				(*un->un_ops->uno_mcast)(ifp);
 +			mutex_exit(&unp->unp_mcastlock);
 +		}
  		switch (cmd) {
  		case SIOCADDMULTI:
  		case SIOCDELMULTI:
 -			/*
 -			 * If there's a hardware multicast filter, and
 -			 * it has been programmed by usbnet_init_rx_tx
 -			 * and is active, update it now.  Otherwise,
 -			 * drop the update on the floor -- it will be
 -			 * observed by usbnet_init_rx_tx next time we
 -			 * bring the interface up.
 -			 */
 -			if (un->un_ops->uno_mcast) {
 -				mutex_enter(&unp->unp_mcastlock);
 -				if (unp->unp_mcastactive)
 -					(*un->un_ops->uno_mcast)(ifp);
 -				mutex_exit(&unp->unp_mcastlock);
 -			}
  			error = 0;
  			break;
  		default:
 @@ -1306,8 +1308,16 @@ usbnet_if_init(struct ifnet *ifp)
  	 * analysis -- and possibly some tweaking -- of sys/net to
  	 * ensure.
  	 */
 -	if (ifp->if_flags & IFF_RUNNING)
 +	if (ifp->if_flags & IFF_RUNNING) {
 +		if (un->un_ops->uno_mcast) {
 +			struct usbnet_private * const unp = un->un_pri;
 +			mutex_enter(&unp->unp_mcastlock);
 +			(*un->un_ops->uno_mcast)(ifp);
 +			unp->unp_mcastactive = true;
 +			mutex_exit(&unp->unp_mcastlock);
 +		}
  		return 0;
 +	}
  
  	error = uno_init(un, ifp);
  	if (error)