[PATCH 1/2] rtl8xxxu: Fix for authentication failure

[PATCH 1/2] rtl8xxxu: Fix for authentication failure

[John Heenan]

This fix enables the same sequence of init behaviour as the alternative
working driver for the wireless rtl8723bu IC at
https://github.com/lwfinger/rtl8723bu

For exampe rtl8xxxu_init_device is now called each time
userspace wpa_supplicant is executed instead of just once when
modprobe is executed.

Along with 'Fix for bogus data used to determine macpower',
wpa_supplicant now reliably and successfully authenticates.

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

Signed-off-by: John Heenan <john@zgus.com>
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 04141e5..f25b4df 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -5779,6 +5779,11 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
 
 	ret = 0;
 
+	ret = rtl8xxxu_init_device(hw);
+	if (ret)
+		goto error_out;
+
+
 	init_usb_anchor(&priv->rx_anchor);
 	init_usb_anchor(&priv->tx_anchor);
 	init_usb_anchor(&priv->int_anchor);
@@ -6080,10 +6085,6 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
 		goto exit;
 	}
 
-	ret = rtl8xxxu_init_device(hw);
-	if (ret)
-		goto exit;
-
 	hw->wiphy->max_scan_ssids = 1;
 	hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
 	hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
-- 
2.10.1

