Tony Lindgren <tony@xxxxxxxxxxx> writes: > * Kalle Valo <kvalo@xxxxxxxxxxxxxx> [200622 14:15]: >> Tony Lindgren <tony@xxxxxxxxxxx> writes: >> >> > We need the spinlock to check if we need to run the queue. Let's use >> > spin_trylock instead and always run the queue unless we know there's >> > nothing to do. >> >> Why? What's the problem you are solving here? > > To simplify the flags and locking use between the threaded irq > and tx work. > > While chasing an occasional hang with an idle wlan doing just a > periodic network scans, I noticed we can start simplifying the > locking between the threaded irq and tx work for the driver. > > No luck so far figuring out what the occasional idle wlan hang is, > but I suspect we end up somewhere in a deadlock between tx work > and the threaded irq. > > We currently have a collection of flags and locking between the > threaded irq and tx work: > > - wl->flags bitops > - wl->mutex > - wl->wl_lock spinlock > > The bitops flags do not need a spinlock around them, and > wlcore_irq() already holds the mutex calling wlcore_irq_locked(). > And we only need the spinlock to see if we need to run the queue > or not. > > So I think eventually we can remove most of the spinlock use in > favor of the mutex. I guess I could leave out the trylock changes > here if this is too many changes at once. > > Or do you see some problem in general with this approach? My only problem was lack of background information in the commit logs. Conditional locking is tricky and I didn't figure out why you are doing that and why it's safe to do. So if you could send v2 with the information above in the commit log I would be happy. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
