All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
@ 2018-04-20  2:30 pkshih
  2018-04-20 12:01 ` Kalle Valo
  2018-04-24 10:16 ` [v2] " Kalle Valo
  0 siblings, 2 replies; 8+ messages in thread
From: pkshih @ 2018-04-20  2:30 UTC (permalink / raw)
  To: kvalo; +Cc: Larry.Finger, linux-wireless, stable

From: Ping-Ke Shih <pkshih@realtek.com>

The module parameter ant_sel is used to control antenna number and path.
There is an existing enum ANT_{X2,X1} defined the antenna number, so
add a new enum ANT_{MAIN,AUX} to make it readable. After this work,
incorrect given values depend on ant_sel were exposed, so refill values
according following definition:
  ant_sel   ant_num   ant_path  print_label
     1      ANT_X1    ANT_AUX        #2
     2      ANT_X2    ANT_MAIN       #1
Then, a workaround resulted from the incorrect values in halbtcoutsrc.c was
removed. These is a existing bug in the workaround while ant_sel=2, but the
case is rare use so user is hard to observe this bug.

The experimental results with single antenna connected to specific path
are in following:
  ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
     0        -8            -62
     1        -62           -10
     2        -6            -60

Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Cc: Stable <stable@vger.kernel.org> # 4.7+
Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>
---
v2: Add more description about fixed bugs in end of first paragraph.

---
 .../net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c | 15 ---------------
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c       | 11 +++++++----
 drivers/net/wireless/realtek/rtlwifi/wifi.h               |  5 +++++
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
index e0f9985582f9..738a7ea7dc4a 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
@@ -158,16 +158,6 @@ static u8 halbtc_get_wifi_central_chnl(struct btc_coexist *btcoexist)
 
 static u8 rtl_get_hwpg_single_ant_path(struct rtl_priv *rtlpriv)
 {
-	struct rtl_mod_params *mod_params = rtlpriv->cfg->mod_params;
-
-	/* override ant_num / ant_path */
-	if (mod_params->ant_sel) {
-		rtlpriv->btcoexist.btc_info.ant_num =
-			(mod_params->ant_sel == 1 ? ANT_X2 : ANT_X1);
-
-		rtlpriv->btcoexist.btc_info.single_ant_path =
-			(mod_params->ant_sel == 1 ? 0 : 1);
-	}
 	return rtlpriv->btcoexist.btc_info.single_ant_path;
 }
 
@@ -178,7 +168,6 @@ static u8 rtl_get_hwpg_bt_type(struct rtl_priv *rtlpriv)
 
 static u8 rtl_get_hwpg_ant_num(struct rtl_priv *rtlpriv)
 {
-	struct rtl_mod_params *mod_params = rtlpriv->cfg->mod_params;
 	u8 num;
 
 	if (rtlpriv->btcoexist.btc_info.ant_num == ANT_X2)
@@ -186,10 +175,6 @@ static u8 rtl_get_hwpg_ant_num(struct rtl_priv *rtlpriv)
 	else
 		num = 1;
 
-	/* override ant_num / ant_path */
-	if (mod_params->ant_sel)
-		num = (mod_params->ant_sel == 1 ? ANT_X2 : ANT_X1) + 1;
-
 	return num;
 }
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
index e7bbbc95cdb1..b4f3f91b590e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
@@ -848,6 +848,9 @@ static bool _rtl8723be_init_mac(struct ieee80211_hw *hw)
 		return false;
 	}
 
+	if (rtlpriv->cfg->ops->get_btc_status())
+		rtlpriv->btcoexist.btc_ops->btc_power_on_setting(rtlpriv);
+
 	bytetmp = rtl_read_byte(rtlpriv, REG_MULTI_FUNC_CTRL);
 	rtl_write_byte(rtlpriv, REG_MULTI_FUNC_CTRL, bytetmp | BIT(3));
 
