Hi Chris,
On Tue, May 28, 2019 at 11:03 PM Chris Chiu <[email protected]> wrote:
> + /*
> + * Single virtual interface permitted since the driver supports STATION
> + * mode only.
I think you can be a bit more explicit by saying e.g.:
Only one virtual interface permitted because only STA mode is
supported and no iface_combinations are provided.
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 039e5ca9d2e4..2d612c2df5b2 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4345,7 +4345,7 @@ void rtl8xxxu_gen2_update_rate_mask(struct rtl8xxxu_priv *priv,
> h2c.b_macid_cfg.ramask3 = (ramask >> 24) & 0xff;
>
> h2c.ramask.arg = 0x80;
> - h2c.b_macid_cfg.data1 = 0;
> + h2c.b_macid_cfg.data1 = priv->ratr_index;
I think ratr_index can be moved to be a function parameter of the
update_rate_mask function. It looks like all callsites already know
which value they want to set. Then you don't have to store it in the
priv structure.
> @@ -5471,6 +5509,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>
> switch (vif->type) {
> case NL80211_IFTYPE_STATION:
> + if (!priv->vif)
> + priv->vif = vif;
> + else
> + return -EOPNOTSUPP;
> rtl8xxxu_stop_tx_beacon(priv);
rtl8xxxu_remove_interface should also set priv->vif back to NULL.
> @@ -6183,6 +6259,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> mutex_destroy(&priv->usb_buf_mutex);
> mutex_destroy(&priv->h2c_mutex);
>
> + cancel_delayed_work_sync(&priv->ra_watchdog);
Given that the work was started in rtl8xxxu_start, I think it should
be cancelled in rtl8xxxu_stop() instead.
Daniel
On Thu, May 30, 2019 at 2:12 AM Daniel Drake <[email protected]> wrote:
>
> Hi Chris,
>
> On Tue, May 28, 2019 at 11:03 PM Chris Chiu <[email protected]> wrote:
> > + /*
> > + * Single virtual interface permitted since the driver supports STATION
> > + * mode only.
>
> I think you can be a bit more explicit by saying e.g.:
>
> Only one virtual interface permitted because only STA mode is
> supported and no iface_combinations are provided.
>
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > index 039e5ca9d2e4..2d612c2df5b2 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > @@ -4345,7 +4345,7 @@ void rtl8xxxu_gen2_update_rate_mask(struct rtl8xxxu_priv *priv,
> > h2c.b_macid_cfg.ramask3 = (ramask >> 24) & 0xff;
> >
> > h2c.ramask.arg = 0x80;
> > - h2c.b_macid_cfg.data1 = 0;
> > + h2c.b_macid_cfg.data1 = priv->ratr_index;
>
> I think ratr_index can be moved to be a function parameter of the
> update_rate_mask function. It looks like all callsites already know
> which value they want to set. Then you don't have to store it in the
> priv structure.
>
You mean moving the ratr_index to be the 4th function parameter of
update_rate_mask which has 2 candidates rtl8xxxu_update_rate_mask
and rtl8xxxu_gen2_update_rate_mask? I was planning to keep the
rtl8xxxu_update_rate_mask the same because old chips seems don't
need the rate index when invoking H2C command to change rate mask.
And rate index is not a common phrase/term for rate adaptive. Theoretically
we just need packet error rate, sgi and other factors to determine the rate
mask. This rate index seems to be only specific to newer Realtek drivers
or firmware for rate adaptive algorithm. I'd like to keep this for gen2 but
I admit it's ugly to put it in the priv structure. Any suggestion is
appreciated.
Thanks
> > @@ -5471,6 +5509,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
> >
> > switch (vif->type) {
> > case NL80211_IFTYPE_STATION:
> > + if (!priv->vif)
> > + priv->vif = vif;
> > + else
> > + return -EOPNOTSUPP;
> > rtl8xxxu_stop_tx_beacon(priv);
>
> rtl8xxxu_remove_interface should also set priv->vif back to NULL.
>
> > @@ -6183,6 +6259,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> > mutex_destroy(&priv->usb_buf_mutex);
> > mutex_destroy(&priv->h2c_mutex);
> >
> > + cancel_delayed_work_sync(&priv->ra_watchdog);
>
> Given that the work was started in rtl8xxxu_start, I think it should
> be cancelled in rtl8xxxu_stop() instead.
>
> Daniel