Larry Finger <Larry.Finger@xxxxxxxxxxxx> writes:
> On 04/20/2018 07:01 AM, Kalle Valo wrote:
>> <pkshih@xxxxxxxxxxx> writes:
>>
>>> From: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxx>
>>> Cc: Stable <stable@xxxxxxxxxxxxxxx> # 4.7+
>>> Reviewed-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
>>> ---
>>> 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