Message-ID: <ZswTABUwME3pliKW@mozart.vkv.me>
Date: Sun, 25 Aug 2024 22:30:40 -0700
From: Calvin Owens <calvin@...nvd.org>
To: David Lin <yu-hao.lin@....com>
Cc: Sascha Hauer <s.hauer@...gutronix.de>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvalo@...nel.org" <kvalo@...nel.org>,
	"johannes@...solutions.net" <johannes@...solutions.net>,
	"briannorris@...omium.org" <briannorris@...omium.org>,
	"francesco@...cini.it" <francesco@...cini.it>,
	Pete Hsieh <tsung-hsien.hsieh@....com>,
	"kernel@...gutronix.de" <kernel@...gutronix.de>, calvin@...nvd.org
Subject: Re: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create nxpwifi to
 support iw61x

On Monday 08/26 at 02:56 +0000, David Lin wrote:
> > From: Calvin Owens <calvin@...nvd.org>
> > Sent: Monday, August 26, 2024 10:50 AM
> > To: David Lin <yu-hao.lin@....com>
> > Cc: Sascha Hauer <s.hauer@...gutronix.de>; linux-wireless@...r.kernel.org;
> > linux-kernel@...r.kernel.org; kvalo@...nel.org; johannes@...solutions.net;
> > briannorris@...omium.org; francesco@...cini.it; Pete Hsieh
> > <tsung-hsien.hsieh@....com>; kernel@...gutronix.de
> > Subject: Re: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create nxpwifi to support
> > iw61x
> > 
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report
> > this email' button
> > 
> > 
> > On Monday 08/26 at 02:33 +0000, David Lin wrote:
> > > > From: Calvin Owens <calvin@...nvd.org>
> > > > Sent: Monday, August 26, 2024 4:38 AM
> > > > To: Sascha Hauer <s.hauer@...gutronix.de>
> > > > Cc: David Lin <yu-hao.lin@....com>; linux-wireless@...r.kernel.org;
> > > > linux-kernel@...r.kernel.org; kvalo@...nel.org;
> > > > johannes@...solutions.net; briannorris@...omium.org;
> > > > francesco@...cini.it; Pete Hsieh <tsung-hsien.hsieh@....com>;
> > > > kernel@...gutronix.de; calvin@...nvd.org
> > > > Subject: [EXT] Re: [PATCH v2 00/43] wifi: nxpwifi: create nxpwifi to
> > > > support iw61x
> > > >
> > > > Caution: This is an external email. Please take care when clicking
> > > > links or opening attachments. When in doubt, report the message
> > > > using the 'Report this email' button
> > > >
> > > >
> > > > On Thursday 08/22 at 14:56 +0200, Sascha Hauer wrote:
> > > > > On Fri, Aug 09, 2024 at 05:44:50PM +0800, David Lin wrote:
> > > > > > This series adds support for IW61x which is a new family of
> > > > > > 2.4/5 GHz dual-band 1x1 Wi-Fi 6, Bluetooth/Bluetooth Low Energy
> > > > > > 5.2 and
> > > > > > 15.4 tri-radio single chip by NXP. These devices support
> > > > > > 20/40/80MHz single spatial stream in both STA and AP mode.
> > > > > > Communication to the IW61x is done via SDIO interface
> > > > > >
> > > > > > This driver is a derivative of existing Mwifiex [1] and based on
> > > > > > similar full-MAC architecture [2]. It has been tested with
> > > > > > i.MX8M Mini evaluation kits in both AP and STA mode.
> > > > > >
> > > > > > All code passes sparse and checkpatch
> > > > > >
> > > > > > Data sheet (require registration):
> > > > > > https://ww/
> > > > > > w.nxp.com%2Fproducts%2Fwireless-connectivity%2Fwi-fi-plus-blueto
> > > > > > oth-
> > > > > >
> > > >
> > &data=05%7C02%7Cyu-hao.lin%40nxp.com%7Cff25728795724a618a5208dcc5
> > > > 45c
> > > > > >
> > > >
> > 5fd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63860215067862
> > > > 3224%
> > > > > >
> > > >
> > 7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> > > > TiI6
> > > > > >
> > > >
> > Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=U0Cej8ysBD%2Fg1Sa4Ia
> > > > Ph63Ot
> > > > > > iTcemadiCfMINYM%2BRL4%3D&reserved=0
> > > > > > plus-802-15-4/2-4-5-ghz-dual-band-1x1-wi-fi-6-802-11ax-plus-blue
> > > > > > toot
> > > > > > h-5-
> > > > > > 4-plus-802-15-4-tri-radio-solution:IW612
> > > > > >
> > > > > > Known gaps to be addressed in the following patches,
> > > > > >   - Enable 11ax capabilities. This initial patch support up to 11ac.
> > > > > >   - Support DFS channel. This initial patch doesn't support DFS channel
> > in
> > > > > >     both AP/STA mode.
> > > > > >
> > > > > > This patch is presented as a request for comment with the
> > > > > > intention of being made into a patch after initial feedbacks are
> > > > > > addressed
> > > > > >
> > > > > > [1] We had considered adding IW61x to mwifiex driver, however due to
> > > > > >     FW architecture, host command interface and supported features
> > are
> > > > > >     significantly different, we have to create the new nxpwifi driver.
> > > > > >     Subsequent NXP chipsets will be added and sustained in this
> > > > > > new
> > > > driver.
> > > > >
> > > > > I added IW61x support to the mwifiex driver and besides the VDLL
> > > > > handling which must be added I didn't notice any differences.
> > > > > There might be other differences, but I doubt that these can't be
> > > > > integrated into the mwifiex driver.
> > > >
> > > > Hi Sascha,
> > > >
> > > > I'd also love to see this patchset, if you're able to share it. I
> > > > can test on an
> > > > IW612 if that's helpful at all.
> > > >
> > > > > Honestly I don't think adding a new driver is a good ideai, given
> > > > > how big wifi drivers are and how limited the review bandwidth is.
> > > > >
> > > > > What we'll end up with is that we'll receive the same patches for
> > > > > both drivers, or worse, only for one driver while the other stays
> > unpatched.
> > > >
> > > > I have some concrete experience with "in-tree driver forks" like this:
> > > > a pair of SCSI drivers named mpt2sas and mpt3sas.
> > > >
> > > > The latter was created as a near copy of the former:
> > > >
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > >
> > t.kernel%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C582c3c0573b74f83
> > 42
> > > >
> > 3408dcc579bc4a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > 60237
> > > >
> > 3871805816%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> > V2luM
> > > >
> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=bLjsRTRsR%2
> > BTtA
> > > > jUIVDY396ZF%2BIkwwUFhAubTCin3IVk%3D&reserved=0
> > >
> > > .org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fco
> > m
> > > >
> > mit%2F%3Fid%3Df92363d12359&data=05%7C02%7Cyu-hao.lin%40nxp.com%7
> > > >
> > Cff25728795724a618a5208dcc545c5fd%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > > >
> > 35%7C0%7C0%7C638602150678637352%7CUnknown%7CTWFpbGZsb3d8eyJW
> > > >
> > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0
> > > >
> > %7C%7C%7C&sdata=mzrLLqJNee7vIdV47j8xVSU%2FByjh%2FnNKnRsx1nw3yNo
> > > > %3D&reserved=0
> > > >
> > > > The result was *exactly* what you forsee happening here: both
> > > > drivers were constantly missing fixes from the other, and they were
> > > > just subtly different enough that it wasn't simple to "port" patches
> > > > from one to the other. It was a frustrating experience for everybody
> > > > involved. I think their git histories prove your point, I'd
> > > > encourage everyone with a horse in this race to take a look at them.
> > > >
> > > > It took three years to finally unify them:
> > > >
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > >
> > t.kernel%2F&data=05%7C02%7Cyu-hao.lin%40nxp.com%7C582c3c0573b74f83
> > 42
> > > >
> > 3408dcc579bc4a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> > 60237
> > > >
> > 3871815005%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> > V2luM
> > > >
> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=RfY6N6WWXI
> > n0gZP
> > > > SBoRySz5eeU8WkFH2HvFHLVNgu3Q%3D&reserved=0
> > >
> > > .org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fco
> > m
> > > >
> > mit%2F%3Fid%3Dc84b06a48c4d&data=05%7C02%7Cyu-hao.lin%40nxp.com%7
> > > >
> > Cff25728795724a618a5208dcc545c5fd%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > > >
> > 35%7C0%7C0%7C638602150678649431%7CUnknown%7CTWFpbGZsb3d8eyJW
> > > >
> > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0
> > > >
> > %7C%7C%7C&sdata=UGjDfngO1POWuydIfmOL%2BR%2BqJ1BoDQW6NboQUV
> > > > q2Xh8%3D&reserved=0
> > > >
> > > > I doubt anyone would disagree that wifi drivers are much more
> > > > complex than SCSI drivers. It would be strictly *worse* here, and
> > > > the path to unifying them strictly longer.
> > > >
> > > > Thanks,
> > > > Calvin
> > > >
> > >
> > > I think Nxpwifi will support NXP new WiFi chips and Mwifiex will support
> > existed NXP WiFi chips.
> > 
> > Hi David,
> > 
> > I understand that. I don't think that really changes anything: there will still be
> > many future patches which need to be applied to both, because the bug being
> > fixed existed before the fork. As the forked driver diverges, that will only
> > become more difficult and error prone.
> > 
> > Thanks,
> > Calvin
> > 
> 
> Nxpwifi is not only a fork from Mwifiex. Especially after we modified Nxpwifi
> based on the comments from Johannes.

