When the driver is used for concurrent mode, both virtual interfaces can
be set to station or AP mode, though only one can be in AP mode at the
same time.
In order to keep the code simple, use only hw port 0 for AP mode. When
an interface is added in AP mode which would be assigned to port 1, use
a switch_port function to transparently swap the mapping between virtual
interface and hw port.
Signed-off-by: Martin Kaistra <[email protected]>
---
.../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 84 ++++++++++++++++++-
1 file changed, 82 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 88730791091a7..595f447874f4d 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -6613,6 +6613,84 @@ static int rtl8xxxu_submit_int_urb(struct ieee80211_hw *hw)
return ret;
}
+static void rtl8xxxu_switch_ports(struct rtl8xxxu_priv *priv)
+{
+ u8 macid[ETH_ALEN], bssid[ETH_ALEN], macid_1[ETH_ALEN], bssid_1[ETH_ALEN];
+ u8 msr, bcn_ctrl, bcn_ctrl_1, atimwnd[2], atimwnd_1[2];
+ struct rtl8xxxu_vif *rtlvif;
+ struct ieee80211_vif *vif;
+ u8 tsftr[8], tsftr_1[8];
+ int i;
+
+ msr = rtl8xxxu_read8(priv, REG_MSR);
+ bcn_ctrl = rtl8xxxu_read8(priv, REG_BEACON_CTRL);
+ bcn_ctrl_1 = rtl8xxxu_read8(priv, REG_BEACON_CTRL_1);
+
+ for (i = 0; i < ARRAY_SIZE(atimwnd); i++)
+ atimwnd[i] = rtl8xxxu_read8(priv, REG_ATIMWND + i);
+ for (i = 0; i < ARRAY_SIZE(atimwnd_1); i++)
+ atimwnd_1[i] = rtl8xxxu_read8(priv, REG_ATIMWND_1 + i);
+
+ for (i = 0; i < ARRAY_SIZE(tsftr); i++)
+ tsftr[i] = rtl8xxxu_read8(priv, REG_TSFTR + i);
+ for (i = 0; i < ARRAY_SIZE(tsftr); i++)
+ tsftr_1[i] = rtl8xxxu_read8(priv, REG_TSFTR1 + i);
+
+ for (i = 0; i < ARRAY_SIZE(macid); i++)
+ macid[i] = rtl8xxxu_read8(priv, REG_MACID + i);
+
+ for (i = 0; i < ARRAY_SIZE(bssid); i++)
+ bssid[i] = rtl8xxxu_read8(priv, REG_BSSID + i);
+
+ for (i = 0; i < ARRAY_SIZE(macid_1); i++)
+ macid_1[i] = rtl8xxxu_read8(priv, REG_MACID1 + i);
+
+ for (i = 0; i < ARRAY_SIZE(bssid_1); i++)
+ bssid_1[i] = rtl8xxxu_read8(priv, REG_BSSID1 + i);
+
+ /* disable bcn function, disable update TSF */
+ rtl8xxxu_write8(priv, REG_BEACON_CTRL, (bcn_ctrl &
+ (~BEACON_FUNCTION_ENABLE)) | BEACON_DISABLE_TSF_UPDATE);
+ rtl8xxxu_write8(priv, REG_BEACON_CTRL_1, (bcn_ctrl_1 &
+ (~BEACON_FUNCTION_ENABLE)) | BEACON_DISABLE_TSF_UPDATE);
+
+ /* switch msr */
+ msr = (msr & 0xf0) | ((msr & 0x03) << 2) | ((msr & 0x0c) >> 2);
+ rtl8xxxu_write8(priv, REG_MSR, msr);
+
+ /* write port0 */
+ rtl8xxxu_write8(priv, REG_BEACON_CTRL, bcn_ctrl_1 & ~BEACON_FUNCTION_ENABLE);
+ for (i = 0; i < ARRAY_SIZE(atimwnd_1); i++)
+ rtl8xxxu_write8(priv, REG_ATIMWND + i, atimwnd_1[i]);
+ for (i = 0; i < ARRAY_SIZE(tsftr_1); i++)
+ rtl8xxxu_write8(priv, REG_TSFTR + i, tsftr_1[i]);
+ for (i = 0; i < ARRAY_SIZE(macid_1); i++)
+ rtl8xxxu_write8(priv, REG_MACID + i, macid_1[i]);
+ for (i = 0; i < ARRAY_SIZE(bssid_1); i++)
+ rtl8xxxu_write8(priv, REG_BSSID + i, bssid_1[i]);
+
+ /* write port1 */
+ rtl8xxxu_write8(priv, REG_BEACON_CTRL_1, bcn_ctrl & ~BEACON_FUNCTION_ENABLE);
+ for (i = 0; i < ARRAY_SIZE(atimwnd); i++)
+ rtl8xxxu_write8(priv, REG_ATIMWND_1 + i, atimwnd[i]);
+ for (i = 0; i < ARRAY_SIZE(tsftr); i++)
+ rtl8xxxu_write8(priv, REG_TSFTR1 + i, tsftr[i]);
+ for (i = 0; i < ARRAY_SIZE(macid); i++)
+ rtl8xxxu_write8(priv, REG_MACID1 + i, macid[i]);
+ for (i = 0; i < ARRAY_SIZE(bssid); i++)
+ rtl8xxxu_write8(priv, REG_BSSID1 + i, bssid[i]);
+
+ /* write bcn ctl */
+ rtl8xxxu_write8(priv, REG_BEACON_CTRL, bcn_ctrl_1);
+ rtl8xxxu_write8(priv, REG_BEACON_CTRL_1, bcn_ctrl);
+
+ vif = priv->vifs[0];
+ priv->vifs[0] = priv->vifs[1];
+ priv->vifs[1] = vif;
+ rtlvif = (struct rtl8xxxu_vif *)priv->vifs[1]->drv_priv;
+ rtlvif->port_num = 1;
+}
+
static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
struct ieee80211_vif *vif)
{
@@ -6640,8 +6718,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
}
break;
case NL80211_IFTYPE_AP:
- if (port_num == 1)
- return -EOPNOTSUPP;
+ if (port_num == 1) {
+ rtl8xxxu_switch_ports(priv);
+ port_num = 0;
+ }
rtl8xxxu_write8(priv, REG_BEACON_CTRL,
BEACON_DISABLE_TSF_UPDATE | BEACON_CTRL_MBSSID);
--
2.39.2
> -----Original Message-----
> From: Martin Kaistra <[email protected]>
> Sent: Monday, December 18, 2023 10:37 PM
> To: [email protected]
> Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
> <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej Siewior
> <[email protected]>
> Subject: [PATCH 19/20] wifi: rtl8xxxu: make supporting AP mode only on port 0 transparent
>
> When the driver is used for concurrent mode, both virtual interfaces can
> be set to station or AP mode, though only one can be in AP mode at the
> same time.
>
> In order to keep the code simple, use only hw port 0 for AP mode. When
> an interface is added in AP mode which would be assigned to port 1, use
> a switch_port function to transparently swap the mapping between virtual
> interface and hw port.
>
> Signed-off-by: Martin Kaistra <[email protected]>
> ---
> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 84 ++++++++++++++++++-
> 1 file changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 88730791091a7..595f447874f4d 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -6613,6 +6613,84 @@ static int rtl8xxxu_submit_int_urb(struct ieee80211_hw *hw)
> return ret;
> }
>
> +static void rtl8xxxu_switch_ports(struct rtl8xxxu_priv *priv)
> +{
[...]
> +
> + vif = priv->vifs[0];
> + priv->vifs[0] = priv->vifs[1];
> + priv->vifs[1] = vif;
> + rtlvif = (struct rtl8xxxu_vif *)priv->vifs[1]->drv_priv;
> + rtlvif->port_num = 1;
nit: Would it be better to swap port_num as well? Currently, port_num of vifs[0]
will be set to 0 by caller, but not sure if further people could misuse this
function.
> +}
> +
> static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif)
> {
> @@ -6640,8 +6718,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
> }
> break;
> case NL80211_IFTYPE_AP:
> - if (port_num == 1)
> - return -EOPNOTSUPP;
> + if (port_num == 1) {
> + rtl8xxxu_switch_ports(priv);
> + port_num = 0;
> + }
>
> rtl8xxxu_write8(priv, REG_BEACON_CTRL,
> BEACON_DISABLE_TSF_UPDATE | BEACON_CTRL_MBSSID);
> --
> 2.39.2
Am 20.12.23 um 07:28 schrieb Ping-Ke Shih:
>
>
>> -----Original Message-----
>> From: Martin Kaistra <[email protected]>
>> Sent: Monday, December 18, 2023 10:37 PM
>> To: [email protected]
>> Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
>> <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej Siewior
>> <[email protected]>
>> Subject: [PATCH 19/20] wifi: rtl8xxxu: make supporting AP mode only on port 0 transparent
>>
>> When the driver is used for concurrent mode, both virtual interfaces can
>> be set to station or AP mode, though only one can be in AP mode at the
>> same time.
>>
>> In order to keep the code simple, use only hw port 0 for AP mode. When
>> an interface is added in AP mode which would be assigned to port 1, use
>> a switch_port function to transparently swap the mapping between virtual
>> interface and hw port.
>>
>> Signed-off-by: Martin Kaistra <[email protected]>
>> ---
>> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 84 ++++++++++++++++++-
>> 1 file changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index 88730791091a7..595f447874f4d 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -6613,6 +6613,84 @@ static int rtl8xxxu_submit_int_urb(struct ieee80211_hw *hw)
>> return ret;
>> }
>>
>> +static void rtl8xxxu_switch_ports(struct rtl8xxxu_priv *priv)
>> +{
>
> [...]
>
>> +
>> + vif = priv->vifs[0];
>> + priv->vifs[0] = priv->vifs[1];
>> + priv->vifs[1] = vif;
>> + rtlvif = (struct rtl8xxxu_vif *)priv->vifs[1]->drv_priv;
>> + rtlvif->port_num = 1;
>
> nit: Would it be better to swap port_num as well? Currently, port_num of vifs[0]
> will be set to 0 by caller, but not sure if further people could misuse this
> function.
the main reason, I did not include setting port_num for priv->vifs[0], is that
priv->vifs[0] is a NULL pointer in the current way this function is called from
rtl8xxxu_add_interface().
do you think it makes sense to add
if (priv->vifs[0])
rtlvif = (struct rtl8xxxu_vif *)priv->vifs[0]->drv_priv;
rtlvif->port_num = 0;
just for completeness sake, even though this code path will currently never get
executed?
>
>
>> +}
>> +
>> static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>> struct ieee80211_vif *vif)
>> {
>> @@ -6640,8 +6718,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>> }
>> break;
>> case NL80211_IFTYPE_AP:
>> - if (port_num == 1)
>> - return -EOPNOTSUPP;
>> + if (port_num == 1) {
>> + rtl8xxxu_switch_ports(priv);
>> + port_num = 0;
>> + }
>>
>> rtl8xxxu_write8(priv, REG_BEACON_CTRL,
>> BEACON_DISABLE_TSF_UPDATE | BEACON_CTRL_MBSSID);
>> --
>> 2.39.2
>
On Wed, 2023-12-20 at 17:34 +0100, Martin Kaistra wrote:
> External mail.
>
>
>
> Am 20.12.23 um 07:28 schrieb Ping-Ke Shih:
> >
> > > -----Original Message-----
> > > From: Martin Kaistra <[email protected]>
> > > Sent: Monday, December 18, 2023 10:37 PM
> > > To: [email protected]
> > > Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
> > > <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej Siewior
> > > <[email protected]>
> > > Subject: [PATCH 19/20] wifi: rtl8xxxu: make supporting AP mode only on port 0 transparent
> > >
> > > +
> > > + vif = priv->vifs[0];
> > > + priv->vifs[0] = priv->vifs[1];
> > > + priv->vifs[1] = vif;
> > > + rtlvif = (struct rtl8xxxu_vif *)priv->vifs[1]->drv_priv;
> > > + rtlvif->port_num = 1;
> >
> > nit: Would it be better to swap port_num as well? Currently, port_num of vifs[0]
> > will be set to 0 by caller, but not sure if further people could misuse this
> > function.
>
> the main reason, I did not include setting port_num for priv->vifs[0], is that
> priv->vifs[0] is a NULL pointer in the current way this function is called from
> rtl8xxxu_add_interface().
>
> do you think it makes sense to add
>
> if (priv->vifs[0])
> rtlvif = (struct rtl8xxxu_vif *)priv->vifs[0]->drv_priv;
> rtlvif->port_num = 0;
>
> just for completeness sake, even though this code path will currently never get
> executed?
>
I missed that point. I just did focus on "switch", but actually this is
"move" from port 0 to 1, right?
As I know, two cases only work on port 0. One is AP mode that you are
doing, and the other is PS in STA mode that isn't implemented by rtl8xxxxu.
For AP mode, current implement moving port 0 to 1 is fine.
In the future, PS in STA mode could need to move port 1 to 0, because
we may disconnect port 0 first and we want STA on port 1 entering PS.
But, I think we can defer this until we really need it.
Am 21.12.23 um 09:25 schrieb Ping-Ke Shih:
> On Wed, 2023-12-20 at 17:34 +0100, Martin Kaistra wrote:
>> External mail.
>>
>>
>>
>> Am 20.12.23 um 07:28 schrieb Ping-Ke Shih:
>>>
>>>> -----Original Message-----
>>>> From: Martin Kaistra <[email protected]>
>>>> Sent: Monday, December 18, 2023 10:37 PM
>>>> To: [email protected]
>>>> Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
>>>> <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej Siewior
>>>> <[email protected]>
>>>> Subject: [PATCH 19/20] wifi: rtl8xxxu: make supporting AP mode only on port 0 transparent
>>>>
>>>> +
>>>> + vif = priv->vifs[0];
>>>> + priv->vifs[0] = priv->vifs[1];
>>>> + priv->vifs[1] = vif;
>>>> + rtlvif = (struct rtl8xxxu_vif *)priv->vifs[1]->drv_priv;
>>>> + rtlvif->port_num = 1;
>>>
>>> nit: Would it be better to swap port_num as well? Currently, port_num of vifs[0]
>>> will be set to 0 by caller, but not sure if further people could misuse this
>>> function.
>>
>> the main reason, I did not include setting port_num for priv->vifs[0], is that
>> priv->vifs[0] is a NULL pointer in the current way this function is called from
>> rtl8xxxu_add_interface().
>>
>> do you think it makes sense to add
>>
>> if (priv->vifs[0])
>> rtlvif = (struct rtl8xxxu_vif *)priv->vifs[0]->drv_priv;
>> rtlvif->port_num = 0;
>>
>> just for completeness sake, even though this code path will currently never get
>> executed?
>>
>
> I missed that point. I just did focus on "switch", but actually this is
> "move" from port 0 to 1, right?
Yes, currently, the function is only used to move the STA mode interface from 0
to 1 in order to make room for AP on 0.
I will leave this patch as is for v2. When the function is used in the future
for a different scenario, the possibility of vifs[0] or vifs[1] being NULL needs
to be thought through anyway and if necessary the setting of port_num = 0 can be
added then as well.
>
> As I know, two cases only work on port 0. One is AP mode that you are
> doing, and the other is PS in STA mode that isn't implemented by rtl8xxxxu.
> For AP mode, current implement moving port 0 to 1 is fine.
>
> In the future, PS in STA mode could need to move port 1 to 0, because
> we may disconnect port 0 first and we want STA on port 1 entering PS.
>
> But, I think we can defer this until we really need it.
>
>
On Thu, 2023-12-21 at 11:54 +0100, Martin Kaistra wrote:
>
> Am 21.12.23 um 09:25 schrieb Ping-Ke Shih:
> > On Wed, 2023-12-20 at 17:34 +0100, Martin Kaistra wrote:
> > >
> > > Am 20.12.23 um 07:28 schrieb Ping-Ke Shih:
> > > > > -----Original Message-----
> > > > > From: Martin Kaistra <[email protected]>
> > > > > Sent: Monday, December 18, 2023 10:37 PM
> > > > > To: [email protected]
> > > > > Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
> > > > > <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej
> > > > > Siewior
> > > > > <[email protected]>
> > > > > Subject: [PATCH 19/20] wifi: rtl8xxxu: make supporting AP mode only on port 0 transparent
> > > > >
> > > > > +
> > > > > + vif = priv->vifs[0];
> > > > > + priv->vifs[0] = priv->vifs[1];
> > > > > + priv->vifs[1] = vif;
> > > > > + rtlvif = (struct rtl8xxxu_vif *)priv->vifs[1]->drv_priv;
> > > > > + rtlvif->port_num = 1;
> > > >
> > > > nit: Would it be better to swap port_num as well? Currently, port_num of vifs[0]
> > > > will be set to 0 by caller, but not sure if further people could misuse this
> > > > function.
> > >
> > > the main reason, I did not include setting port_num for priv->vifs[0], is that
> > > priv->vifs[0] is a NULL pointer in the current way this function is called from
> > > rtl8xxxu_add_interface().
> > >
> > > do you think it makes sense to add
> > >
> > > if (priv->vifs[0])
> > > rtlvif = (struct rtl8xxxu_vif *)priv->vifs[0]->drv_priv;
> > > rtlvif->port_num = 0;
> > >
> > > just for completeness sake, even though this code path will currently never get
> > > executed?
> > >
> >
> > I missed that point. I just did focus on "switch", but actually this is
> > "move" from port 0 to 1, right?
>
> Yes, currently, the function is only used to move the STA mode interface from 0
> to 1 in order to make room for AP on 0.
>
> I will leave this patch as is for v2. When the function is used in the future
> for a different scenario, the possibility of vifs[0] or vifs[1] being NULL needs
> to be thought through anyway and if necessary the setting of port_num = 0 can be
> added then as well.
>
Would you like to add a comment like above description to help people to
understand your thinking?
Anyway, this patch looks good to me. Please add my reviewed-by by v2.