Almost every upm_transfer function starts with:
mutex_enter(&sc->sc_lock);
err = usb_insert_transfer(xfer);
mutex_exit(&sc->sc_lock);
if (err)
return err;
Some of them have debug messages sprinkled in here too, or assert
that err == USBD_NORMAL_COMPLETION (alternative is USBD_IN_PROGRESS,
only for pipes with up_running or up_serialise, presumably not
applicable for these types of pipes). Some of them also assert
xfer->ux_status == USBD_NOT_STARTED, which is guaranteed on entry and
preserved by usb_insert_transer.
Exceptions:
- arch/mips/adm5120/dev/ahci.c ahci_device_isoc_transfer just returns
USBD_NORMAL_COMPLETION, but I'm pretty sure this is and always has
been broken anyway, so won't make anything worse (if anything, might
make it better...)
- external/bsd/dwc2/dwc2.c dwc2_device_bulk_transfer and
dwc2_device_isoc_transfer _also_ issue dwc2_device_start(xfer)
under the lock. This is probably a better way to do it, but let's
do it uniformly across all HCIs at once.
- rump/dev/lib/libugenhc/ugenhc.c rumpusb_device_bulk_transfer
sometimes returns USBD_IN_PROGRESS _without_ queueing the transfer,
in the !rump_threads case. Not really sure how this is supposed to
work... If it actually breaks anything, we can figure it out.
Make usbnet_stop private now that no drivers use it.
None of the driver-independent logic in usbnet_stop has any effect at
this point because we are guaranteed not to be running, so only the
driver-dependent logic in *_uno_stop (at most) is needed.
For drivers with no *_uno_stop, just omit the call to usbnet_stop
altogether.
Some of this logic is obviously redundant with the subsequent call to
*_reset -- to be addressed in a subsequent commit.
Init/stop and ioctl happen under IFNET_LOCK. Multicast updates only
happen after init and before stop. Core lock is no longer a relevant
part of the API. Internally, it serves essentially just to lock out
asynchronous mii activity during init/stop.
During attach, the caller has exclusive access to the usbnet until
usbnet_attach_ifp. At other times, register access is serialized
either by the usbnet multicast lock or by IFNET_LOCK.
Some callers don't check the error code, e.g. ~all the mii phy
drivers using PHY_READ. Just return zero if the device is gone or
the xfer fails for any other reason.
The usbnet wrappers don't add anything important. We already test
usbnet_isdying in axen_cmd, and that's already a best-effort thing
(which should probably be done better by having usbd_do_request fail
promptly if detaching anyway).
These only happen either during the transition up or down (init or
stop), or while that transition is excluded (ioctl).
This may be called from ioctl or from init, which both hold the ifnet
lock.
XXX smsc_setoe_locked should maybe trigger reinit because the rx loop
behaves differently depending on whether checksumming is enabled.
XXX mue_sethwcsum_locked needs to exclude mcast updates.
This can change at any moment; no software lock can prevent the
device from being detached. Any test of it is necessarily
best-effort just to avoid wasting time later on waiting for requests
to fail or time out.
To make this work:
1. Do it only under a new lock, unp_mcastlock. This lock lives at
IPL_SOFTCLOCK so it can be taken from network stack callouts. It
is forbidden to acquire the usbnet core lock under unp_mcastlock.
2. Do it only after usbnet_init_rx_tx and before usbnet_stop; if
issued at any other time, drop the update on the floor.
3. Make usbnet_init_rx_tx apply any pending multicast filter updates
under the lock before setting the flag that allows SIOCADDMULTI or
SIOCDELMULTI to apply the updates.
4. Remove core lock asserts from various drivers' register access
routines. This is necessary because the multicast filter updates
are done with register reads/writes, but _cannot_ take the core
lock when the caller holds softnet_lock.
This now programs the hardware multicast filter redundantly in many
drivers which already explicitly call *_uno_mcast from the *_uno_init
routines. This is probably harmless, but it will likely be better to
remove the explicit calls.
This legacy flag is a figment of userland's imagination. The actual
kernel state is ec->ec_flags & ETHER_F_ALLMULTI, protected by the
ETHER_LOCK, so that multicast filter updates -- which run without
IFNET_LOCK -- need not attempt to write racily to ifp->if_flags.
This operation only needs to update the hardware to reflect
SIOCADDMULTI/SIOCDELMULTI. Not clear that everything in aue(4) needs
to be reset -- in fact I'm pretty sure that's undesirable!
WARNING: I have not tested this with a real aue(4) device.
Every driver does this already. This will enable us to change the
lock that serializes access to the registers so we can go back to
doing this synchronously in SIOCADDMULTI/SIOCDELMULTI.
The only state this touches is unp_if_flags, and all paths touching
it also hold IFNET_LOCK -- not to mention this is the only path that
touches unp_if_flags in the first place!
After mii_detach, these have all completed and no new ones can be
made, and detach doesn't start destroying anything until after
mii_detach has returned, so there is no need to hang onto a reference
count here.
These run with IFNET_LOCK held, and the interface cannot be detached
until the IFNET_LOCK is released, so there is no need to hang onto a
reference count here.
usbnet_detach cannot run until the attach routine has finished
(unless a driver goes out of its way to tie its shoelaces together
and explicitly call it during the attach routine, which none of them
do), so there is no need to hang onto a reference count that we
release before attach returns.
This callback always runs with the IFNET_LOCK held, and the interface
cannot be detached until the IFNET_LOCK is released, so there is no
need to hang onto a reference count here. (None of the subnet
drivers touch the IFNET_LOCK except to verify it is held sometimes.)
This callback always runs with IFNET_LOCK held, and during a task
that usbnet_detach prevents scheduling anew and waits for finishing
before completing the detach, so there is no need to hang onto a
reference count here.
This callback always runs with the IFNET_LOCK held, and the interface
cannot be detached until the IFNET_LOCK is released, so there is no
need to hang onto a reference count here. (None of the usbnet
drivers touch the IFNET_LOCK except to verify it is held sometimes.)
Now that is tested and set with atomic_load/store, there is no need
to hold the lock -- which means we can set it while the core lock is
held during, e.g., a reset sequence, and use that to interrupt the
sequence so it doesn't get stuck waiting to time out when the device
is physically removed.
This way we don't need to hold the core lock to avoid upsetting
sanitizers (which probably find the current code upsetting), and we
can use it to exit early from timeout loops that run under the core
lock (which is probably not necessary for them to do anyway, but
let's worry about that later).
This reduces code in all drivers except urndis(4) and aue(4).
However, it's still safe for urndis to drop the core lock because the
ifnet is locked, and the ifnet lock covers the DOWN->UP (uno_init)
and UP->DOWN (uno_stop) transitions.
riastradh