@@ -2696,21 +2699,21 @@ void rtl8723be_read_bt_coexist_info_from_hwpg(struct ieee80211_hw *hw,
 		rtlpriv->btcoexist.btc_info.bt_type = BT_RTL8723B;
 		rtlpriv->btcoexist.btc_info.ant_num = (value & 0x1);
 		rtlpriv->btcoexist.btc_info.single_ant_path =
-			 (value & 0x40);	/*0xc3[6]*/
+			 (value & 0x40 ? ANT_AUX : ANT_MAIN);	/*0xc3[6]*/
 	} else {
 		rtlpriv->btcoexist.btc_info.btcoexist = 0;
 		rtlpriv->btcoexist.btc_info.bt_type = BT_RTL8723B;
 		rtlpriv->btcoexist.btc_info.ant_num = ANT_X2;
-		rtlpriv->btcoexist.btc_info.single_ant_path = 0;
+		rtlpriv->btcoexist.btc_info.single_ant_path = ANT_MAIN;
 	}
 
 	/* override ant_num / ant_path */
 	if (mod_params->ant_sel) {
 		rtlpriv->btcoexist.btc_info.ant_num =
-			(mod_params->ant_sel == 1 ? ANT_X2 : ANT_X1);
+			(mod_params->ant_sel == 1 ? ANT_X1 : ANT_X2);
 
 		rtlpriv->btcoexist.btc_info.single_ant_path =
-			(mod_params->ant_sel == 1 ? 0 : 1);
+			(mod_params->ant_sel == 1 ? ANT_AUX : ANT_MAIN);
 	}
 }
 
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index c32985cfe48d..ab32a62ead81 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -2882,6 +2882,11 @@ enum bt_ant_num {
 	ANT_X1 = 1,
 };
 