[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 <john@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

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

[Jes Sorensen]

John Heenan <john@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.
>
> For rtl8xxxu-devel branch of git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
>
> Signed-off-by: John Heenan <john@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) {

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

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

[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.Sorensen@redhat.com> wrote:
> John Heenan <john@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.
>>
>
> 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

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

[Jes Sorensen]

John Heenan <john@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

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

[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

Re: [PATCH 1/2] rtl8xxxu: Fix for authentication failure

[Larry Finger]

On 10/30/2016 05:20 AM, John Heenan wrote:
> This fix enables the same sequence of init behaviour as the alternative
> working driver for the wireless rtl8723bu IC at
> https://github.com/lwfinger/rtl8723bu
>
> For exampe rtl8xxxu_init_device is now called each time
> userspace wpa_supplicant is executed instead of just once when
> modprobe is executed.

After all the trouble you have had with your patches, I would expect you to use 
more care when composing the commit message. Note the typo in the paragraph above.

> Along with 'Fix for bogus data used to determine macpower',
> wpa_supplicant now reliably and successfully authenticates.

I'm not sure this paragraph belongs in the permanent commit record.

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

I know this line does not belong. If you want to include information like this, 
include it after a line containing "---". Those lines will be available to 
reviewers and maintainers, but will be stripped before it gets included in the 
code base.

> Signed-off-by: John Heenan <john@zgus.com>
> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 04141e5..f25b4df 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -5779,6 +5779,11 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>
>  	ret = 0;
>
> +	ret = rtl8xxxu_init_device(hw);
> +	if (ret)
> +		goto error_out;
> +
> +
>  	init_usb_anchor(&priv->rx_anchor);
>  	init_usb_anchor(&priv->tx_anchor);
>  	init_usb_anchor(&priv->int_anchor);
> @@ -6080,10 +6085,6 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>  		goto exit;
>  	}
>
> -	ret = rtl8xxxu_init_device(hw);
> -	if (ret)
> -		goto exit;
> -
>  	hw->wiphy->max_scan_ssids = 1;
>  	hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
>  	hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
>

I will let Jes comment on any side effects of this code change.

Larry

-- 
If I was stranded on an island and the only way to get off
the island was to make a pretty UI, I’d die there.

Linus Torvalds

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

[David Miller]

From: Larry Finger <Larry.Finger@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.

Re: [PATCH 1/2] rtl8xxxu: Fix for authentication failure

[John Heenan]

On 3 November 2016 at 11:00, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 10/30/2016 05:20 AM, John Heenan wrote:
>>
>> This fix enables the same sequence of init behaviour as the alternative
>> working driver for the wireless rtl8723bu IC at
>> https://github.com/lwfinger/rtl8723bu
>>
>> For exampe rtl8xxxu_init_device is now called each time
>> userspace wpa_supplicant is executed instead of just once when
>> modprobe is executed.
>
>
> After all the trouble you have had with your patches, I would expect you to
> use more care when composing the commit message. Note the typo in the
> paragraph above.
>

OK, the nasty games continue and the message is not getting through.

An appropriate response by a maintainer would have been to request I
revise the code according to the way it has currently and elegantly
revised in.

[PATCH v2] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

where I use a simple pointer comparison of priv->fops with &rtl8723bu_fops.

As far as I can see there is nothing more to be done on my part code
wise. The supposed issue with matching functions, as far as I can see,
is a non issue.

If you want to comment on comments in patch messages please do so on
the above current proposed patch instead of dragging up stale and
irreelvant patch proposals to make a petty point.

Jes has now moved the goal posts, indicating all drivers for rtl8xxxu
need to be tested fro similar issues. If there are problems with other
rtl8xxxu drives then it is not my problem and has nothing to do with
me. My issue is only with the rtl8723bu driver.

Such doubts also makes it look as if there was never any proper
testing and there is no real interest in proper testing. After all
that would involve cooperation and team work.

Want concrete evidence? I actually did report problems to Jes in
private emails, AS REQUESTED, in dmesg before I started tests and
posting patches. In the emalI stated I was willing to test drivers
with printk messages.  I did not even get the courtesy of a reply.

Want even more concrete evidence? Jes has some very strange comments
in his tree for his current last commit
f958b1e0806c045830d78c4287fbcddf9e5fd9d0

   " This uncovered a huge bug where the old code would use struct
    ieee80211_rate.flags to test for rate parameters, which is always
    zero, instead of the flags value from struct ieee80211_tx_rate.

    "Time to find a brown paper bag :("

John Heenan


>> Along with 'Fix for bogus data used to determine macpower',
>> wpa_supplicant now reliably and successfully authenticates.
>
>
> I'm not sure this paragraph belongs in the permanent commit record.
>
>> For rtl8xxxu-devel branch of
>> git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
>
>
> I know this line does not belong. If you want to include information like
> this, include it after a line containing "---". Those lines will be
> available to reviewers and maintainers, but will be stripped before it gets
> included in the code base.
>
>
>> Signed-off-by: John Heenan <john@zgus.com>
>> ---
>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index 04141e5..f25b4df 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -5779,6 +5779,11 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>>
>>         ret = 0;
>>
>> +       ret = rtl8xxxu_init_device(hw);
>> +       if (ret)
>> +               goto error_out;
>> +
>> +
>>         init_usb_anchor(&priv->rx_anchor);
>>         init_usb_anchor(&priv->tx_anchor);
>>         init_usb_anchor(&priv->int_anchor);
>> @@ -6080,10 +6085,6 @@ static int rtl8xxxu_probe(struct usb_interface
>> *interface,
>>                 goto exit;
>>         }
>>
>> -       ret = rtl8xxxu_init_device(hw);
>> -       if (ret)
>> -               goto exit;
>> -
>>         hw->wiphy->max_scan_ssids = 1;
>>         hw->wiphy->max_scan_ie_len = IEEE80211_MAX_DATA_LEN;
>>         hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
>>
>
> I will let Jes comment on any side effects of this code change.
>
> Larry
>
> --
> If I was stranded on an island and the only way to get off
> the island was to make a pretty UI, I’d die there.
>
> Linus Torvalds

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

[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.

Re: [PATCH 1/2] rtl8xxxu: Fix for authentication failure

[Larry Finger]

On 11/03/2016 02:10 AM, John Heenan wrote:
> On 3 November 2016 at 11:00, Larry Finger <Larry.Finger@lwfinger.net> wrote:
>> On 10/30/2016 05:20 AM, John Heenan wrote:
>>>
>>> This fix enables the same sequence of init behaviour as the alternative
>>> working driver for the wireless rtl8723bu IC at
>>> https://github.com/lwfinger/rtl8723bu
>>>
>>> For exampe rtl8xxxu_init_device is now called each time
>>> userspace wpa_supplicant is executed instead of just once when
>>> modprobe is executed.
>>
>>
>> After all the trouble you have had with your patches, I would expect you to
>> use more care when composing the commit message. Note the typo in the
>> paragraph above.
>>
>
> OK, the nasty games continue and the message is not getting through.
>
> An appropriate response by a maintainer would have been to request I
> revise the code according to the way it has currently and elegantly
> revised in.

I am NOT a maintainer for this driver. I do have an interest in it, but not in 
any official capacity.

If you cannot accept constructive criticism, you are in the wrong activity. 
Please grow up!

Larry

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

[Larry Finger]

On 11/03/2016 03:41 AM, Joe Perches wrote:
> 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.

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

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

[Jes Sorensen]

Joe Perches <joe@perches.com> writes:
> 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.

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