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

[sc.dying%gmail.com@localhost]

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

From: sc.dying%gmail.com@localhost
To: gnats-bugs%netbsd.org@localhost, kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost
Cc: 
Subject: Re: kern/57645: bridge does not work on raspberry pi
Date: Sun, 8 Oct 2023 21:36:07 +0000

 On 2023/10/08 13:35, Michael van Elst wrote:
 > 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.
 
 It works for me, too.
 Thank you for rewriting this.
 
 >  
 >  
 >  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)
 >