rtl8xxxu: Fix for authentication failure
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/2 rtl8xxxu: Fix for authentication failure
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
--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +5 -4
@@ -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 );
Signature · 1 line
2.10.1
Quoted 7 lines · 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/rtl8723buFor exampe
rtl8xxxu_init_deviceis now called each time userspacewpa_supplicantis 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.
Quoted 2 lines
Along with 'Fix for bogus data used to determine macpower',
wpa_supplicantnow reliably and successfully authenticates.
I'm not sure this paragraph belongs in the permanent commit record.
Quoted 1 line
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.
Quoted 23 lines
Signed-off-by John Heenan <[email protected]>--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +5
-0@@ -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);
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c static int rtl8xxxu_probe(struct usb_interface *interface, · line 6090
Quoted 10 lines
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +0
-4@@ -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
Signature · 4 lines
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
Quoted 6 lines · Larry Finger
Quoted 5 lines · 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
Quoted 9 lines
Quoted 3 lines
For exampe
rtl8xxxu_init_deviceis now called each time userspacewpa_supplicantis 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
Quoted 61 lines
Quoted 2 lines
Along with 'Fix for bogus data used to determine macpower',
wpa_supplicantnow reliably and successfully authenticates.I'm not sure this paragraph belongs in the permanent commit record.
Quoted 2 lines
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.
Quoted 33 lines
Signed-off-by John Heenan <[email protected]>--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +5
-4@@ -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 ;Quoted 2 lines
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
Quoted 17 lines · John Heenan
Quoted 6 lines · Larry Finger
Quoted 5 lines · 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/rtl8723buQuoted 9 lines
Quoted 3 lines
For exampe
rtl8xxxu_init_deviceis now called each time userspacewpa_supplicantis 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.
Quoted 5 lines
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
PATCH 2/2 rtl8xxxu: Fix for bogus data used to determine macpower
In reply to a message not in the archiveCode 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
--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +1 -0
@@ -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 ) {
Signature · 1 line
2.10.1
Jes Sorensen
NAKRe: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower
Quoted 23 lines · John Heenan
Code tests show data returned by
rtl8xxxu_read8(priv, REG_CR), used to set macpower, is never0xea. It is only ever0x01(first time after modprobe) usingwpa_supplicantand0x00thereafter usingwpa_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 <[email protected]>--- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 1 + 1 file changed, 1 insertion(+)drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +0
-0
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c static int rtl8xxxu_init_device(struct ieee80211_hw *hw) · line 3910
Quoted 7 lines
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +1
-0@@ -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
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
Quoted 23 lines · Jes Sorensen
Quoted 11 lines · John Heenan
Code tests show data returned by
rtl8xxxu_read8(priv, REG_CR), used to set macpower, is never0xea. It is only ever0x01(first time after modprobe) usingwpa_supplicantand0x00thereafter usingwpa_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
Quoted 10 lines · 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.
Quoted 8 lines
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.
Quoted 9 lines
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
0xefor 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.
Quoted 3 lines
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.
Quoted 2 lines
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
Quoted 2 lines · Jes Sorensen
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 2/2] rtl8xxxu: Fix for bogus data used to determine macpower
Quoted 9 lines · Joe Perches
Quoted 2 lines · Jes Sorensen
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-organizelog_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
In reply to Joe PerchesQuoted 9 lines · Joe Perches
Quoted 2 lines · Jes Sorensen
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-organizelog_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
Re: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower
In reply to John HeenanQuoted 10 lines · John Heenan
Code tests show data returned by
rtl8xxxu_read8(priv, REG_CR), used to set macpower, is never0xea. It is only ever0x01(first time after modprobe) usingwpa_supplicantand0x00thereafter usingwpa_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.
Quoted 2 lines
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
maintainerRe: [PATCH 2/2] rtl8xxxu: Fix for bogus data used to determine macpower
From: Larry Finger <[email protected]>
Date: Wed, 2 Nov 2016 20:00:03 -0500
Quoted 9 lines · John Heenan
Quoted 8 lines
Code tests show data returned by
rtl8xxxu_read8(priv, REG_CR), used to set
macpower, is never0xea. It is only ever0x01(first time after modprobe)
usingwpa_supplicantand0x00thereafter usingwpa_supplicant. These results
occurs with 'Fix for authentication failure' [PATCH 1/2] in place.
Quoted 8 lines
Quoted 5 lines
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.