Hi, > -----Original Message----- > From: Dmitry Torokhov [mailto:dtor@xxxxxxxxxx] > Sent: 2017年7月21日 1:29 > To: Brian Norris > Cc: Xinming Hu; Xinming Hu; Ganapathi Bhat; Linux Wireless; Kalle Valo; > rajatja@xxxxxxxxxx; 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@xxxxxxxxxxxx> > wrote: > > Hi, > > > > On Thu, Jul 20, 2017 at 10:54:16AM +0000, Xinming Hu wrote: > >> Hi Brian, > >> > >> > -----Original Message----- > >> > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > >> > Sent: 2017年7月20日 4:10 > >> > To: Xinming Hu > >> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; > >> > rajatja@xxxxxxxxxx; 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@xxxxxxxxxxx> > >> > > > >> > > 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
