On 05/01/17(Thu) 14:56, Stefan Sperling wrote: > I've received reports from various folks over the last year where iwm > would print messages like these while various ifconfig commands are used: > > iwm0: could not initiate scan > iwm0: device timeout > > I found this also happens on one of my machines during bsd.rd upgrades. > While the upgrade script processes /etc/hostname.iwm0 it ends up running > something like this: > ifconfig iwm0 nwid foo wpakey foofoofoo; ifconfig iwm0 down; dhclient iwm0 > This command triggers the 'could not initiate scan' message for me even with > a GENERIC.MP kernel. > > The problem seems to be that iwm's various tasks will sleep while waiting > for a device command to finish. If an ioctl process now sneaks in it will > trigger new commands to be sent in parallel, which the hardware does not like. > > I see one way of fixing this: grab the ioctl lock in all of iwm's tasks. > With this patch I cannot reproduce the problem anymore. > > It's a bit tricky since the newstate task may be scheduled from interrupt > context as well as ioctl process context. So it only grabs the lock if it > is not already held by another process. > > I don't really like this but it seems this is needed for correctness. > Does somebody have a better idea?
I have a different idea, I'm not sure it is better. It appears to me, once again, that you're working around the design of the wireless stack in iwm(4). The stack hasn't been design with the idea that callbacks can sleep. That's why you deferred the logic of at least ``ic_update_htprot'', ``ic_ampdu_rx_start'', ``iwm_ampdu_rx_stop'' and ``iwm_newstate'' to tasks. If I recall correctly the original motivation was to wait for the firmware to finish its work, something that iwn(4) doesn't do. Now you found that deferring the work can lead to races. So I'd say make the driver damn simple and don't wait for the firmware. Another option would be to fix the stack, but that's more work. I'm afraid that with the lock you know introduce a difference set of race... see below. > @@ -5487,7 +5510,18 @@ iwm_newstate_task(void *psc) > enum ieee80211_state ostate = ic->ic_state; > struct iwm_node *in = (struct iwm_node *)ic->ic_bss; > int arg = sc->ns_arg; > - int err; > + int err, s, locked; > + > + /* > + * Prevent ioctls from interfering while we are sleeping. > + * Since this task can be scheduled from ioctl context another > + * process might already be holding the lock and waiting for us > + * to finish up. Don't grab the lock in that case. > + */ > + locked = (rw_status(&sc->ioctl_rwl) != 0); > + if (!locked) > + rw_enter_write(&sc->ioctl_rwl); So basically ioctl() grabs the lock then sleep. So you can't reuse this particular lock. As long as you want to keep the execution of your hooks asynchronous, it should be enough to sleep at the end of iwm_ioctl() until all the scheduled tasks are completed. It's similar to usbd_ref_wait(), however this can lead to other races with timeouts.
