Re: [PATCH 1/4] wlcore: Use spin_trylock in wlcore_irq_locked() for running the queue

[Kalle Valo <kvalo@codeaurora.org>]

Tony Lindgren <tony@atomide.com> writes:

> * Kalle Valo <kvalo@codeaurora.org> [200622 14:15]:
>> Tony Lindgren <tony@atomide.com> 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