| Message ID | 20190904180547.14809-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,D/E/Unstable,Pull] Enable Realtek Wireless Lan 8723DE | expand |
On Thu, Sep 05, 2019 at 02:05:47AM +0800, Kai-Heng Feng wrote: > BugLink: https://bugs.launchpad.net/bugs/1780590 > > [Impact] > There's no in-kernel support for Realtek 8723DE, so users need to use > out-of-tree DKMS which is not from Ubuntu archive. This has security > implication and should be avoided. Also this provides pretty bad user > experience. > > [Fix] > Add support to Realtek 8723DE. > All commits are cherry-picked from Realtek maintained repo: > https://github.com/rtlwifi-linux/rtw88_8723de > > [Test] > With the patch series applied, 8723DE can scan and connect to APs > succesfully. Also did some S3 smoke test, it continues to work. > > [Regression Potential] > Low. The device in question was never supported, and if there's any > regression, we can count on Realtek Wireless team, thy are now pretty > responsive on upstream mailing list. I have not had a chance to review this yet, however I did want to ask -- has the driver been reviewed to ensure that it is suitable to be signed by the build-time ephemaral module signing key? I.e. it does not expose any interfaces to userspace which would compromise kernel lockdown? Thanks, Seth
On Wed, Sep 11, 2019 at 7:35 PM Seth Forshee <seth.forshee@canonical.com> wrote: > > On Thu, Sep 05, 2019 at 02:05:47AM +0800, Kai-Heng Feng wrote: > > BugLink: https://bugs.launchpad.net/bugs/1780590 > > > > [Impact] > > There's no in-kernel support for Realtek 8723DE, so users need to use > > out-of-tree DKMS which is not from Ubuntu archive. This has security > > implication and should be avoided. Also this provides pretty bad user > > experience. > > > > [Fix] > > Add support to Realtek 8723DE. > > All commits are cherry-picked from Realtek maintained repo: > > https://github.com/rtlwifi-linux/rtw88_8723de > > > > [Test] > > With the patch series applied, 8723DE can scan and connect to APs > > succesfully. Also did some S3 smoke test, it continues to work. > > > > [Regression Potential] > > Low. The device in question was never supported, and if there's any > > regression, we can count on Realtek Wireless team, thy are now pretty > > responsive on upstream mailing list. > > I have not had a chance to review this yet, however I did want to ask -- > has the driver been reviewed to ensure that it is suitable to be signed > by the build-time ephemaral module signing key? I.e. it does not expose > any interfaces to userspace which would compromise kernel lockdown? The debugfs is enabled as Kconfig suggested, and all debugfs entries with write permission have input string checking, entries with read permissions don't expose sensitive information. So it looks good overall. Furthermore, "debugfs: Restrict debugfs when the kernel is locked down" should prevent malicious act from happening. Other than debugfs there are no userspace interfaces are exposed. Kai-Heng > > Thanks, > Seth
On 04.09.19 20:05, Kai-Heng Feng wrote: > BugLink: https://bugs.launchpad.net/bugs/1780590 > > [Impact] > There's no in-kernel support for Realtek 8723DE, so users need to use > out-of-tree DKMS which is not from Ubuntu archive. This has security > implication and should be avoided. Also this provides pretty bad user > experience. > > [Fix] > Add support to Realtek 8723DE. > All commits are cherry-picked from Realtek maintained repo: > https://github.com/rtlwifi-linux/rtw88_8723de > > [Test] > With the patch series applied, 8723DE can scan and connect to APs > succesfully. Also did some S3 smoke test, it continues to work. > > [Regression Potential] > Low. The device in question was never supported, and if there's any > regression, we can count on Realtek Wireless team, thy are now pretty > responsive on upstream mailing list. > > [Disco] > The following changes since commit 9155387474aad243521665dbb39cbb60fa2b2f28: > > KVM: PPC: Book3S: Add count cache flush parameters to kvmppc_get_cpu_char() (2019-09-03 23:06:56 -0400) > > are available in the Git repository at: > > https://git.launchpad.net/~kaihengfeng/linux lp1780590-disco > > for you to fetch changes up to f8ed6357e830b208af21dad780f34817c1d5ac69: > > UBUNTU: [Config] CONFIG_RTW88_8723DE=y (2019-09-04 15:56:47 +0800) > > [Eoan] > The following changes since commit a0ba553e8433044b1351b3805f6f5af89712da39: > > UBUNTU: SAUCE: Input: elantech - enable middle button for one more ThinkPad (2019-08-30 11:54:30 -0500) > > are available in the Git repository at: > > https://git.launchpad.net/~kaihengfeng/linux lp1780590-eoan > > for you to fetch changes up to ba57d3bad694710d48b26e7cfc3d249f28fda2b5: > > UBUNTU: [Config] CONFIG_RTW88_8723DE=y (2019-09-04 15:54:15 +0800) > > [Unstable] > The following changes since commit ff4e0cca519adb9b44d8fbaad998e159937a4b22: > > UBUNTU: Ubuntu-5.3.0-9.10 (2019-09-03 10:27:33 +0200) > > are available in the Git repository at: > > https://git.launchpad.net/~kaihengfeng/linux lp1780590-unstable > > for you to fetch changes up to c4eefc15ef3f978b69aacb8a348a06944e57971d: > > UBUNTU: [Config] CONFIG_RTW88_8723DE=y (2019-09-04 15:49:35 +0800) > > Delaying to consider this for D until resolved in E.
On Thu, Sep 05, 2019 at 02:05:47AM +0800, Kai-Heng Feng wrote: > BugLink: https://bugs.launchpad.net/bugs/1780590 > > [Impact] > There's no in-kernel support for Realtek 8723DE, so users need to use > out-of-tree DKMS which is not from Ubuntu archive. This has security > implication and should be avoided. Also this provides pretty bad user > experience. > > [Fix] > Add support to Realtek 8723DE. > All commits are cherry-picked from Realtek maintained repo: > https://github.com/rtlwifi-linux/rtw88_8723de > > [Test] > With the patch series applied, 8723DE can scan and connect to APs > succesfully. Also did some S3 smoke test, it continues to work. > > [Regression Potential] > Low. The device in question was never supported, and if there's any > regression, we can count on Realtek Wireless team, thy are now pretty > responsive on upstream mailing list. Sorry for the long delay in reviewing these, but it was quite a pile. On the one hand, the changes look like upstreamable sorts of changes, nothing obviously horrendous or anything like that. However it is a *lot* of changes, and not just adding new hw support. There's a very substantial amount of refactoring and enhancing the code for already supported hw. For that reason I don't think this is SRU material. For eoan, I'm still on the fence. I'd like to know what kind of regression testing has been done (even if it wasn't done by you personally, i.e. if realtek is validating the changes against supported hardware). The maintenance burden could also be an issue. Can we count on assistance from you/realtek if upstream fixes to the driver conflict with the very substantial changes made by these patches? And what's the story on these patches going upstream? When we start to think about having to maintain these on top of 5.4, then we're talking about a much longer timeframe for maintaining these patches. Will these be going into 5.4? If not, why? If we support this in eoan there's a presumption that we will continue to support it in FF, so I want to be sure that we don't end up with something that's going to be extremely difficult to maintain for the life of an LTS release. I'm also curious why you think we shouldn't add a dkms package for this driver instead of pulling the changes into the kernel. Granted that dkms isn't ideal, but neither is 16,000+ lines of non-upstream changes. Thanks, Seth
> On Sep 27, 2019, at 03:20, Seth Forshee <seth.forshee@canonical.com> wrote: > > On Thu, Sep 05, 2019 at 02:05:47AM +0800, Kai-Heng Feng wrote: >> BugLink: https://bugs.launchpad.net/bugs/1780590 >> >> [Impact] >> There's no in-kernel support for Realtek 8723DE, so users need to use >> out-of-tree DKMS which is not from Ubuntu archive. This has security >> implication and should be avoided. Also this provides pretty bad user >> experience. >> >> [Fix] >> Add support to Realtek 8723DE. >> All commits are cherry-picked from Realtek maintained repo: >> https://github.com/rtlwifi-linux/rtw88_8723de >> >> [Test] >> With the patch series applied, 8723DE can scan and connect to APs >> succesfully. Also did some S3 smoke test, it continues to work. >> >> [Regression Potential] >> Low. The device in question was never supported, and if there's any >> regression, we can count on Realtek Wireless team, thy are now pretty >> responsive on upstream mailing list. > > Sorry for the long delay in reviewing these, but it was quite a pile. Realtek finally replied to us. > > On the one hand, the changes look like upstreamable sorts of changes, > nothing obviously horrendous or anything like that. However it is a > *lot* of changes, and not just adding new hw support. There's a very > substantial amount of refactoring and enhancing the code for already > supported hw. For that reason I don't think this is SRU material. We even switched the driver for Realtek 8822BE WiFi from rtlwifi.ko to rtw88.ko during previous SRU cycle. > > For eoan, I'm still on the fence. I'd like to know what kind of > regression testing has been done (even if it wasn't done by you > personally, i.e. if realtek is validating the changes against supported > hardware). Realtek confirmed they tested against on all hardware the rtw88 driver supports. > > The maintenance burden could also be an issue. Can we count on > assistance from you/realtek if upstream fixes to the driver conflict > with the very substantial changes made by these patches? Yes, they are going to do that and I'll backport the driver when needed. > > And what's the story on these patches going upstream? When we start to > think about having to maintain these on top of 5.4, then we're talking > about a much longer timeframe for maintaining these patches. Will these > be going into 5.4? If not, why? If we support this in eoan there's a > presumption that we will continue to support it in FF, so I want to be > sure that we don't end up with something that's going to be extremely > difficult to maintain for the life of an LTS release. Yes, all of them will be upstreamed. Realtek expects everything lands to 5.6 or 5.7. It takes time because linux-wireless are congested so not everyone can respond immediately. It's hard to say the difficulty to backport future patches to 5.4, it largely depends on if there's another round of major net/mac80211 refactoring. What I can say is that I'll try my best to keep everything in line. > > I'm also curious why you think we shouldn't add a dkms package for this > driver instead of pulling the changes into the kernel. Granted that dkms > isn't ideal, but neither is 16,000+ lines of non-upstream changes. Realtek WiFi modules are mostly used in budget devices, and many of them still use spinning rust or eMMC. So recompiling DKMS can be a painful experience and I really hope we can avoid that. Kai-Heng > > Thanks, > Seth
On Wed, Oct 16, 2019 at 09:53:41PM +0800, Kai-Heng Feng wrote: > > > > On Sep 27, 2019, at 03:20, Seth Forshee <seth.forshee@canonical.com> wrote: > > > > On Thu, Sep 05, 2019 at 02:05:47AM +0800, Kai-Heng Feng wrote: > >> BugLink: https://bugs.launchpad.net/bugs/1780590 > >> > >> [Impact] > >> There's no in-kernel support for Realtek 8723DE, so users need to use > >> out-of-tree DKMS which is not from Ubuntu archive. This has security > >> implication and should be avoided. Also this provides pretty bad user > >> experience. > >> > >> [Fix] > >> Add support to Realtek 8723DE. > >> All commits are cherry-picked from Realtek maintained repo: > >> https://github.com/rtlwifi-linux/rtw88_8723de > >> > >> [Test] > >> With the patch series applied, 8723DE can scan and connect to APs > >> succesfully. Also did some S3 smoke test, it continues to work. > >> > >> [Regression Potential] > >> Low. The device in question was never supported, and if there's any > >> regression, we can count on Realtek Wireless team, thy are now pretty > >> responsive on upstream mailing list. > > > > Sorry for the long delay in reviewing these, but it was quite a pile. > > Realtek finally replied to us. > > > > > On the one hand, the changes look like upstreamable sorts of changes, > > nothing obviously horrendous or anything like that. However it is a > > *lot* of changes, and not just adding new hw support. There's a very > > substantial amount of refactoring and enhancing the code for already > > supported hw. For that reason I don't think this is SRU material. > > We even switched the driver for Realtek 8822BE WiFi from rtlwifi.ko to rtw88.ko during previous SRU cycle. > > > > > For eoan, I'm still on the fence. I'd like to know what kind of > > regression testing has been done (even if it wasn't done by you > > personally, i.e. if realtek is validating the changes against supported > > hardware). > > Realtek confirmed they tested against on all hardware the rtw88 driver supports. > > > > > The maintenance burden could also be an issue. Can we count on > > assistance from you/realtek if upstream fixes to the driver conflict > > with the very substantial changes made by these patches? > > Yes, they are going to do that and I'll backport the driver when needed. > > > > > And what's the story on these patches going upstream? When we start to > > think about having to maintain these on top of 5.4, then we're talking > > about a much longer timeframe for maintaining these patches. Will these > > be going into 5.4? If not, why? If we support this in eoan there's a > > presumption that we will continue to support it in FF, so I want to be > > sure that we don't end up with something that's going to be extremely > > difficult to maintain for the life of an LTS release. > > Yes, all of them will be upstreamed. Realtek expects everything lands to 5.6 or 5.7. It takes time because linux-wireless are congested so not everyone can respond immediately. > > It's hard to say the difficulty to backport future patches to 5.4, it largely depends on if there's another round of major net/mac80211 refactoring. > What I can say is that I'll try my best to keep everything in line. Okay, I'm open to pulling the patches into unstable, subject to monitoring their progress towards getting upstream. If progress stalls we may revert them before focal releases. However, the patches seem to need a refresh for 5.4, so nacking this pull request. Please send an updated pull request for unstable. Thanks, Seth