I understand you've done real work here. But at the same time, nearly
1/3rd of the changeset against the original driver is renaming things:

    {0}[calvin ~/linux/drivers/net/wireless/nxp/nxpwifi] tar cf ~/nxpwifi-v2.tar .
    {0}[calvin ~/linux/drivers/net/wireless/nxp/nxpwifi] cd ../../marvell/mwifiex
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] tar xf ~/nxpwifi-v2.tar
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git add *
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git diff --cached --shortstat
     42 files changed, 13878 insertions(+), 14274 deletions(-)
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] sed -i 's/nxpwifi/mwifiex/g' *
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] sed -i 's/NXPWIFI/MWIFIEX/g' *
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git add *
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] git diff --cached --shortstat
     42 files changed, 9940 insertions(+), 10336 deletions(-)
    {0}[calvin ~/linux/drivers/net/wireless/marvell/mwifiex] cat * | wc -l
    45591

I'd consider that a fork. For those following at home, I pushed a branch
to github with nxpwifi v2 applied as a single patchbomb to mwifiex, with
the renames backed out:

    https://github.com/jcalvinowens/linux/tree/work/nxpwifi-on-mwifiex-no-renames

For my own selfish part, I agree with Sascha: I'd prefer to see that
changeset as patches against the existing driver, because I know from
experience that having a community built around one driver will enable
me to deliver better results for my users, and will make it easier for
me to contribute when I find bugs to fix.

But this is just one random opinion, I'm not who you need to convince :)

Thanks,
Calvin

> I think the real bugs fixes of Mwifiex will become less frequently.
> We can monitor these patches and apply them from Mwifiex to Nxpwifi.
>
> If we fix issues of Nxpwifi which is also related to Mwifiex, we will
> submit patches back to Mwifiex.
>
> Thanks,
> David 
> 

Powered by blists - more mailing lists