| Message ID | 20240820-mwifiex-cleanup-v1-10-320d8de4a4b7@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: mwifiex: cleanup driver | expand |
> From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: Tuesday, August 20, 2024 7:56 PM > To: Brian Norris <briannorris@chromium.org>; Francesco Dolcini > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de> > Subject: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > Align multiline if() under the opening brace. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/net/wireless/marvell/mwifiex/wmm.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c > b/drivers/net/wireless/marvell/mwifiex/wmm.c > index bcb61dab7dc86..1b1222c73728f 100644 > --- a/drivers/net/wireless/marvell/mwifiex/wmm.c > +++ b/drivers/net/wireless/marvell/mwifiex/wmm.c > @@ -1428,13 +1428,13 @@ mwifiex_dequeue_tx_packet(struct > mwifiex_adapter *adapter) > } > > if (!ptr->is_11n_enabled || > - ptr->ba_status || > - priv->wps.session_enable) { > + ptr->ba_status || > + priv->wps.session_enable) { > if (ptr->is_11n_enabled && > - ptr->ba_status && > - ptr->amsdu_in_ampdu && > - mwifiex_is_amsdu_allowed(priv, tid) && > - mwifiex_is_11n_aggragation_possible(priv, ptr, > + ptr->ba_status && > + ptr->amsdu_in_ampdu && > + mwifiex_is_amsdu_allowed(priv, tid) && > + mwifiex_is_11n_aggragation_possible(priv, ptr, > > adapter->tx_buf_size)) > mwifiex_11n_aggregate_pkt(priv, ptr, ptr_index); > /* ra_list_spinlock has been freed in > > -- > 2.39.2 > I wonder we still need patch for indent issue here? If so I am sure we will need a bunch of similar patches which I don't think really help improve mwifiex quality Actually in its successor Nxpwifi (currently under review), we have cleaned up all indent, and checkpatch errors/warnings/checks. I would suggest for Mwifiex we only take real bug fixes (Odd fixes). •WARNING: It's generally not useful to have the filename in the file •WARNING: Possible unnecessary 'out of memory' message •WARNING: space prohibited between function name and open parenthesis '(‘ •WARNING: Consecutive strings are generally better as a single string •WARNING: unchecked sscanfreturn value •WARNING: Single statement macros should not use a do {} while (0) loop •WARNING: do {} while (0) macros should not be semicolon terminated •WARNING: macros should not use a trailing semicolon •ERROR: Use C99 flexible arrays - see https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays •WARNING: Block comments use * on subsequent lines •WARNING: const array should probably be static const •WARNING: Prefer using '"%s...", __func__' to using 'mwifiex_register', this function's name, in a string •WARNING: braces {} are not necessary for any arm of this statement •WARNING: Prefer strscpyover strlcpy- see: https://github.com/KSPP/linux/issues/89 •WARNING: Statements should start on a tabstop •WARNING: Prefer strscpyover strcpy- see: https://github.com/KSPP/linux/issues/88 •WARNING: Unnecessary space before function pointer arguments •WARNING: Possible repeated word: 'send’ •WARNING: Possible repeated word: 'enabled’ •WARNING: quoted string split across lines •WARNING: ENOSYS means 'invalid syscallnr' and nothing else •WARNING:simple_strtolis obsolete, use kstrtolinstead •ERROR: code indent should use tabs where possible •WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 •CHECK: Avoid CamelCase: <HostCmd_CMD_ADD_NEW_STATION> •WARNING: void function return statements are not generally useful •WARNING: suspect code indent for conditional statements •WARNING: braces {} are not necessary for single statement blocks •WARNING: Avoid multiple line dereference •WARNING: function definition argument ‘xxx' should also have an identifier name •WARNING: else is not generally useful after a break or return •WARNING: Missing a blank line after declarations •WARNING: Block comments use a trailing */ on a separate line •ERROR: need consistent spacing around '+’ •ERROR: open brace '{' following function definitions go on the next line •WARNING: Comparisons should place the constant on the right side of the test Thanks, David
On 22.08.2024 09:36:29, David Lin wrote: > I wonder we still need patch for indent issue here? If so I am sure we > will need a bunch of similar patches which I don't think really help > improve mwifiex quality mwifiex is the best mainline driver we have for these devices. > Actually in its successor Nxpwifi (currently under review), we have > cleaned up all indent, and checkpatch errors/warnings/checks. Public review? regards, Marc
On 22.08.2024 11:53:25, Marc Kleine-Budde wrote: > On 22.08.2024 09:36:29, David Lin wrote: > > I wonder we still need patch for indent issue here? If so I am sure we > > will need a bunch of similar patches which I don't think really help > > improve mwifiex quality > > mwifiex is the best mainline driver we have for these devices. > > > Actually in its successor Nxpwifi (currently under review), we have > > cleaned up all indent, and checkpatch errors/warnings/checks. > > Public review? Found it: https://lore.kernel.org/all/20240809094533.1660-1-yu-hao.lin@nxp.com/ Marc
> From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: Thursday, August 22, 2024 5:53 PM > To: David Lin <yu-hao.lin@nxp.com> > Cc: Sascha Hauer <s.hauer@pengutronix.de>; Brian Norris > <briannorris@chromium.org>; Francesco Dolcini <francesco@dolcini.it>; > Kalle Valo <kvalo@kernel.org>; linux-wireless@vger.kernel.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > Subject: Re: RE: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > On 22.08.2024 09:36:29, David Lin wrote: > > I wonder we still need patch for indent issue here? If so I am sure we > > will need a bunch of similar patches which I don't think really help > > improve mwifiex quality > > mwifiex is the best mainline driver we have for these devices. > Yes, we will continue to fix bugs of mwifiex just like we added the WPA3 support for it. > > Actually in its successor Nxpwifi (currently under review), we have > > cleaned up all indent, and checkpatch errors/warnings/checks. > > Public review? > Nxpwifi patch v2 had been submitted recently. > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 22.08.2024 11:59:55, David Lin wrote: > > From: Marc Kleine-Budde <mkl@pengutronix.de> > > Sent: Thursday, August 22, 2024 5:53 PM > > To: David Lin <yu-hao.lin@nxp.com> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>; Brian Norris > > <briannorris@chromium.org>; Francesco Dolcini <francesco@dolcini.it>; > > Kalle Valo <kvalo@kernel.org>; linux-wireless@vger.kernel.org; > > linux-kernel@vger.kernel.org; kernel@pengutronix.de > > Subject: Re: RE: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > > > On 22.08.2024 09:36:29, David Lin wrote: > > > I wonder we still need patch for indent issue here? If so I am sure we > > > will need a bunch of similar patches which I don't think really help > > > improve mwifiex quality > > > > mwifiex is the best mainline driver we have for these devices. > > > > Yes, we will continue to fix bugs of mwifiex just like we added the > WPA3 support for it. Why do you think 2 drivers are easier to handle/support/maintain/... than 1 driver, especially given the low available review bandwidth? regards, Marc
> From: Marc Kleine-Budde <mkl@pengutronix.de> > Sent: Thursday, August 22, 2024 8:05 PM > To: David Lin <yu-hao.lin@nxp.com> > Cc: Sascha Hauer <s.hauer@pengutronix.de>; Brian Norris > <briannorris@chromium.org>; Francesco Dolcini <francesco@dolcini.it>; > Kalle Valo <kvalo@kernel.org>; linux-wireless@vger.kernel.org; > linux-kernel@vger.kernel.org; kernel@pengutronix.de > Subject: Re: RE: RE: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > On 22.08.2024 11:59:55, David Lin wrote: > > > From: Marc Kleine-Budde <mkl@pengutronix.de> > > > Sent: Thursday, August 22, 2024 5:53 PM > > > To: David Lin <yu-hao.lin@nxp.com> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>; Brian Norris > > > <briannorris@chromium.org>; Francesco Dolcini > > > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org>; > > > linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > > kernel@pengutronix.de > > > Subject: Re: RE: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > > > > > On 22.08.2024 09:36:29, David Lin wrote: > > > > I wonder we still need patch for indent issue here? If so I am > > > > sure we will need a bunch of similar patches which I don't think > > > > really help improve mwifiex quality > > > > > > mwifiex is the best mainline driver we have for these devices. > > > > > > > Yes, we will continue to fix bugs of mwifiex just like we added the > > WPA3 support for it. > > Why do you think 2 drivers are easier to handle/support/maintain/... > than 1 driver, especially given the low available review bandwidth? > Nxpwifi is used to support new NXP WiFi chips. You can check the commit message of Nxpwifi to know the reason why we need a new driver to support new NXP WiFi chips. Thanks, David > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 22.08.2024 12:11:15, David Lin wrote: > > Why do you think 2 drivers are easier to handle/support/maintain/... > > than 1 driver, especially given the low available review bandwidth? > > > > Nxpwifi is used to support new NXP WiFi chips. You can check the commit message of Nxpwifi to > know the reason why we need a new driver to support new NXP WiFi chips. FTR: https://lore.kernel.org/all/20240809094533.1660-1-yu-hao.lin@nxp.com/ >> [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. Thanks for the clarification. regards, Marc
On Thu, Aug 22, 2024 at 09:36:29AM +0000, David Lin wrote: > > From: Sascha Hauer <s.hauer@pengutronix.de> > > Sent: Tuesday, August 20, 2024 7:56 PM > > To: Brian Norris <briannorris@chromium.org>; Francesco Dolcini > > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org> > > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de> > > Subject: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > > > Align multiline if() under the opening brace. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/net/wireless/marvell/mwifiex/wmm.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c > > b/drivers/net/wireless/marvell/mwifiex/wmm.c > > index bcb61dab7dc86..1b1222c73728f 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/wmm.c > > +++ b/drivers/net/wireless/marvell/mwifiex/wmm.c > > @@ -1428,13 +1428,13 @@ mwifiex_dequeue_tx_packet(struct > > mwifiex_adapter *adapter) > > } > > > > if (!ptr->is_11n_enabled || > > - ptr->ba_status || > > - priv->wps.session_enable) { > > + ptr->ba_status || > > + priv->wps.session_enable) { > > if (ptr->is_11n_enabled && > > - ptr->ba_status && > > - ptr->amsdu_in_ampdu && > > - mwifiex_is_amsdu_allowed(priv, tid) && > > - mwifiex_is_11n_aggragation_possible(priv, ptr, > > + ptr->ba_status && > > + ptr->amsdu_in_ampdu && > > + mwifiex_is_amsdu_allowed(priv, tid) && > > + mwifiex_is_11n_aggragation_possible(priv, ptr, > > > > adapter->tx_buf_size)) > > mwifiex_11n_aggregate_pkt(priv, ptr, ptr_index); > > /* ra_list_spinlock has been freed in > > > > -- > > 2.39.2 > > > > I wonder we still need patch for indent issue here? If so I am sure we > will need a bunch of similar patches which I don't think really help > improve mwifiex quality > > Actually in its successor Nxpwifi (currently under review), we have > cleaned up all indent, and checkpatch errors/warnings/checks. BTW you advertised nxpwifi not as a successor to mwifiex, but as the driver to be used for new chips. This means we still have to deal with the mwifiex driver in the future to support the old chips, so even if nxpwifi is merged it still makes sense to clean up mwifiex. Sascha
> From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: Monday, August 26, 2024 5:37 PM > To: David Lin <yu-hao.lin@nxp.com> > Cc: Brian Norris <briannorris@chromium.org>; Francesco Dolcini > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org>; > linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > kernel@pengutronix.de > Subject: Re: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > 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 Thu, Aug 22, 2024 at 09:36:29AM +0000, David Lin wrote: > > > From: Sascha Hauer <s.hauer@pengutronix.de> > > > Sent: Tuesday, August 20, 2024 7:56 PM > > > To: Brian Norris <briannorris@chromium.org>; Francesco Dolcini > > > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org> > > > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > > kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de> > > > Subject: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > > > > > Align multiline if() under the opening brace. > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > drivers/net/wireless/marvell/mwifiex/wmm.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c > > > b/drivers/net/wireless/marvell/mwifiex/wmm.c > > > index bcb61dab7dc86..1b1222c73728f 100644 > > > --- a/drivers/net/wireless/marvell/mwifiex/wmm.c > > > +++ b/drivers/net/wireless/marvell/mwifiex/wmm.c > > > @@ -1428,13 +1428,13 @@ mwifiex_dequeue_tx_packet(struct > > > mwifiex_adapter *adapter) > > > } > > > > > > if (!ptr->is_11n_enabled || > > > - ptr->ba_status || > > > - priv->wps.session_enable) { > > > + ptr->ba_status || > > > + priv->wps.session_enable) { > > > if (ptr->is_11n_enabled && > > > - ptr->ba_status && > > > - ptr->amsdu_in_ampdu && > > > - mwifiex_is_amsdu_allowed(priv, tid) && > > > - mwifiex_is_11n_aggragation_possible(priv, > ptr, > > > + ptr->ba_status && > > > + ptr->amsdu_in_ampdu && > > > + mwifiex_is_amsdu_allowed(priv, tid) && > > > + mwifiex_is_11n_aggragation_possible(priv, ptr, > > > > > > adapter->tx_buf_size)) > > > mwifiex_11n_aggregate_pkt(priv, ptr, > ptr_index); > > > /* ra_list_spinlock has been freed in > > > > > > -- > > > 2.39.2 > > > > > > > I wonder we still need patch for indent issue here? If so I am sure we > > will need a bunch of similar patches which I don't think really help > > improve mwifiex quality > > > > Actually in its successor Nxpwifi (currently under review), we have > > cleaned up all indent, and checkpatch errors/warnings/checks. > > BTW you advertised nxpwifi not as a successor to mwifiex, but as the driver to > be used for new chips. This means we still have to deal with the mwifiex driver > in the future to support the old chips, so even if nxpwifi is merged it still makes > sense to clean up mwifiex. > > Sascha > Just like what I listed for the errors/warning/checks of Mwifiex running with checkpatch. Mwifiex has so many issues. As the driver will only support legacy devices and the state of it is "Odd fixes", It is better to fix really bugs of Mwifiex instead of cleanup it. David
On Mon, Aug 26, 2024 at 09:48:38AM +0000, David Lin wrote: > > From: Sascha Hauer <s.hauer@pengutronix.de> > > Sent: Monday, August 26, 2024 5:37 PM > > To: David Lin <yu-hao.lin@nxp.com> > > Cc: Brian Norris <briannorris@chromium.org>; Francesco Dolcini > > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org>; > > linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > kernel@pengutronix.de > > Subject: Re: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > > > 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 Thu, Aug 22, 2024 at 09:36:29AM +0000, David Lin wrote: > > > > From: Sascha Hauer <s.hauer@pengutronix.de> > > > > Sent: Tuesday, August 20, 2024 7:56 PM > > > > To: Brian Norris <briannorris@chromium.org>; Francesco Dolcini > > > > <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org> > > > > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > > > kernel@pengutronix.de; Sascha Hauer <s.hauer@pengutronix.de> > > > > Subject: [EXT] [PATCH 10/31] wifi: mwifiex: fix indention > > > > > > > > Align multiline if() under the opening brace. > > > > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > --- > > > > drivers/net/wireless/marvell/mwifiex/wmm.c | 12 ++++++------ > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c > > > > b/drivers/net/wireless/marvell/mwifiex/wmm.c > > > > index bcb61dab7dc86..1b1222c73728f 100644 > > > > --- a/drivers/net/wireless/marvell/mwifiex/wmm.c > > > > +++ b/drivers/net/wireless/marvell/mwifiex/wmm.c > > > > @@ -1428,13 +1428,13 @@ mwifiex_dequeue_tx_packet(struct > > > > mwifiex_adapter *adapter) > > > > } > > > > > > > > if (!ptr->is_11n_enabled || > > > > - ptr->ba_status || > > > > - priv->wps.session_enable) { > > > > + ptr->ba_status || > > > > + priv->wps.session_enable) { > > > > if (ptr->is_11n_enabled && > > > > - ptr->ba_status && > > > > - ptr->amsdu_in_ampdu && > > > > - mwifiex_is_amsdu_allowed(priv, tid) && > > > > - mwifiex_is_11n_aggragation_possible(priv, > > ptr, > > > > + ptr->ba_status && > > > > + ptr->amsdu_in_ampdu && > > > > + mwifiex_is_amsdu_allowed(priv, tid) && > > > > + mwifiex_is_11n_aggragation_possible(priv, ptr, > > > > > > > > adapter->tx_buf_size)) > > > > mwifiex_11n_aggregate_pkt(priv, ptr, > > ptr_index); > > > > /* ra_list_spinlock has been freed in > > > > > > > > -- > > > > 2.39.2 > > > > > > > > > > I wonder we still need patch for indent issue here? If so I am sure we > > > will need a bunch of similar patches which I don't think really help > > > improve mwifiex quality > > > > > > Actually in its successor Nxpwifi (currently under review), we have > > > cleaned up all indent, and checkpatch errors/warnings/checks. > > > > BTW you advertised nxpwifi not as a successor to mwifiex, but as the driver to > > be used for new chips. This means we still have to deal with the mwifiex driver > > in the future to support the old chips, so even if nxpwifi is merged it still makes > > sense to clean up mwifiex. > > > > Sascha > > > > Just like what I listed for the errors/warning/checks of Mwifiex running with checkpatch. > Mwifiex has so many issues. As the driver will only support legacy devices and the state of > it is "Odd fixes", It is better to fix really bugs of Mwifiex instead of cleanup it. The way you use "legacy" is from a silicon vendors perspective. Many real users only start to use a chip when it's already legacy for the silicon vendor. Sascha
diff --git a/drivers/net/wireless/marvell/mwifiex/wmm.c b/drivers/net/wireless/marvell/mwifiex/wmm.c index bcb61dab7dc86..1b1222c73728f 100644 --- a/drivers/net/wireless/marvell/mwifiex/wmm.c +++ b/drivers/net/wireless/marvell/mwifiex/wmm.c @@ -1428,13 +1428,13 @@ mwifiex_dequeue_tx_packet(struct mwifiex_adapter *adapter) } if (!ptr->is_11n_enabled || - ptr->ba_status || - priv->wps.session_enable) { + ptr->ba_status || + priv->wps.session_enable) { if (ptr->is_11n_enabled && - ptr->ba_status && - ptr->amsdu_in_ampdu && - mwifiex_is_amsdu_allowed(priv, tid) && - mwifiex_is_11n_aggragation_possible(priv, ptr, + ptr->ba_status && + ptr->amsdu_in_ampdu && + mwifiex_is_amsdu_allowed(priv, tid) && + mwifiex_is_11n_aggragation_possible(priv, ptr, adapter->tx_buf_size)) mwifiex_11n_aggregate_pkt(priv, ptr, ptr_index); /* ra_list_spinlock has been freed in
Align multiline if() under the opening brace. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/wireless/marvell/mwifiex/wmm.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)