netdev - Re: [PATCH 04/24] rtw89: add debug files

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] [day] [month] [year] [list]

Date:   Mon, 12 Jul 2021 18:40:36 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>,
        Ping-Ke Shih <pkshih@...ltek.com>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Sascha Hauer <kernel@...gutronix.de>,
        Kalle Valo <kvalo@...eaurora.org>
Subject: Re: [PATCH 04/24] rtw89: add debug files

On Fri, Jul 2, 2021 at 9:15 PM Oleksij Rempel <o.rempel@...gutronix.de> wrote:
> For example drivers/net/wireless/realtek/rtw89/debug.c is 2404 of potentially
> removable code. Some one should review it or outoptimize it by using
> existing frameworks.

Only ~28 of those lines are for the debug logging you point out.
(There's a few more in the .h file, but still.) Most of that is
unrelated code for dumping other debug info about the Realtek
chip/firmware/driver state, or performing other debugging operations.

> As you noticed, not many people are willing to review this driver. IMO,
> it is related to the RealTek reputation of making code drops with lots
> of not not usable or duplicated code. So to say - offloading the dirty work to
> the community. For example this patch set:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/staging/rts5139?h=v5.13&qt=author&q=rempel

FWIW, that driver was introduced into mainline (staging) by somebody @
Realsil, not Realtek:

https://git.kernel.org/linus/1dac4186bcc663cb8c2bcc59481aea8fe9124a6c

Sure, Realtek previously developed plenty of copy/paste/modify Linux
drivers, and it's likely most of that code was based off their
downstream drivers (which suffered from duplication), but they rarely
(never?) tried to get them merged upstream. Can you really blame them
for having non-upstream-friendly code in their
not-intended-for-upstream drivers? Now they've been nudged into doing
the upstream work themselves (*cough* *cough*) with rtw88 and now
rtw89, and IMO, they aren't suffering nearly of the same kinds of
duplication problems you note. But agreed, the reputational problems
might still be lingering.

If we're opining on lack of review: I haven't watched so many other
wireless drivers' review and inclusion in mainline (I tend to bother
with them once they're already mostly upstream, and I mostly just need
to fix bugs), but my impression is that the biggest impediment is
Kalle's limited resources. Most other successful drivers have
dedicated submaintainers who do the review or else append their name
on submissions and do pull requests, whether or not they got much
mailing list review. Everyone else who isn't so lucky has to wait in
line for Kalle, often for quite some time. This is getting a bit off
topic though.

> This new rtw89 driver seems to confirm this reputation, but I cani't say it
> for sure without spending a week on reverse engineering it.

FWIW, I've looked through it lightly (and I looked through rtw88 quite
a bit), and I don't see much (or any, really) of those same kinds of
problems. It's not perfect code of course, but I don't think
duplication is the biggest sort of problem.

Anyway, thanks for reviewing, and thanks for any issues you do point out!

Brian