| Message ID | 1500446187-8034-1-git-send-email-huxinming820@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote: > From: Xinming Hu <huxm@marvell.com> > > Add module parameter to control tdls uapsd support, which is > default disabled. Why default disabled, when it looks like it used to be on by default?
Hi Brian, > -----Original Message----- > From: Brian Norris [mailto:briannorris@chromium.org] > Sent: 2017年7月20日 4:10 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@google.com; Zhiyuan > Yang; Tim Song; Cathy Luo; Xinming Hu > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module parameter > > External Email > > ---------------------------------------------------------------------- > On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote: > > From: Xinming Hu <huxm@marvell.com> > > > > Add module parameter to control tdls uapsd support, which is default > > disabled. > > Why default disabled, when it looks like it used to be on by default? Oho, I should comment more in description, it is now confusing. We just fixed an tdls uapsd issue in latest firmware, so try to disable this feature as a workaround to the old firmware. At the same time, it is optional to enable this feature in specific case. I will add more comments in V2. Please let us know if you have more comments. Regards, Simon
Hi, On Thu, Jul 20, 2017 at 10:54:16AM +0000, Xinming Hu wrote: > Hi Brian, > > > -----Original Message----- > > From: Brian Norris [mailto:briannorris@chromium.org] > > Sent: 2017年7月20日 4:10 > > To: Xinming Hu > > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@google.com; Zhiyuan > > Yang; Tim Song; Cathy Luo; Xinming Hu > > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module parameter > > > > External Email > > > > ---------------------------------------------------------------------- > > On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote: > > > From: Xinming Hu <huxm@marvell.com> > > > > > > Add module parameter to control tdls uapsd support, which is default > > > disabled. > > > > Why default disabled, when it looks like it used to be on by default? > > Oho, I should comment more in description, it is now confusing. > We just fixed an tdls uapsd issue in latest firmware, so try to disable > this feature as a workaround to the old firmware. At the same time, > it is optional to enable this feature in specific case. That helps a bit, thanks. > I will add more comments in V2. > Please let us know if you have more comments. I won't insist on changes, but for something like this, it seems reasonable (if you have really fixed the issue in the latest firmware) to just provide the knob to disable as a backup, not as a default. If someone is going to update their kernel (to include this patch), but not update their firmware, then they probably should know enough to be able to provide the module parameter to disable. Or alternatively: how is anyone supposed to know whether their current firmware is broken or not? And how is this feature ever going to be default-enabled again? New chipsets with new firmware should hopefully not have the same bugs, no? Brian
On Thu, Jul 20, 2017 at 10:16 AM, Brian Norris <briannorris@chromium.org> wrote: > Hi, > > On Thu, Jul 20, 2017 at 10:54:16AM +0000, Xinming Hu wrote: >> Hi Brian, >> >> > -----Original Message----- >> > From: Brian Norris [mailto:briannorris@chromium.org] >> > Sent: 2017年7月20日 4:10 >> > To: Xinming Hu >> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@google.com; Zhiyuan >> > Yang; Tim Song; Cathy Luo; Xinming Hu >> > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module parameter >> > >> > External Email >> > >> > ---------------------------------------------------------------------- >> > On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote: >> > > From: Xinming Hu <huxm@marvell.com> >> > > >> > > Add module parameter to control tdls uapsd support, which is default >> > > disabled. >> > >> > Why default disabled, when it looks like it used to be on by default? >> >> Oho, I should comment more in description, it is now confusing. >> We just fixed an tdls uapsd issue in latest firmware, so try to disable >> this feature as a workaround to the old firmware. At the same time, >> it is optional to enable this feature in specific case. > > That helps a bit, thanks. > >> I will add more comments in V2. >> Please let us know if you have more comments. > > I won't insist on changes, but for something like this, it seems > reasonable (if you have really fixed the issue in the latest firmware) > to just provide the knob to disable as a backup, not as a default. If > someone is going to update their kernel (to include this patch), but not > update their firmware, then they probably should know enough to be able > to provide the module parameter to disable. > > Or alternatively: how is anyone supposed to know whether their current > firmware is broken or not? And how is this feature ever going to be > default-enabled again? New chipsets with new firmware should hopefully > not have the same bugs, no? Better yet, the driver could check the firmware version, and if it is known bad disable the feature automatically. Then there is no need to provide this knob. Thanks.
Hi, > -----Original Message----- > From: Dmitry Torokhov [mailto:dtor@google.com] > Sent: 2017年7月21日 1:29 > To: Brian Norris > Cc: Xinming Hu; Xinming Hu; Ganapathi Bhat; Linux Wireless; Kalle Valo; > rajatja@google.com; Zhiyuan Yang; Tim Song; Cathy Luo > Subject: [EXT] Re: Re: [PATCH] mwifiex: add tdls uapsd support module > parameter > > External Email > > ---------------------------------------------------------------------- > On Thu, Jul 20, 2017 at 10:16 AM, Brian Norris <briannorris@chromium.org> > wrote: > > Hi, > > > > On Thu, Jul 20, 2017 at 10:54:16AM +0000, Xinming Hu wrote: > >> Hi Brian, > >> > >> > -----Original Message----- > >> > From: Brian Norris [mailto:briannorris@chromium.org] > >> > Sent: 2017年7月20日 4:10 > >> > To: Xinming Hu > >> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; > >> > rajatja@google.com; Zhiyuan Yang; Tim Song; Cathy Luo; Xinming Hu > >> > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module > >> > parameter > >> > > >> > External Email > >> > > >> > ------------------------------------------------------------------- > >> > --- On Wed, Jul 19, 2017 at 06:36:27AM +0000, Xinming Hu wrote: > >> > > From: Xinming Hu <huxm@marvell.com> > >> > > > >> > > Add module parameter to control tdls uapsd support, which is > >> > > default disabled. > >> > > >> > Why default disabled, when it looks like it used to be on by default? > >> > >> Oho, I should comment more in description, it is now confusing. > >> We just fixed an tdls uapsd issue in latest firmware, so try to > >> disable this feature as a workaround to the old firmware. At the same > >> time, it is optional to enable this feature in specific case. > > > > That helps a bit, thanks. > > > >> I will add more comments in V2. > >> Please let us know if you have more comments. > > > > I won't insist on changes, but for something like this, it seems > > reasonable (if you have really fixed the issue in the latest firmware) > > to just provide the knob to disable as a backup, not as a default. If > > someone is going to update their kernel (to include this patch), but > > not update their firmware, then they probably should know enough to be > > able to provide the module parameter to disable. > > > > Or alternatively: how is anyone supposed to know whether their current > > firmware is broken or not? And how is this feature ever going to be > > default-enabled again? New chipsets with new firmware should hopefully > > not have the same bugs, no? > > Better yet, the driver could check the firmware version, and if it is known bad > disable the feature automatically. Then there is no need to provide this knob. > Thanks for all the comments. I deed misuse the moduld parameter. The tdls upasd capability have already been disabled during tdls setup. While here we are trying to rely on tdls peer uapsd capability in tdls configuration. So we can simply disable it in tdls config to keep it match with our default capability. Sorry for the confusing in this patch. I have sent the replace version for review. Regards, Simon > Thanks. > > -- > Dmitry
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c index 3ecb59f..2cc8e54 100644 --- a/drivers/net/wireless/marvell/mwifiex/init.c +++ b/drivers/net/wireless/marvell/mwifiex/init.c @@ -25,6 +25,10 @@ #include "wmm.h" #include "11n.h" +static bool tdls_uapsd; +module_param(tdls_uapsd, bool, 0000); +MODULE_PARM_DESC(tdls_uapsd, "tdls uapsd support enable:1, disable:0"); + /* * This function adds a BSS priority table to the table list. * @@ -154,6 +158,7 @@ int mwifiex_init_priv(struct mwifiex_private *priv) priv->del_list_idx = 0; priv->hs2_enabled = false; priv->check_tdls_tx = false; + priv->tdls_uapsd_support = tdls_uapsd; memcpy(priv->tos_to_tid_inv, tos_to_tid_inv, MAX_NUM_TID); mwifiex_init_11h_params(priv); diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index f8cf307..ef5eac72 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -660,6 +660,7 @@ struct mwifiex_private { u8 check_tdls_tx; struct timer_list auto_tdls_timer; bool auto_tdls_timer_active; + u8 tdls_uapsd_support; struct idr ack_status_frames; /* spin lock for ack status */ spinlock_t ack_status_lock; diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c index 534d94a..d5da565 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -1789,9 +1789,11 @@ static int mwifiex_cmd_chan_region_cfg(struct mwifiex_private *priv, put_unaligned_le16(params->capability, pos); config_len += sizeof(params->capability); - qos_info = params->uapsd_queues | (params->max_sp << 5); - wmm_qos_info = (struct mwifiex_ie_types_qos_info *)(pos + - config_len); + if (priv->tdls_uapsd_support) + qos_info = params->uapsd_queues | (params->max_sp << 5); + else + qos_info = 0; + wmm_qos_info = (void *)(pos + config_len); wmm_qos_info->header.type = cpu_to_le16(WLAN_EID_QOS_CAPA); wmm_qos_info->header.len = cpu_to_le16(sizeof(qos_info)); wmm_qos_info->qos_info = qos_info;