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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAye0QP0FQetaaa-RE23teVgZ335ELE8O5qnzMA7CwQ4T8wc_Q@mail.gmail.com>
Date:   Wed, 2 Nov 2016 11:13:34 +1000
From:   John Heenan <john@...s.com>
To:     Jes Sorensen <Jes.Sorensen@...hat.com>,
        Barry Day <briselec@...il.com>,
        Rafał Miłecki <zajec5@...il.com>
Cc:     Kalle Valo <kvalo@...eaurora.org>, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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 <john@...s.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@...s.com> wrote:
>> On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@...hat.com> wrote:
>>> John Heenan <john@...s.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.
>>