On Fri, Sep 20, 2024 at 10:28:55AM +0200, Remi Pommarel wrote:
> On Thu, Sep 19, 2024 at 12:22:10PM +0200, Johannes Berg wrote:
> > On Thu, 2024-09-19 at 12:02 +0200, Nicolas Cavallari wrote:
> > >
> > > > Did I miss something ? Which one should we do ? I'm not sure of all the
> > > > implications of switching to the wiphy work queue and why it did not get
> > > > converted like the color_change_finalize_work stuff ?
> > >
> > > ieee80211_color_collision_detection_work() used to lock the wdev mutex, now it
> > > does not hold anything since 076fc8775da("wifi: cfg80211: remove wdev mutex")
> > >
> > > Also the rate limiting uses delayed_work_pending(), There is no wiphy work queue
> > > equivalent AFAIK, so the explicit lock is probably the way to go.
> >
> > That won't work, it's cancel_delayed_work_sync() under the wiphy mutex,
> > so that'll cause deadlocks (and should cause lockdep complaints about
> > them.)
>
> Yes you are right, and AFAIU that is this kind of issue using wiphy work
> queue would prevent. With wiphy work queue if wiphy_delayed_work_cancel
> is called with wiphy lock held, work cannot be running after it returns;
> making it handy to replace cancel_delayed_work_sync() with.
>
> So, in my opinion, switching to wiphy work queue seems to be the
> prefered solution here.
>
> While there is no wiphy work queue equivalent of delayed_work_pending(),
> I think using timer_pending(&link->color_collision_detect_work->timer)
> to replace delayed_work_pending(), even if the semantic is a bit
> different, would be ok to fulfill the rate limiting purpose. Having the
> same delayed_work_pending() semantics on wiphy work queue would require
> to take wiphy lock which seem a bit superfluous here.
Forgot to mention that I am currently running a wiphy work version of
ieee80211_color_collision_detection_work() using timer_pending() as a
delayed_work_pending() substitute since a couple of hours and lockdep
did not complain so far.
--
Remi
