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

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

[John Heenan]

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 <john@zgus.com>
---
 .../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

[Jes Sorensen]

John Heenan <john@zgus.com> 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 <john@zgus.com>
> ---
>  .../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
> +

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.

>  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
  
>  	/* 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!

> @@ -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. It also has to be investigated exactly
why this matters for 8723bu. It is possible this matters for other
devices, but we need to find out exactly what causes this not moving a
major block of init code around like this.

>  	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;
> +	}

Again, this coding style abuse will never go into this driver,

Jes

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

[Jes Sorensen]

John Heenan <john-AMlJbUfTsso@public.gmane.org> 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 <john-AMlJbUfTsso@public.gmane.org>
> ---
>  .../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
> +

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.

>  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
  
>  	/* 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!

> @@ -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. It also has to be investigated exactly
why this matters for 8723bu. It is possible this matters for other
devices, but we need to find out exactly what causes this not moving a
major block of init code around like this.

>  	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;
> +	}

Again, this coding style abuse will never go into this driver,

Jes

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

[John Heenan]

On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> John Heenan <john@zgus.com> 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 <john@zgus.com>
>> ---
>>  .../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
>> +
>
> 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);
>> +             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.

>  It also has to be investigated exactly
> why this matters for 8723bu. It is possible this matters for other
> devices, but we need to find out exactly what causes this not moving a
> major block of init code around like this.

I have no problem with this.

Given what you said yesterday we are not going to get definitive
answers. It is going to have to be trial and error while tracing
through what vendor code does.

Apparently as far as Realtek are concerned, we are nobodies and they
want nothing to do with us.

>
>>       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;
>> +     }
>
> Again, this coding style abuse will never go into this driver,

As above, what abuse? I am not being facetious. Just puzzled.

> Jes

Again have a nice day!

John

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

[Barry Day]

On Mon, Oct 31, 2016 at 05:25:12PM -0400, Jes Sorensen wrote:
> As mentioned previously, if this is to be changed here, it has to be
> matched in the _stop section too. It also has to be investigated exactly
> why this matters for 8723bu. It is possible this matters for other
> devices, but we need to find out exactly what causes this not moving a
> major block of init code around like this.
>

I've tested this on the 8192e and 8192c. The same problems occurs with the
8192e but not the 8192c. Stopping and restarting wpa_supplicant had
no affect on the 8192c ability to connect to an AP.

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

[Jes Sorensen]

Barry Day <briselec@gmail.com> writes:
> On Mon, Oct 31, 2016 at 05:25:12PM -0400, Jes Sorensen wrote:
>> As mentioned previously, if this is to be changed here, it has to be
>> matched in the _stop section too. It also has to be investigated exactly
>> why this matters for 8723bu. It is possible this matters for other
>> devices, but we need to find out exactly what causes this not moving a
>> major block of init code around like this.
>
> I've tested this on the 8192e and 8192c. The same problems occurs with the
> 8192e but not the 8192c. Stopping and restarting wpa_supplicant had
> no affect on the 8192c ability to connect to an AP.

Which version of the driver? I did push in some changes for 8192eu
recently that fixed the issue with 8192eu driver reloading, and I would
be interested in knowing if this didn't fix the problem for you?

Second, could we please keep this on the linux-wireless list where it
belongs. Everybody else doesn't necessarily want to receive a copy via
netdev and linux-kernel

Jes

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

[Barry Day]

On Mon, Oct 31, 2016 at 06:41:30PM -0400, Jes Sorensen wrote:
> Barry Day <briselec@gmail.com> writes:
> > On Mon, Oct 31, 2016 at 05:25:12PM -0400, Jes Sorensen wrote:
> >> As mentioned previously, if this is to be changed here, it has to be
> >> matched in the _stop section too. It also has to be investigated exactly
> >> why this matters for 8723bu. It is possible this matters for other
> >> devices, but we need to find out exactly what causes this not moving a
> >> major block of init code around like this.
> >
> > I've tested this on the 8192e and 8192c. The same problems occurs with the
> > 8192e but not the 8192c. Stopping and restarting wpa_supplicant had
> > no affect on the 8192c ability to connect to an AP.
> 
> Which version of the driver? I did push in some changes for 8192eu
> recently that fixed the issue with 8192eu driver reloading, and I would
> be interested in knowing if this didn't fix the problem for you?
> 
> Second, could we please keep this on the linux-wireless list where it
> belongs. Everybody else doesn't necessarily want to receive a copy via
> netdev and linux-kernel
> 
> Jes

The tests were done with the latest version of rtl8xxxu-devel. I haven't
noticed any occurence of the previous issue with the 8192eu.

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

[Joe Perches]

On Tue, 2016-11-01 at 08:15 +1000, John Heenan wrote:
> > On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> > > > John Heenan <john@zgus.com> 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 <john@zgus.com>
> > > ---
> > >  .../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
> > > +
> > 
> > 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.

Typical kernel style would be:

	if (val == 0xea ||
	    (udesc->idVendor == USB_VENDOR_ID_REALTEK &&
	     udesc->idProduct == USB_PRODUCT_ID_RTL8723BU))
		macpower = false;
	else
		macpower = true;

ie: logical continuations at EOL and indentation aligned to parentheses
    using as many leading tabs as possible, then spaces where necessary

or maybe:

	macpower = !(val == 0xea ||
		     (idesc->idVendor == USB_VENDOR_ID_REALTEK &&
		      udesc->idProduct == USB_PRODUCT_ID_RTL8723BU));

but the first one seems easier to read.

> > > @@ -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;
> > > +     }
> > 
> > Again, this coding style abuse will never go into this driver,
> 
> As above, what abuse? I am not being facetious. Just puzzled.

Same logical continuation and indentation alignment.

>  Again have a nice day!

That's pleasant of you but Jen's rarely seems pleasant in return via
email.  I trust he's more personable over a beer though.  Perhaps one
day we'll all have one together.

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

[John Heenan]

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 <john@zgus.com> wrote:
> On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>> John Heenan <john@zgus.com> 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] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

[John Heenan]

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=C5=82 Mi=C5=82ecki 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 <john@zgus.com> 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 <john@zgus.com> wrote:
>> On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrot=
e:
>>> John Heenan <john@zgus.com> 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 =3D &priv->udev->descriptor;
>>>>
>>>>       ret =3D 0;
>>>>
>>>> +     if(udesc->idVendor =3D=3D USB_VENDOR_ID_REALTEK
>>>> +                     && udesc->idProduct =3D=3D USB_PRODUCT_ID_RTL872=
3BU) {
>>>> +             ret =3D 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] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

[John Heenan]

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 <john@zgus.com> 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 <john@zgus.com> wrote:
>> On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>>> John Heenan <john@zgus.com> 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] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC

[Jes Sorensen]

Barry Day <briselec@gmail.com> writes:
> On Mon, Oct 31, 2016 at 06:41:30PM -0400, Jes Sorensen wrote:
>> Barry Day <briselec@gmail.com> writes:
>> > On Mon, Oct 31, 2016 at 05:25:12PM -0400, Jes Sorensen wrote:
>> >> As mentioned previously, if this is to be changed here, it has to be
>> >> matched in the _stop section too. It also has to be investigated exactly
>> >> why this matters for 8723bu. It is possible this matters for other
>> >> devices, but we need to find out exactly what causes this not moving a
>> >> major block of init code around like this.
>> >
>> > I've tested this on the 8192e and 8192c. The same problems occurs with the
>> > 8192e but not the 8192c. Stopping and restarting wpa_supplicant had
>> > no affect on the 8192c ability to connect to an AP.
>> 
>> Which version of the driver? I did push in some changes for 8192eu
>> recently that fixed the issue with 8192eu driver reloading, and I would
>> be interested in knowing if this didn't fix the problem for you?
>> 
>> Second, could we please keep this on the linux-wireless list where it
>> belongs. Everybody else doesn't necessarily want to receive a copy via
>> netdev and linux-kernel
>
> The tests were done with the latest version of rtl8xxxu-devel. I haven't
> noticed any occurence of the previous issue with the 8192eu.

OK, I am back from my trip, but still burried alive catching up on
things. This is very much on the list of things I want to investigate.

Jes

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

[Jes Sorensen]

John Heenan <john@zgus.com> writes:
> 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.

Lets get this a little more clear - first of all, I have asked you to
investigate which part resolves the problem. Rather than 'I randomly
moved something around and it happens to work for me'.

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

Once bad code is in place, it is way harder to get rid of it again. It
is *normal* for maintainers to ask contributors to do things
correctly. In addition you have been asked repeatedly by multiple people
to respect coding style, but every patch you posted violated it again in
a different way, instead of spending the little time it would take for
you to get it right.

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

Now you are pissing on my name - do you really want to be taken
seriously here?

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

I have made it very clear I want this issue resolved, but I want it
done right.

Jes