On Thu, 2024-03-28 at 00:53 +0200, Bitterblue Smith wrote:
>
> On 27/03/2024 20:48, Kalle Valo wrote:
> > Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> writes:
> >
> > > On 22/03/2024 08:04, Ping-Ke Shih wrote:
> > > > On Wed, 2024-03-20 at 21:43 +0200, Bitterblue Smith wrote:
> > >
> > > [...]
> > >
> > > > > +DEFINE_MUTEX(globalmutex_power);
> > > > > +DEFINE_MUTEX(globalmutex_for_fwdownload);
> > > > > +DEFINE_MUTEX(globalmutex_for_power_and_efuse);
> > > > > +DEFINE_MUTEX(globalmutex_for_mac0_2g_mac1_5g);
> > > >
> > > > The consumers of globalmutex_for_mac0_2g_mac1_5g are complex. Why do they
> > > > check mutex_is_locked()? Race conditions between two instances?
> > > >
> > >
> > > I couldn't think of a sufficiently short name, like
> > > "lock_mac0_2g_mac1_5g", so I used mutex_is_locked(). That's probably
> > > a bad idea. It should be like this:
> > >
> > > /* Let the first starting mac load RF parameters and do LCK */
> > > if (rtlhal->macphymode == DUALMAC_DUALPHY &&
> > > ((rtlhal->interfaceindex == 0 && rtlhal->bandset == BAND_ON_2_4G) ||
> > > (rtlhal->interfaceindex == 1 && rtlhal->bandset == BAND_ON_5G))) {
> > > mutex_lock(&globalmutex_for_mac0_2g_mac1_5g);
> > > lock_mac0_2g_mac1_5g = true;
> > > }
globalmutex_for_mac0_2g_mac1_5g is only used in rtl92du_hw_init(), and
globalmutex_for_power_and_efuse does very similar thing. Can we combine them
into one? Since both are only used in rtl92du_hw_init(), it would not be a
problem to enlarge their critical section.
>
> * Dual MAC dual PHY: this I can't find to buy anymore. This
> appears in the system as two Wifi devices, each working on
> a different band. It has two USB interfaces. Two instances
> of the driver access the same device. This is what the
> mutexes are for.
I traced the code, and found rules of two MAC/PHY are:
1. read efuse to decide single or two MAC/PHY
2.1. if single MAC/PHY, register_hw as 2T2R (done)
2.2. if dual MAC/PHY (to step 3)
3. read interface index (USB: from bInterfaceNumber; PCI: from PCI onfigure space)
4. register interface index 0 as 1T1R on 5 GHz band only.
register interface index 1 as 1T1R on 2 GHz band only.
This is the case two instances (netdev) access single one hardware device,
so seemingly it is hard to avoid global locks to prevent racing between them.
An alternative thought is to support only single MAC/PHY, but not sure if
driver can override setting of efuse that programmed the card as two MAC/PHY.
>
> I said earlier that I think two devices can work at the same
> time, even with the global mutexes, but now I remembered there
> are two more global variables: curveindex_5g[] and
> curveindex_2g[] in phy.c. One driver instance fills one array
> during LC calibration, but the other driver instance reads
> from the array when switching the channel. If I'm reading this
> right. So two devices plugged in at the same time probably
> won't work correctly.
That should be a problem.
>
> How can you avoid this when the hardware is the way it is?
> My one idea is to add a global map to hold the mutexes and
> arrays, using the USB port number, etc as the key.
Seemingly we need something like per device data.
>
> > I'm starting to wonder if extending rtlwifi is actually the right
> > approach. We have modern drivers like rtl8xxxu, rtw88 etc. with better
> > design.
> >
>
> I think extending rtlwifi is the right approach when it already
> has most of the code needed. I really don't want to reinvent this
> particular wheel.
Right, but honestly rtlwifi did a lot of tricky things we can't understand
the whole picture...
> I don't want to add the dual MAC stuff to rtl8xxxu or rtw88.
I don't want that neither.
