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