[PATCH 0/2] rtl8xxxu: Fix allows wpa_supplicant to authenticate
With the current kernel release, wpa_supplicant results in authentication failure with a Cube i9 tablet (a Surface Pro like device): Successfully initialized wpa_supplicant wlp0s20f0u7i2: SME: Trying to authenticate with 10:fe:ed:62:7a:78 (SSID='localre' freq=2417 MHz) wlp0s20f0u7i2: SME: Trying to authenticate with 10:fe:ed:62:7a:78 (SSID='localre' freq=2417 MHz) wlp0s20f0u7i2: SME: Trying to authenticate with 10:fe:ed:62:7a:78 (SSID='localre' freq=2417 MHz) wlp0s20f0u7i2: SME: Trying to authenticate with 10:fe:ed:62:7a:78 (SSID='localre' freq=2417 MHz) wlp0s20f0u7i2: CTRL-EVENT-SSID-TEMP-DISABLED id=0 ssid="localre" auth_failures=1 duration=10 reason=CONN_FAILED There is a workaround: that ONLY works once per invocation of wpa_supplicant: rmmod rtl8xxxu modprobe rtl8xxxu The follwing two patches result in reliable behaviour, without a workaround, of wpa_supplicant: Successfully initialized wpa_supplicant wlp0s20f0u7i2: SME: Trying to authenticate with 10:fe:ed:62:7a:78 (SSID='localre' freq=2417 MHz) wlp0s20f0u7i2: Trying to associate with 10:fe:ed:62:7a:78 (SSID='localre' freq=2417 MHz) wlp0s20f0u7i2: Associated with 10:fe:ed:62:7a:78 wlp0s20f0u7i2: CTRL-EVENT-SUBNET-STATUS-UPDATE status=0 wlp0s20f0u7i2: WPA: Key negotiation completed with 10:fe:ed:62:7a:78 [PTK=CCMP GTK=CCMP] wlp0s20f0u7i2: CTRL-EVENT-CONNECTED - Connection to 10:fe:ed:62:7a:78 completed [id=0 id_str=] The patches are for kernel tree: git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git branch: rtl8xxxu-devel The first patch moves init code so that each time wpa_supplicant is invoked there is similar init behaviour as the alternative working rtl8xxxu driver https://github.com/lwfinger/rtl8723bu The second patch enables more complete initialisation to occur. There are three issues: 1. The value returned by "rtl8xxxu_read8(priv, REG_CR);", to set macpower, is never 0xef. The value is either 0x01 (first time with wpa_supplcant after modprobe) or 0x00 (re executing wpa_supplicant) 2. Trying to use the value 0x00 or 0x01 retutned to determine macpower setting always resulted in failure 3. At the very least 'rtl8xxxu_init_queue_reserved_page(priv);' must always be invoked, even if not all of the extra init sequence arising from setting macpower to false is run. Patched code with a suitable Makefile will be available from https://github.com/johnheenan/rtl8xxxu for testing by Cube i9 owners John Heenan (2): rtl8xxxu: Fix for authentication failure rtl8xxxu: Fix for bogus data used to determine macpower drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) -- 2.10.1
[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 Signed-off-by: John Heenan --- 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
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
---
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
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 wrote: > John Heenan 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
[PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
The rtl8723bu wireless IC shows evidence of a more agressive approach to
power saving, powering down its RF side when there is no wireless
interfacing but leaving USB interfacing intact. This makes the wireless
IC more suitable for use in devices which need to keep their power use
as low as practical, such as tablets and Surface Pro type devices.
In effect this means that a full initialisation must be performed
whenever a wireless interface is brought up. It also means that
interpretations of power status from general wireless registers should
not be relied on to influence an init sequence.
The patch works by forcing a fuller initialisation and forcing it to
occur more often in code paths (such as occurs during a low level
authentication that initiates wireless interfacing).
The initialisation sequence is now more consistent with code based
directly on vendor code. For example while the vendor derived code
interprets a register as indcating a particular powered state, it does
not use this information to influence its init sequence.
The rtl8723bu device has a unique USB VID and PID. This is taken
advantage of for the patch to ensure only rtl8723bu devices are affected
by this patch.
With this patch wpa_supplicant reliably and consistently connects with
an AP. Before a workaround such as executing rmmod and modprobe before
each call to wpa_supplicant worked with some distributions.
Signed-off-by: John Heenan
---
.../net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 24 ++
1 file changed, 20 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..f36e674 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -79,6 +79,8 @@ MODULE_PARM_DESC(dma_agg_pages, "Set DMA aggregation pages
(range 1-127, 0 to di
#define RTL8XXXU_TX_URB_LOW_WATER 25
#define RTL8XXXU_TX_URB_HIGH_WATER 32
+#define USB_PRODUCT_ID_RTL8723BU 0xb720
+
static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
struct rtl8xxxu_rx_urb *rx_urb);
@@ -3892,6 +3894,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
u8 val8;
u16 val16;
u32 val32;
+ struct usb_device_descriptor *udesc = &priv->udev->descriptor;
/* Check if MAC is already powered on */
val8 = rtl8xxxu_read8(priv, REG_CR);
@@ -3900,7 +3903,9 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
* Fix 92DU-VC S3 hang with the reason is that secondary mac is not
* initialized. First MAC returns 0xea, second MAC returns 0x00
*/
- if (val8 == 0xea)
+ if (val8 == 0xea
+ || (udesc->idVendor == USB_VENDOR_ID_REALTEK
+ && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU))
macpower = false;
else
macpower = true;
@@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
struct rtl8xxxu_tx_urb *tx_urb;
unsigned long flags;
int ret, i;
+ struct usb_device_descriptor *udesc = &priv->udev->descriptor;
ret = 0;
+ if(udesc->idVendor == USB_VENDOR_ID_REALTEK
+ && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
+ 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,9 +6093,12 @@ static int rtl8xxxu_probe(struct usb_interface
*interface,
goto exit;
}
- ret = rtl8xxxu_init_device(hw);
- if (ret)
+ if(!(id->idVendor == USB_VENDOR_ID_REALTEK
+ && id->idProduct == USB_PRODUCT_ID_RTL8723BU)) {
+ 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;
@@ -6191,7 +6207,7 @@ static struct usb_device_id dev_table[] = {
/* Tested by Myckel Habets */
{USB_DEVICE_AND_INTERFACE_INFO(0x2357, 0x0109, 0xff, 0xff, 0xff),
.driver_info = (unsigned long)&rtl8192eu_fops},
-{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, 0xb720, 0xff, 0xff,
0xff),
+{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK,
USB_PRODUCT_ID_RTL8723BU, 0xff, 0xff, 0xff),
.driver_info = (unsigned long)&rtl8723bu_fops},
#ifdef CONFIG_RTL8XXXU_UNTESTED
/* Still supported by rtlwifi */
--
2.10.1
Re: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
On 1 November 2016 at 07:25, Jes Sorensen wrote:
> John Heenan writes:
>> The rtl8723bu wireless IC shows evidence of a more agressive approach to
>> power saving, powering down its RF side when there is no wireless
>> interfacing but leaving USB interfacing intact. This makes the wireless
>> IC more suitable for use in devices which need to keep their power use
>> as low as practical, such as tablets and Surface Pro type devices.
>>
>> In effect this means that a full initialisation must be performed
>> whenever a wireless interface is brought up. It also means that
>> interpretations of power status from general wireless registers should
>> not be relied on to influence an init sequence.
>>
>> The patch works by forcing a fuller initialisation and forcing it to
>> occur more often in code paths (such as occurs during a low level
>> authentication that initiates wireless interfacing).
>>
>> The initialisation sequence is now more consistent with code based
>> directly on vendor code. For example while the vendor derived code
>> interprets a register as indcating a particular powered state, it does
>> not use this information to influence its init sequence.
>>
>> The rtl8723bu device has a unique USB VID and PID. This is taken
>> advantage of for the patch to ensure only rtl8723bu devices are affected
>> by this patch.
>>
>> With this patch wpa_supplicant reliably and consistently connects with
>> an AP. Before a workaround such as executing rmmod and modprobe before
>> each call to wpa_supplicant worked with some distributions.
>>
>> Signed-off-by: John Heenan
>> ---
>> .../net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 24
>> ++
>> 1 file changed, 20 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..f36e674 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -79,6 +79,8 @@ MODULE_PARM_DESC(dma_agg_pages, "Set DMA aggregation pages
>> (range 1-127, 0 to di
>> #define RTL8XXXU_TX_URB_LOW_WATER25
>> #define RTL8XXXU_TX_URB_HIGH_WATER 32
>>
>> +#define USB_PRODUCT_ID_RTL8723BU 0xb720
>> +
>
> Absolutely not! You have no guarantee that this is the only id used for
> 8723bu devices, and adding a #define for each is not going to happen.
Thanks for you reply.
I have no problem with that. However the patch does get the point
across in a minimalist and efficient way of what the issues are.
Currently there is no property available to determine the information required.
>
>> static int rtl8xxxu_submit_rx_urb(struct rtl8xxxu_priv *priv,
>> struct rtl8xxxu_rx_urb *rx_urb);
>>
>> @@ -3892,6 +3894,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw
>> *hw)
>> u8 val8;
>> u16 val16;
>> u32 val32;
>> + struct usb_device_descriptor *udesc = &priv->udev->descriptor;
>
> Indentaiton
OK. Missed that one.
>
>> /* Check if MAC is already powered on */
>> val8 = rtl8xxxu_read8(priv, REG_CR);
>> @@ -3900,7 +3903,9 @@ static int rtl8xxxu_init_device(struct ieee80211_hw
>> *hw)
>>* Fix 92DU-VC S3 hang with the reason is that secondary mac is not
>>* initialized. First MAC returns 0xea, second MAC returns 0x00
>>*/
>> - if (val8 == 0xea)
>> + if (val8 == 0xea
>> + || (udesc->idVendor == USB_VENDOR_ID_REALTEK
>> + && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU))
>> macpower = false;
>> else
>> macpower = true;
>
> Please respect proper kernel coding style!
I don't know what you mean. Your code has real tabs. My code has real
tabs. The kernel style goes on about tabs being 8 spaces. So do you
want: real tabs or real spaces?
You said no lines over 80 columns long. This is what i have done.
>
>> @@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>> struct rtl8xxxu_tx_urb *tx_urb;
>> unsigned long flags;
>> int ret, i;
>> + struct usb_device_descriptor *udesc = &priv->udev->descriptor;
>>
>> ret = 0;
>>
>> + if(udesc->idVendor == USB_VENDOR_ID_REALTEK
>> + && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
>> + ret = rtl8xxxu_init_device(hw);
>> +
Re: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
I have a prepared another patch that is not USB VID/PID dependent for
rtl8723bu devices. It is more elegant. I will send it after this
email.
If I have more patches is it preferable I just put them on github only
and notify a link address until there might be some resolution?
What I meant below about not finding a matching function is that I
cannot find matching actions to take on any function calls in
rtl8xxxu_stop as for rtl8xxxu_start.
The function that is called later and potentially more than once for
rtl8723bu devices is rtl8xxxu_init_device. There is no corresponding
rtl8xxxu_deinit_device function.
enable_rf is called in rtl8xxxu_start, not in the delayed call to
rtl8xxxu_init_device. The corresponding disable_rf is called in
rtl8xxxu_stop. So no matching issue here. However please see below for
another potential issue.
power_on is called in rtl8xxxu_init_device. power_off is called in
rtl8xxxu_disconnect. Does not appear to be an issue if calling
power_on has no real effect if already on.
The following should be looked at though. For all devices enable_rf is
only called once. For the proposed patch the first call to
rtl8xxxu_init_device is called after the single call to enable_rf for
rtl8723bu devices. Without the patch enable_rf is called after
rtl8xxxu_init_device for all devices
Perhaps enable_rf just configures RF modes to start up when RF is
powered on or if called after power_on then enters requested RF modes.
So I cannot see appropriate additional matching action to take.
Below is some background for anyone interested
rtl8xxxu_start and rtl8xxxu_stop are assigned to rtl8xxxu_ops.
rtl8xxxu_probe assigns rtl8xxxu_ops to its driver layer with
ieee80211_register_hw.
rtl8xxxu_disconnect unassigns rtl8xxxu_ops from its driver layer with
ieee80211_unregister_hw.
rtl8xxxu_probe and rtl8xxxu_disconnect are USB driver functions
John Heenan
On 1 November 2016 at 08:15, John Heenan wrote:
> On 1 November 2016 at 07:25, Jes Sorensen wrote:
>> John Heenan writes:
>
>>
>>> @@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>>> struct rtl8xxxu_tx_urb *tx_urb;
>>> unsigned long flags;
>>> int ret, i;
>>> + struct usb_device_descriptor *udesc = &priv->udev->descriptor;
>>>
>>> ret = 0;
>>>
>>> + if(udesc->idVendor == USB_VENDOR_ID_REALTEK
>>> + && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
>>> + ret = rtl8xxxu_init_device(hw);
>>> + if (ret)
>>> + goto error_out;
>>> + }
>>> +
>>
>> As mentioned previously, if this is to be changed here, it has to be
>> matched in the _stop section too.
>
> I looked at this and could not find a matching function. I will have a
> look again.
>
[PATCH v2] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
The rtl8723bu wireless IC shows evidence of a more agressive approach to
power saving, powering down its RF side when there is no wireless
interfacing but leaving USB interfacing intact. This makes the wireless
IC more suitable for use in devices which need to keep their power use
as low as practical, such as tablets and Surface Pro type devices.
In effect this means that a full initialisation must be performed
whenever a wireless interface is brought up. It also means that
interpretations of power status from general wireless registers should
not be relied on to influence an init sequence.
The patch works by forcing a fuller initialisation and forcing it to
occur more often in code paths (such as occurs during a low level
authentication that initiates wireless interfacing).
The initialisation sequence is now more consistent with code based
directly on vendor code. For example while the vendor derived code
interprets a register as indcating a particular powered state, it does
not use this information to influence its init sequence.
Only devices that use the rtl8723bu driver are affected by this patch.
With this patch wpa_supplicant reliably and consistently connects with
an AP. Before a workaround such as executing rmmod and modprobe before
each call to wpa_supplicant worked with some distributions.
Signed-off-by: John Heenan
---
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 16
1 file changed, 12 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..ab2f2ef 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -3900,7 +3900,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
* Fix 92DU-VC S3 hang with the reason is that secondary mac is not
* initialized. First MAC returns 0xea, second MAC returns 0x00
*/
- if (val8 == 0xea)
+ if (val8 == 0xea || priv->fops == &rtl8723bu_fops)
macpower = false;
else
macpower = true;
@@ -5779,6 +5779,12 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
ret = 0;
+ if(priv->fops == &rtl8723bu_fops) {
+ 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,9 +6086,11 @@ static int rtl8xxxu_probe(struct usb_interface
*interface,
goto exit;
}
- ret = rtl8xxxu_init_device(hw);
- if (ret)
- goto exit;
+ if(priv->fops != &rtl8723bu_fops) {
+ 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;
--
2.10.1
Re: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
Barry Day has submitted real world reports for the 8192eu and 8192cu.
This needs to be acknowledged. I have submitted real world reports for
the 8723bu.
When it comes down to it, it looks like the kernel code changes are
really going to be very trivial to fix this problem and we need to
take the focus off dramatic outbursts over style issues to a strategy
for getting usable results from real world testing.
Addressing style issues in a dramatic manner to me looks like a mean
sport for maintainers who line up to easy target first time
contributors. This mean attitude comes from the top with a well known
comment about "publicly making fun of people". The polite comments
over style from Joe Perches and Rafał Miłecki are welcomed.
An effective strategy would be to insert some printk statements to
trace what init steps vendor derived drivers do each time
wpa_supplicant is called and ask real world testers to report their
results. This is a lot more productive and less error prone than
laboriously pouring over vendor source code. Alternative drivers that
use vendor code from Realtek is enormously complicated and a huge pain
to make sense of.
Joe Sorensen's driver code is far easier to make sense of and it is a
shame Realtek don't come to the party. Joe Sorensens's code take takes
advantage of the excellent work of kernel contributors to the mac80211
driver.
Previous comments I made about enable_rf, rtl8xxxu_start,
rtl8xxxu_init_device etc should be clarified. I will leave it for the
moment as it currently serves no direct useful purpose.
John Heenan
On 1 November 2016 at 17:24, John Heenan wrote:
> I have a prepared another patch that is not USB VID/PID dependent for
> rtl8723bu devices. It is more elegant. I will send it after this
> email.
>
> If I have more patches is it preferable I just put them on github only
> and notify a link address until there might be some resolution?
>
> What I meant below about not finding a matching function is that I
> cannot find matching actions to take on any function calls in
> rtl8xxxu_stop as for rtl8xxxu_start.
>
> The function that is called later and potentially more than once for
> rtl8723bu devices is rtl8xxxu_init_device. There is no corresponding
> rtl8xxxu_deinit_device function.
>
> enable_rf is called in rtl8xxxu_start, not in the delayed call to
> rtl8xxxu_init_device. The corresponding disable_rf is called in
> rtl8xxxu_stop. So no matching issue here. However please see below for
> another potential issue.
>
> power_on is called in rtl8xxxu_init_device. power_off is called in
> rtl8xxxu_disconnect. Does not appear to be an issue if calling
> power_on has no real effect if already on.
>
> The following should be looked at though. For all devices enable_rf is
> only called once. For the proposed patch the first call to
> rtl8xxxu_init_device is called after the single call to enable_rf for
> rtl8723bu devices. Without the patch enable_rf is called after
> rtl8xxxu_init_device for all devices
>
> Perhaps enable_rf just configures RF modes to start up when RF is
> powered on or if called after power_on then enters requested RF modes.
>
> So I cannot see appropriate additional matching action to take.
>
> Below is some background for anyone interested
>
> rtl8xxxu_start and rtl8xxxu_stop are assigned to rtl8xxxu_ops.
>
> rtl8xxxu_probe assigns rtl8xxxu_ops to its driver layer with
> ieee80211_register_hw.
>
> rtl8xxxu_disconnect unassigns rtl8xxxu_ops from its driver layer with
> ieee80211_unregister_hw.
>
> rtl8xxxu_probe and rtl8xxxu_disconnect are USB driver functions
>
> John Heenan
>
>
> On 1 November 2016 at 08:15, John Heenan wrote:
>> On 1 November 2016 at 07:25, Jes Sorensen wrote:
>>> John Heenan writes:
>>
>>>
>>>> @@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>>>> struct rtl8xxxu_tx_urb *tx_urb;
>>>> unsigned long flags;
>>>> int ret, i;
>>>> + struct usb_device_descriptor *udesc = &priv->udev->descriptor;
>>>>
>>>> ret = 0;
>>>>
>>>> + if(udesc->idVendor == USB_VENDOR_ID_REALTEK
>>>> + && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
>>>> + ret = rtl8xxxu_init_device(hw);
>>>> + if (ret)
>>>> + goto error_out;
>>>> + }
>>>> +
>>>
>>> As mentioned previously, if this is to be changed here, it has to be
>>> matched in the _stop section too.
>>
>> I looked at this and could not find a matching function. I will have a
>> look again.
>>
Re: [PATCH 1/2] rtl8xxxu: Fix for authentication failure
On 3 November 2016 at 11:00, Larry Finger 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
>> ---
>> 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
