lkml.org 
[lkml]   [2016]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
    From
    Date
    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.

    \
     
     \ /
      Last update: 2016-11-01 04:03    [W:4.339 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site