+enum bt_ant_path {
+	ANT_MAIN = 0,
+	ANT_AUX = 1,
+};
+
 enum bt_co_type {
 	BT_2WIRE = 0,
 	BT_ISSC_3WIRE = 1,
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
  2018-04-20  2:30 [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition pkshih
@ 2018-04-20 12:01 ` Kalle Valo
  2018-04-20 20:56   ` Larry Finger
  2018-04-24 10:16 ` [v2] " Kalle Valo
  1 sibling, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2018-04-20 12:01 UTC (permalink / raw)
  To: pkshih; +Cc: Larry.Finger, linux-wireless, stable

<pkshih@realtek.com> writes:

> From: Ping-Ke Shih <pkshih@realtek.com>
>
> The module parameter ant_sel is used to control antenna number and path.
> There is an existing enum ANT_{X2,X1} defined the antenna number, so
> add a new enum ANT_{MAIN,AUX} to make it readable. After this work,
> incorrect given values depend on ant_sel were exposed, so refill values
> according following definition:
>   ant_sel   ant_num   ant_path  print_label
>      1      ANT_X1    ANT_AUX        #2
>      2      ANT_X2    ANT_MAIN       #1
> Then, a workaround resulted from the incorrect values in halbtcoutsrc.c was
> removed. These is a existing bug in the workaround while ant_sel=2, but the
> case is rare use so user is hard to observe this bug.
>
> The experimental results with single antenna connected to specific path
> are in following:
>   ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>      0        -8            -62
>      1        -62           -10
>      2        -6            -60
>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> Cc: Stable <stable@vger.kernel.org> # 4.7+
> Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> v2: Add more description about fixed bugs in end of first paragraph.

Sorry, I still don't understand the bug you are fixing. It shouldn't
take more than one minute to understand a commit log.

I prose that you rewrite the commit log, or at least parts of if it.
When you are describing a bug don't talk about enums or source files,
that's an implementation detail, and instead talk how the bug looks like
from users point of view. For example:

  "On HP laptop model 1234 with Realtex 4321 device users are supposed
   to use ant_sel module parameter with value 77 to use the correct
   antenna. But instead rtlwifi incorrectly chose antenna 88 with lower
   transmit power and that caused packet loss. Fix it so that the user
   gets better transmit power and..."

(that's of course all made up information as I don't know what the
actual bug is)

And after that you can write rtlwifi internals in the commit log as much
as you want :) But there needs to be a clear generic description of the
bug first and it needs to understandable in one read.

To make this faster I propose that you send the new commit log as a
reply to this mail and I can then comment faster.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
  2018-04-20 12:01 ` Kalle Valo
@ 2018-04-20 20:56   ` Larry Finger
  2018-04-23  2:19       ` Pkshih
  2018-04-23 13:47     ` Kalle Valo
  0 siblings, 2 replies; 8+ messages in thread
From: Larry Finger @ 2018-04-20 20:56 UTC (permalink / raw)
  To: Kalle Valo, pkshih; +Cc: linux-wireless, stable

On 04/20/2018 07:01 AM, Kalle Valo wrote:
> <pkshih@realtek.com> writes:
> 
>> From: Ping-Ke Shih <pkshih@realtek.com>
>>
>> The module parameter ant_sel is used to control antenna number and path.
>> There is an existing enum ANT_{X2,X1} defined the antenna number, so
>> add a new enum ANT_{MAIN,AUX} to make it readable. After this work,
>> incorrect given values depend on ant_sel were exposed, so refill values
>> according following definition:
>>    ant_sel   ant_num   ant_path  print_label
>>       1      ANT_X1    ANT_AUX        #2
>>       2      ANT_X2    ANT_MAIN       #1
>> Then, a workaround resulted from the incorrect values in halbtcoutsrc.c was
>> removed. These is a existing bug in the workaround while ant_sel=2, but the
>> case is rare use so user is hard to observe this bug.
>>
>> The experimental results with single antenna connected to specific path
>> are in following:
>>    ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>>       0        -8            -62
>>       1        -62           -10
>>       2        -6            -60
>>
>> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>> Cc: Stable <stable@vger.kernel.org> # 4.7+
>> Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>
>> ---
>> v2: Add more description about fixed bugs in end of first paragraph.
> 
> Sorry, I still don't understand the bug you are fixing. It shouldn't
> take more than one minute to understand a commit log.
> 
> I prose that you rewrite the commit log, or at least parts of if it.
> When you are describing a bug don't talk about enums or source files,
> that's an implementation detail, and instead talk how the bug looks like
> from users point of view. For example:
> 
>    "On HP laptop model 1234 with Realtex 4321 device users are supposed
>     to use ant_sel module parameter with value 77 to use the correct
>     antenna. But instead rtlwifi incorrectly chose antenna 88 with lower
>     transmit power and that caused packet loss. Fix it so that the user
>     gets better transmit power and..."
> 
> (that's of course all made up information as I don't know what the
> actual bug is)
> 
> And after that you can write rtlwifi internals in the commit log as much
> as you want :) But there needs to be a clear generic description of the
> bug first and it needs to understandable in one read.
> 
> To make this faster I propose that you send the new commit log as a
> reply to this mail and I can then comment faster.

Kalle,

As I have some responsibility in creating this mess, let me try to write a new 
commit log. I hope this answers your questions.

Thanks,

Larry

====================================

Some HP laptops have only a single wifi antenna. This would not be a problem 
except that they were shipped with an incorrectly encoded EFUSE. It should have 
been possible to open the computer and transfer the antenna connection to the 
other terminal except that such action might void the warranty, and moving the 
antenna broke the Windows driver. The fix was to add a module option that would 
override the EFUSE encoding. That was done with commit c18d8f509571 ("rtlwifi: 
rtl8723be: Add antenna select module parameter"). There was still a problem with 
Bluetooth coexistence, which was addressed with commit baa170229095 ("rtlwifi: 
btcoexist: Implement antenna selection"). There were still problems, thus there 
were commit 0ff78adeef11 ("rtlwifi: rtl8723be: fix ant_sel code") and commit 
6d6226928369 ("rtlwifi: btcoexist: Fix antenna selection code"). Despite all 
these attempts at fixing the problem, the code is not yet right. A proper fix is 
important as there are now instances of laptops having RTL8723DE chips with the 
same problem.

The module parameter ant_sel is used to control antenna number and path.
At present enum ANT_{X2,X1} is used to define the antenna number, but this 
choice is not intuitive, thus change to a new enum ANT_{MAIN,AUX} to make it 
more readable. This change showed examples where incorrect values were used. It 
was also possible to remove a workaround in halbtcoutsrc.c.

The experimental results with single antenna connected to specific path
are now as follows:
   ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
      0        -8            -62
      1        -62           -10
      2        -6            -60

Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Fixes: c18d8f509571 ("rtlwifi: rtl8723be: Add antenna select module parameter")
Fixes: baa170229095 ("rtlwifi: btcoexist: Implement antenna selection")
Fixes: 0ff78adeef11 ("rtlwifi: rtl8723be: fix ant_sel code")
Fixes: 6d6226928369 ("rtlwifi: btcoexist: Fix antenna selection code")
Cc: Stable <stable@vger.kernel.org> # 4.7+
Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
  2018-04-20 20:56   ` Larry Finger
@ 2018-04-23  2:19       ` Pkshih
  2018-04-23 13:47     ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Pkshih @ 2018-04-23  2:19 UTC (permalink / raw)
  To: kvalo, Larry.Finger; +Cc: linux-wireless, stable

VGhhbmtzIGZvciBzcGVuZGluZyB5b3VyIHRpbWUgdG8gdGVhY2ggYW5kIGhlbHAsIEthbGxlIGFu
ZCBMYXJyeS4NCkknbGwgcmVjb3JkIHRoaXMgZXhhbXBsZSB0byBpbnRlcm5hbCB3aGl0ZSBib2Fy
ZC4NCg0KUEsNCg0K

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
@ 2018-04-23  2:19       ` Pkshih
  0 siblings, 0 replies; 8+ messages in thread
From: Pkshih @ 2018-04-23  2:19 UTC (permalink / raw)
  To: kvalo, Larry.Finger; +Cc: linux-wireless, stable

Thanks for spending your time to teach and help, Kalle and Larry.
I'll record this example to internal white board.

PK


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
  2018-04-20 20:56   ` Larry Finger
  2018-04-23  2:19       ` Pkshih
@ 2018-04-23 13:47     ` Kalle Valo
  2018-04-23 16:09       ` Larry Finger
  1 sibling, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2018-04-23 13:47 UTC (permalink / raw)
  To: Larry Finger; +Cc: pkshih, linux-wireless, stable

Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 04/20/2018 07:01 AM, Kalle Valo wrote:
>> <pkshih@realtek.com> writes:
>>
>>> From: Ping-Ke Shih <pkshih@realtek.com>
>>>
>>> The module parameter ant_sel is used to control antenna number and path.
>>> There is an existing enum ANT_{X2,X1} defined the antenna number, so
>>> add a new enum ANT_{MAIN,AUX} to make it readable. After this work,
>>> incorrect given values depend on ant_sel were exposed, so refill values
>>> according following definition:
>>>    ant_sel   ant_num   ant_path  print_label
>>>       1      ANT_X1    ANT_AUX        #2
>>>       2      ANT_X2    ANT_MAIN       #1
>>> Then, a workaround resulted from the incorrect values in halbtcoutsrc.c was
>>> removed. These is a existing bug in the workaround while ant_sel=2, but the
>>> case is rare use so user is hard to observe this bug.
>>>
>>> The experimental results with single antenna connected to specific path
>>> are in following:
>>>    ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>>>       0        -8            -62
>>>       1        -62           -10
>>>       2        -6            -60
>>>
>>> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>>> Cc: Stable <stable@vger.kernel.org> # 4.7+
>>> Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>
>>> ---
>>> v2: Add more description about fixed bugs in end of first paragraph.
>>
>> Sorry, I still don't understand the bug you are fixing. It shouldn't
>> take more than one minute to understand a commit log.
>>
>> I prose that you rewrite the commit log, or at least parts of if it.
>> When you are describing a bug don't talk about enums or source files,
>> that's an implementation detail, and instead talk how the bug looks like
>> from users point of view. For example:
>>
>>    "On HP laptop model 1234 with Realtex 4321 device users are supposed
>>     to use ant_sel module parameter with value 77 to use the correct
>>     antenna. But instead rtlwifi incorrectly chose antenna 88 with lower
>>     transmit power and that caused packet loss. Fix it so that the user
>>     gets better transmit power and..."
>>
>> (that's of course all made up information as I don't know what the
>> actual bug is)
>>
>> And after that you can write rtlwifi internals in the commit log as much
>> as you want :) But there needs to be a clear generic description of the
>> bug first and it needs to understandable in one read.
>>
>> To make this faster I propose that you send the new commit log as a
>> reply to this mail and I can then comment faster.
>
> Kalle,
>
> As I have some responsibility in creating this mess, let me try to
> write a new commit log. I hope this answers your questions.
>
> Thanks,
>
> Larry
>
> ====================================
>
> Some HP laptops have only a single wifi antenna. This would not be a
> problem except that they were shipped with an incorrectly encoded
> EFUSE. It should have been possible to open the computer and transfer
> the antenna connection to the other terminal except that such action
> might void the warranty, and moving the antenna broke the Windows
> driver. The fix was to add a module option that would override the
> EFUSE encoding. That was done with commit c18d8f509571 ("rtlwifi:
> rtl8723be: Add antenna select module parameter"). There was still a
> problem with Bluetooth coexistence, which was addressed with commit
> baa170229095 ("rtlwifi: btcoexist: Implement antenna selection").
> There were still problems, thus there were commit 0ff78adeef11
> ("rtlwifi: rtl8723be: fix ant_sel code") and commit 6d6226928369
> ("rtlwifi: btcoexist: Fix antenna selection code"). Despite all these
> attempts at fixing the problem, the code is not yet right. A proper
> fix is important as there are now instances of laptops having
> RTL8723DE chips with the same problem.

This looks better, thanks.

> The module parameter ant_sel is used to control antenna number and path.
> At present enum ANT_{X2,X1} is used to define the antenna number, but
> this choice is not intuitive, thus change to a new enum ANT_{MAIN,AUX}
> to make it more readable. This change showed examples where incorrect
> values were used. It was also possible to remove a workaround in
> halbtcoutsrc.c.
>
> The experimental results with single antenna connected to specific path
> are now as follows:
>   ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>      0        -8            -62
>      1        -62           -10
>      2        -6            -60

But after reading this I'm still not sure if users are supposed to do
anything. I guess they will continue using existing values with the
ant_sel module parameter and nothing changes in that regard?

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition
  2018-04-23 13:47     ` Kalle Valo
@ 2018-04-23 16:09       ` Larry Finger
  0 siblings, 0 replies; 8+ messages in thread
From: Larry Finger @ 2018-04-23 16:09 UTC (permalink / raw)
  To: Kalle Valo; +Cc: pkshih, linux-wireless, stable

On 04/23/2018 08:47 AM, Kalle Valo wrote:
> Larry Finger <Larry.Finger@lwfinger.net> writes:
> 
>> On 04/20/2018 07:01 AM, Kalle Valo wrote:
>>> <pkshih@realtek.com> writes:
>>>
>>>> From: Ping-Ke Shih <pkshih@realtek.com>
>>>>
>>>> The module parameter ant_sel is used to control antenna number and path.
>>>> There is an existing enum ANT_{X2,X1} defined the antenna number, so
>>>> add a new enum ANT_{MAIN,AUX} to make it readable. After this work,
>>>> incorrect given values depend on ant_sel were exposed, so refill values
>>>> according following definition:
>>>>     ant_sel   ant_num   ant_path  print_label
>>>>        1      ANT_X1    ANT_AUX        #2
>>>>        2      ANT_X2    ANT_MAIN       #1
>>>> Then, a workaround resulted from the incorrect values in halbtcoutsrc.c was
>>>> removed. These is a existing bug in the workaround while ant_sel=2, but the
>>>> case is rare use so user is hard to observe this bug.
>>>>
>>>> The experimental results with single antenna connected to specific path
>>>> are in following:
>>>>     ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>>>>        0        -8            -62
>>>>        1        -62           -10
>>>>        2        -6            -60
>>>>
>>>> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>>>> Cc: Stable <stable@vger.kernel.org> # 4.7+
>>>> Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>
>>>> ---
>>>> v2: Add more description about fixed bugs in end of first paragraph.
>>>
>>> Sorry, I still don't understand the bug you are fixing. It shouldn't
>>> take more than one minute to understand a commit log.
>>>
>>> I prose that you rewrite the commit log, or at least parts of if it.
>>> When you are describing a bug don't talk about enums or source files,
>>> that's an implementation detail, and instead talk how the bug looks like
>>> from users point of view. For example:
>>>
>>>     "On HP laptop model 1234 with Realtex 4321 device users are supposed
>>>      to use ant_sel module parameter with value 77 to use the correct
>>>      antenna. But instead rtlwifi incorrectly chose antenna 88 with lower
>>>      transmit power and that caused packet loss. Fix it so that the user
>>>      gets better transmit power and..."
>>>
>>> (that's of course all made up information as I don't know what the
>>> actual bug is)
>>>
>>> And after that you can write rtlwifi internals in the commit log as much
>>> as you want :) But there needs to be a clear generic description of the
>>> bug first and it needs to understandable in one read.
>>>
>>> To make this faster I propose that you send the new commit log as a
>>> reply to this mail and I can then comment faster.
>>
>> Kalle,
>>
>> As I have some responsibility in creating this mess, let me try to
>> write a new commit log. I hope this answers your questions.
>>
>> Thanks,
>>
>> Larry
>>
>> ====================================
>>
>> Some HP laptops have only a single wifi antenna. This would not be a
>> problem except that they were shipped with an incorrectly encoded
>> EFUSE. It should have been possible to open the computer and transfer
>> the antenna connection to the other terminal except that such action
>> might void the warranty, and moving the antenna broke the Windows
>> driver. The fix was to add a module option that would override the
>> EFUSE encoding. That was done with commit c18d8f509571 ("rtlwifi:
>> rtl8723be: Add antenna select module parameter"). There was still a
>> problem with Bluetooth coexistence, which was addressed with commit
>> baa170229095 ("rtlwifi: btcoexist: Implement antenna selection").
>> There were still problems, thus there were commit 0ff78adeef11
>> ("rtlwifi: rtl8723be: fix ant_sel code") and commit 6d6226928369
>> ("rtlwifi: btcoexist: Fix antenna selection code"). Despite all these
>> attempts at fixing the problem, the code is not yet right. A proper
>> fix is important as there are now instances of laptops having
>> RTL8723DE chips with the same problem.
> 
> This looks better, thanks.
> 
>> The module parameter ant_sel is used to control antenna number and path.
>> At present enum ANT_{X2,X1} is used to define the antenna number, but
>> this choice is not intuitive, thus change to a new enum ANT_{MAIN,AUX}
>> to make it more readable. This change showed examples where incorrect
>> values were used. It was also possible to remove a workaround in
>> halbtcoutsrc.c
>>
>> The experimental results with single antenna connected to specific path
>> are now as follows:
>>    ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>>       0        -8            -62
>>       1        -62           -10
>>       2        -6            -60
> 
> But after reading this I'm still not sure if users are supposed to do
> anything. I guess they will continue using existing values with the
> ant_sel module parameter and nothing changes in that regard?

If users have two antennas, or a vendor that correctly encoded the EFUSE, this 
patch will change nothing for them. If they have the low-signal problem, then 
their choice of a value for ant_sel should not change. At least we are now sure 
that using that module option will provide correct operation.

Larry

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [v2] rtlwifi: cleanup 8723be ant_sel definition
  2018-04-20  2:30 [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition pkshih
  2018-04-20 12:01 ` Kalle Valo
@ 2018-04-24 10:16 ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2018-04-24 10:16 UTC (permalink / raw)
  To: Ping-Ke Shih; +Cc: Larry.Finger, linux-wireless, stable

Ping-Ke Shih <pkshih@realtek.com> wrote:

> From: Ping-Ke Shih <pkshih@realtek.com>
> 
> Some HP laptops have only a single wifi antenna. This would not be a
> problem except that they were shipped with an incorrectly encoded
> EFUSE. It should have been possible to open the computer and transfer
> the antenna connection to the other terminal except that such action
> might void the warranty, and moving the antenna broke the Windows
> driver. The fix was to add a module option that would override the
> EFUSE encoding. That was done with commit c18d8f509571 ("rtlwifi:
> rtl8723be: Add antenna select module parameter"). There was still a
> problem with Bluetooth coexistence, which was addressed with commit
> baa170229095 ("rtlwifi: btcoexist: Implement antenna selection").
> There were still problems, thus there were commit 0ff78adeef11
> ("rtlwifi: rtl8723be: fix ant_sel code") and commit 6d6226928369
> ("rtlwifi: btcoexist: Fix antenna selection code"). Despite all these
> attempts at fixing the problem, the code is not yet right. A proper
> fix is important as there are now instances of laptops having
> RTL8723DE chips with the same problem.
> 
> The module parameter ant_sel is used to control antenna number and path.
> At present enum ANT_{X2,X1} is used to define the antenna number, but
> this choice is not intuitive, thus change to a new enum ANT_{MAIN,AUX}
> to make it more readable. This change showed examples where incorrect
> values were used. It was also possible to remove a workaround in
> halbtcoutsrc.c.
> 
> The experimental results with single antenna connected to specific path
> are now as follows:
>   ant_sel  ANT_MAIN(#1)  ANT_AUX(#2)
>      0        -8            -62
>      1        -62           -10
>      2        -6            -60
> 
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> Fixes: c18d8f509571 ("rtlwifi: rtl8723be: Add antenna select module parameter")
> Fixes: baa170229095 ("rtlwifi: btcoexist: Implement antenna selection")
> Fixes: 0ff78adeef11 ("rtlwifi: rtl8723be: fix ant_sel code")
> Fixes: 6d6226928369 ("rtlwifi: btcoexist: Fix antenna selection code")
> Cc: Stable <stable@vger.kernel.org> # 4.7+
> Reviewed-by: Larry Finger <Larry.Finger@lwfinger.net>

Patch applied to wireless-drivers.git, thanks.

af8a41cccf8f rtlwifi: cleanup 8723be ant_sel definition

-- 
https://patchwork.kernel.org/patch/10351789/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-04-24 10:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-20  2:30 [PATCH v2] rtlwifi: cleanup 8723be ant_sel definition pkshih
2018-04-20 12:01 ` Kalle Valo
2018-04-20 20:56   ` Larry Finger
2018-04-23  2:19     ` Pkshih
2018-04-23  2:19       ` Pkshih
2018-04-23 13:47     ` Kalle Valo
2018-04-23 16:09       ` Larry Finger
2018-04-24 10:16 ` [v2] " Kalle Valo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.