[PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower

[John Heenan]

Code tests show data returned by rtl8xxxu_read8(priv, REG_CR), used to set
macpower, is never 0xea. It is only ever 0x01 (first time after modprobe)
using wpa_supplicant and 0x00 thereafter using wpa_supplicant. These results
occurs with 'Fix for authentication failure' [PATCH 1/2] in place.

Whatever was returned, code tests always showed that at least
rtl8xxxu_init_queue_reserved_page(priv);
is always required. Not called if macpower set to true.

Please see cover letter, [PATCH 0/2], for more information from tests.

For rtl8xxxu-devel branch of git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git

Signed-off-by: John Heenan <jo...@zgus.com>
---
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index f25b4df..aae05f3 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -3904,6 +3904,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
macpower = false;
else
macpower = true;
+ macpower = false; // Code testing shows macpower must always be set to false to avoid failure

ret = fops->power_on(priv);
if (ret < 0) {
--
2.10.1

[Jes Sorensen]

Sorry but this patch is neither serious nor acceptable. First of all,
hardcoding macpower like this right after an if statement is plain
wrong, second your comments violate all kernel rules.

Second, you argue this was tested using code test - on which device? Did
you test it on all rtl8xxxu based devices or just rtl8723bu?

NACK

Jes

[John Heenan]

Thanks for your reply.

The code was tested on a Cube i9 which has an internal rtl8723bu.

No other devices were tested.

I am happy to accept in an ideal context hard coding macpower is
undesirable, the comment is undesirable and it is wrong to assume the
issue is not unique to the rtl8723bu.

Your reply is idealistic. What can I do now? I should of course have
factored out other untested devices in my patches. The apparent
concern you have with process over outcome is a useful lesson.

We are not in an ideal situation. The comment is of course relevant
and useful to starting a process to fixing a real bug I do not have
sufficient information to refine any further for and others do. In the
circumstances nothing really more can be expected.

My patch cover letter, [PATCH 0/2] provides evidence of a mess with
regard to determining macpower for the rtl8723bu and what is
subsequently required. This is important.

The kernel driver code is very poorly documented and there is not a
single source reference to device documentation. For example macpower
is noting more than a setting that is true or false according to
whether a read of a particular register return 0xef or not. Such value
was never obtained so a full init sequence was never performed.

It would be helpful if you could provide a link to device references.
As it is, how am I supposed to revise the patch without relevant
information?

My patch code works with the Cube i9, as is, despite a lack of
adequate information. Before it did not. That is a powerful statement

Have a nice day.

John Heenan

On 30 October 2016 at 22:00, Jes Sorensen <Jes.So...@redhat.com> wrote:
> John Heenan <jo...@zgus.com> writes:
>> Code tests show data returned by rtl8xxxu_read8(priv, REG_CR), used to set
>> macpower, is never 0xea. It is only ever 0x01 (first time after modprobe)
>> using wpa_supplicant and 0x00 thereafter using wpa_supplicant. These results
>> occurs with 'Fix for authentication failure' [PATCH 1/2] in place.
>>
>> Whatever was returned, code tests always showed that at least
>> rtl8xxxu_init_queue_reserved_page(priv);
>> is always required. Not called if macpower set to true.
>>
>> Please see cover letter, [PATCH 0/2], for more information from tests.
>>
>

[Jes Sorensen]

John Heenan <jo...@zgus.com> writes:
> Thanks for your reply.
>
> The code was tested on a Cube i9 which has an internal rtl8723bu.
>
> No other devices were tested.
>
> I am happy to accept in an ideal context hard coding macpower is
> undesirable, the comment is undesirable and it is wrong to assume the
> issue is not unique to the rtl8723bu.
>
> Your reply is idealistic. What can I do now? I should of course have
> factored out other untested devices in my patches. The apparent
> concern you have with process over outcome is a useful lesson.
>
> We are not in an ideal situation. The comment is of course relevant
> and useful to starting a process to fixing a real bug I do not have
> sufficient information to refine any further for and others do. In the
> circumstances nothing really more can be expected.

Well you should start by reporting the issue and either providing a
patch that only affects 8723bu, or work on a generic solution. I
appreciate patches, but I do not appreciate patches that will make
something work for one person and break for everyone else - I spent a
lot of time making sure the driver works across the different devices.

The comment violates all Linux standards - first rule when modifying
code is to respect the style of the code you are dealing with.

Code is 80 characters wide, and comments are /* */ never the ugly C++
crap.

> My patch cover letter, [PATCH 0/2] provides evidence of a mess with
> regard to determining macpower for the rtl8723bu and what is
> subsequently required. This is important.
>
> The kernel driver code is very poorly documented and there is not a
> single source reference to device documentation. For example macpower
> is noting more than a setting that is true or false according to
> whether a read of a particular register return 0xef or not. Such value
> was never obtained so a full init sequence was never performed.

The kernel driver is documented with the information I have - there is
NO device documentation because Realtek refuses to provide any. I have
written the driver based on what I have retried by reading the vendor
drivers. If you can provide better documentation, I certainly would love
to get it.

> It would be helpful if you could provide a link to device references.
> As it is, how am I supposed to revise the patch without relevant
> information?

Look at the USB device table, it shows you which devices are supported.

> My patch code works with the Cube i9, as is, despite a lack of
> adequate information. Before it did not. That is a powerful statement

The driver works with a lot of different devices in itself that is a
powerful statement!

Yes I want to see it work with as many devices as possible, but just
moving things around without balancing it and not explaining why is not
a fix. If we move more of the init sequence to _start() you also have to
move matching pieces to _stop().

Jes

[Larry Finger]

On 10/30/2016 05:21 AM, John Heenan wrote:
> Code tests show data returned by rtl8xxxu_read8(priv, REG_CR), used to set
> macpower, is never 0xea. It is only ever 0x01 (first time after modprobe)
> using wpa_supplicant and 0x00 thereafter using wpa_supplicant. These results
> occurs with 'Fix for authentication failure' [PATCH 1/2] in place.
>
> Whatever was returned, code tests always showed that at least
> rtl8xxxu_init_queue_reserved_page(priv);
> is always required. Not called if macpower set to true.
>
> Please see cover letter, [PATCH 0/2], for more information from tests.

That cover letter will NOT be included in the commit message, thus referring to
it here is totally pointless.

>
> For rtl8xxxu-devel branch of git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git

Same comment as for the previous patch.

Again I leave the review of the code changes to Jes.

Larry

[David Miller]

From: Larry Finger <Larry....@lwfinger.net>
Date: Wed, 2 Nov 2016 20:00:03 -0500

> On 10/30/2016 05:21 AM, John Heenan wrote:
>> Code tests show data returned by rtl8xxxu_read8(priv, REG_CR), used to
>> set
>> macpower, is never 0xea. It is only ever 0x01 (first time after
>> modprobe)
>> using wpa_supplicant and 0x00 thereafter using wpa_supplicant. These
>> results
>> occurs with 'Fix for authentication failure' [PATCH 1/2] in place.
>>
>> Whatever was returned, code tests always showed that at least
>> rtl8xxxu_init_queue_reserved_page(priv);
>> is always required. Not called if macpower set to true.
>>
>> Please see cover letter, [PATCH 0/2], for more information from tests.
>
> That cover letter will NOT be included in the commit message, thus
> referring to it here is totally pointless.

This is why when a patch series is added to GIT, the cover letter
must be added to the merge commit that adds that series.

It is therefore perfectly valid to refer to such text from a
commit contained by that merge commit.

[Joe Perches]

On Sun, 2016-10-30 at 19:02 -0400, Jes Sorensen wrote:
> Code is 80 characters wide, and comments are /* */ never the ugly C++
> crap.

You might look at the recent Linus Torvalds authored commit
5e467652ffef (?printk: re-organize log_output() to be more legible")
which does both of those: c99 // comments and > 80 columns.

Absolutes are for zealots.

[Larry Finger]

Of course, but who is going to criticize Linus? I have gently chided him when an
untested patch of his was inserted just before the final release, and broke my
laptop. At least the bisection was pretty quick. :)

Larry

[Jes Sorensen]

What Linus does in his code, is totally up to him. What I pull into the
driver that *I* maintain, is up to me. It is perfectly normal to expect
submitters to respect the coding style of the piece of code they are
trying to edit.

Jes