Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Apr 2021 18:22:16 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Pkshih <pkshih@realtek.com>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "kvalo@codeaurora.org" <kvalo@codeaurora.org>
Subject: Re: [PATCH v4 13/19] rtw89: 8852a: add 8852a specific files
Message-ID: <YItbyGGlZN4q9iOh@google.com>
References: <20210429080149.7068-1-pkshih@realtek.com>
 <20210429080149.7068-14-pkshih@realtek.com>
 <YIsg3iv6d4L6n9Uk@google.com>
 <1619739791.1874.18.camel@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1619739791.1874.18.camel@realtek.com>
Precedence: bulk

On Thu, Apr 29, 2021 at 11:43:12PM +0000, Pkshih wrote:
> On Thu, 2021-04-29 at 21:10 +0000, Brian Norris wrote:
> > rtw89_write_rf() is holding a mutex (rf_mutex). Judging by its trivial
> > usage (it's only protecting register reads/writes), it probably could be
> > a spinlock instead -- although I do note some magic udelay()s in there.
> > 
> 
> The udelay() is needed to ensure the indirect-write correct.

OK. Maybe deserves a comment for the future. Is this a
hardware-specified timing (measured in number of cycles or similar, on
the WiFi chip side), or something you're just guessing at?

> > Alternatively, it looks like you'd be safe moving to the non-atomic
> > ieee80211_iterate_active_interfaces() in rtw89_leave_lps().
> > 
> 
> For most cases of?rtw89_leave_lps(), we can use?ieee80211_iterate_active_interfaces(),
> which hold iflist_mtx lock, except to?
> 
> 	ieee80211_recalc_ps(local);	// held iflist_mtx lock
> 		...
> 		ieee80211_hw_config
> 			...
> 			rtw89_leave_lps()
> 				...
> 				ieee80211_iterate_active_interfaces()
> 
> That will leads deadlock.

Good point.

> Another variant ieee80211_iterate_active_interfaces_mtx() that doesn't
> hold a lock may be a solution. The the comment says "This version can
> only be used while holding the RTNL.", and the code within the function
> says "lockdep_assert_wiphy(hw->wiphy);". I'm not sure if I can use it
> to prevent locking iflist_mtx twice.

This doesn't quite feel like the right thing. You're in the midst of
many other callback layers, and I don't think this is the right place to
be grabbing those locks. But I haven't researched this very closely yet.

> If I can use it, I can add a argument 'mtx', like rtw89_leave_lps(rtwdev, bool mtx),
> to judge using ieee80211_iterate_active_interfaces() or ieee80211_iterate_active_interfaces_mtx().
> 
> I'm also thinking that we can still use?ieee80211_iterate_active_interfaces_atomic()
> to merely collect?rtwvif->mac_id list, and use a loop to do leave_lps
> out of the atomic iterate.

That's probably safe, because we're already holding rtwdev->mutex, so
there's no chance of our mac_id going away. If that solution isn't too
complex, it makes sense to me.

Brian