| Message ID | 20231128083115.613235-1-yu-hao.lin@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | wifi: mwifiex: added code to support host mlme. | expand |
Hello Lin,
thanks for the patches here, I can clearly see that this code is going
through some real testing given the improvements you did lately.
I have commented on the single patches, and honestly I did not look into
the code details at the moment.
The major feedback from me is the following:
1 - you should not add code with a bug and than fix a bug in the same
series, you should have a non buggy patch in the first place (e.g.
git --amend). (this applies till the patch is not merged into the
maintainer tree, of course).
2 - point 1 applies also to reviewer comments
3 - if you have fixes that are not connected to the feature addition
you are doing is beneficial to have those separated, this makes
reviewing easier, they can be "prioritized" to some extent (given
that they are fixes) and follow a slightly different patch flow
(they can get applied, depending on the maintainers decision, when the
merge window is closed and should be backported). Not to mention
that smaller patch series are appreciated, "Maximum of 7-12 patches
per patchset " from [1]
In general I would suggest you to have a look at [1], not sure how up to
date is that compared to the in-tree Documentation/process/.
On Tue, Nov 28, 2023 at 04:31:03PM +0800, David Lin wrote:
> 5. Address reviewer comments.
You should list the changes you did, something that generic is forcing
the reviewer to compare v7 vs v6 to known what changed.
[1] https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Francesco
Francesco Dolcini <francesco@dolcini.it> writes: > Hello Lin, > thanks for the patches here, I can clearly see that this code is going > through some real testing given the improvements you did lately. > > I have commented on the single patches, and honestly I did not look into > the code details at the moment. > > The major feedback from me is the following: > 1 - you should not add code with a bug and than fix a bug in the same > series, you should have a non buggy patch in the first place (e.g. > git --amend). (this applies till the patch is not merged into the > maintainer tree, of course). > 2 - point 1 applies also to reviewer comments > 3 - if you have fixes that are not connected to the feature addition > you are doing is beneficial to have those separated, this makes > reviewing easier, they can be "prioritized" to some extent (given > that they are fixes) and follow a slightly different patch flow > (they can get applied, depending on the maintainers decision, when the > merge window is closed and should be backported). Not to mention > that smaller patch series are appreciated, "Maximum of 7-12 patches > per patchset " from [1] > > In general I would suggest you to have a look at [1], not sure how up to > date is that compared to the in-tree Documentation/process/. I haven't looked at the actual patches but a generic comment from me is that usually it's not a good idea for newcomers to submit a huge patchset like this. Start with something small, just with one patch first, learn the process and what we require from patches. After you have gained more knowledge you can start doing more complex stuff.
> From: Francesco Dolcini <francesco@dolcini.it> > Sent: Friday, December 1, 2023 7:49 PM > To: David Lin <yu-hao.lin@nxp.com> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > briannorris@chromium.org; kvalo@kernel.org; francesco@dolcini.it; Pete > Hsieh <tsung-hsien.hsieh@nxp.com> > Subject: [EXT] Re: [PATCH v7 00/12] wifi: mwifiex: added code to support > host mlme. > > 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 > > > Hello Lin, > thanks for the patches here, I can clearly see that this code is going through > some real testing given the improvements you did lately. > > I have commented on the single patches, and honestly I did not look into the > code details at the moment. > > The major feedback from me is the following: > 1 - you should not add code with a bug and than fix a bug in the same > series, you should have a non buggy patch in the first place (e.g. > git --amend). (this applies till the patch is not merged into the > maintainer tree, of course). > 2 - point 1 applies also to reviewer comments > 3 - if you have fixes that are not connected to the feature addition > you are doing is beneficial to have those separated, this makes > reviewing easier, they can be "prioritized" to some extent (given > that they are fixes) and follow a slightly different patch flow > (they can get applied, depending on the maintainers decision, when > the > merge window is closed and should be backported). Not to mention > that smaller patch series are appreciated, "Maximum of 7-12 patches > per patchset " from [1] > > In general I would suggest you to have a look at [1], not sure how up to date > is that compared to the in-tree Documentation/process/. > > On Tue, Nov 28, 2023 at 04:31:03PM +0800, David Lin wrote: > > 5. Address reviewer comments. > You should list the changes you did, something that generic is forcing the > reviewer to compare v7 vs v6 to known what changed. Can I summary what should I do and hopefully I can make agreement with you: 1. Separate patch v7 6/12 as a single patch. 2. Merged all other patches as a single patch for host mlme. So there should be no patch v8 and only have two patches, one for host mlme and another one to fix hostap restart issue. > > > [1] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwireles > s.wiki.kernel.org%2Fen%2Fdevelopers%2Fdocumentation%2Fsubmittingpatch > es&data=05%7C01%7Cyu-hao.lin%40nxp.com%7Cde461eef26c44e28e80c08d > bf2638639%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63837028 > 1495779090%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj > oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=y > OW9SndPm%2FDuI%2BZU8fxBieerDxVZp5RzefQiSfFA%2BW0%3D&reserved=0 > > Francesco
> From: Kalle Valo <kvalo@kernel.org> > Sent: Friday, December 1, 2023 8:25 PM > To: Francesco Dolcini <francesco@dolcini.it> > Cc: David Lin <yu-hao.lin@nxp.com>; linux-wireless@vger.kernel.org; > linux-kernel@vger.kernel.org; briannorris@chromium.org; Pete Hsieh > <tsung-hsien.hsieh@nxp.com> > Subject: [EXT] Re: [PATCH v7 00/12] wifi: mwifiex: added code to support > host mlme. > > 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 > > > Francesco Dolcini <francesco@dolcini.it> writes: > > > Hello Lin, > > thanks for the patches here, I can clearly see that this code is going > > through some real testing given the improvements you did lately. > > > > I have commented on the single patches, and honestly I did not look > > into the code details at the moment. > > > > The major feedback from me is the following: > > 1 - you should not add code with a bug and than fix a bug in the same > > series, you should have a non buggy patch in the first place (e.g. > > git --amend). (this applies till the patch is not merged into the > > maintainer tree, of course). > > 2 - point 1 applies also to reviewer comments > > 3 - if you have fixes that are not connected to the feature addition > > you are doing is beneficial to have those separated, this makes > > reviewing easier, they can be "prioritized" to some extent (given > > that they are fixes) and follow a slightly different patch flow > > (they can get applied, depending on the maintainers decision, when > the > > merge window is closed and should be backported). Not to mention > > that smaller patch series are appreciated, "Maximum of 7-12 patches > > per patchset " from [1] > > > > In general I would suggest you to have a look at [1], not sure how up > > to date is that compared to the in-tree Documentation/process/. > > I haven't looked at the actual patches but a generic comment from me is that > usually it's not a good idea for newcomers to submit a huge patchset like > this. Start with something small, just with one patch first, learn the process > and what we require from patches. After you have gained more knowledge > you can start doing more complex stuff. > Can you help to check this patch? https://patchwork.kernel.org/project/linux-wireless/patch/20231128082544.613179-1-yu-hao.lin@nxp.com/ This is a very small patch and it really fixed issue of mwifiex. > -- > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw > ork.kernel.org%2Fproject%2Flinux-wireless%2Flist%2F&data=05%7C01%7Cyu- > hao.lin%40nxp.com%7C47171ff92b204d48d7be08dbf26894c4%7C686ea1d3b > c2b4c6fa92cd99c5c301635%7C0%7C0%7C638370303262144837%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > wiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2FwH3UEUxKckAwlAVJkh > 5LpR2L76a1uyOmxXVWiyTUmQ%3D&reserved=0 > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwireles > s.wiki.kernel.org%2Fen%2Fdevelopers%2Fdocumentation%2Fsubmittingpatch > es&data=05%7C01%7Cyu-hao.lin%40nxp.com%7C47171ff92b204d48d7be08d > bf26894c4%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63837030 > 3262144837%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj > oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=F > I%2BwsMoSatv3woPhT2yWhHeFIi5pP1uzFQYUrynAZO0%3D&reserved=0
On Fri, Dec 01, 2023 at 11:05:47PM +0000, David Lin wrote: > > On Tue, Nov 28, 2023 at 04:31:03PM +0800, David Lin wrote: > > > 5. Address reviewer comments. > > You should list the changes you did, something that generic is forcing the > > reviewer to compare v7 vs v6 to known what changed. > > Can I summary what should I do and hopefully I can make agreement with you: > > 1. Separate patch v7 6/12 as a single patch. > 2. Merged all other patches as a single patch for host mlme. I would suggest to proceed the following way: 1. v8 of this series should have only 2 patches - PATCH v8 1/2 : add host mle station support. - PATCH v8 2/2 : add host mle AP support. Any kind of fix on these 2 new functionalities should be squashed in these single 2 patches. No commit to add a functionality with a bug that is fixed with a follow-up commit. If you discover bugs during your testing this is great, just amend the original commit that introduced it. I am assuming that is fair to implement station and AP support in separated patches, please speak up if this is not the case. 2. PATCH v7 06/12: this should be send as a new separate patch, with a Fixes: tag and Cc:stable. > So there should be no patch v8 and only have two patches, one for host > mlme and another one to fix hostap restart issue. It's ok to have a v8, restarting another series will be even more confusing IMO. Thanks! Francesco
> From: Francesco Dolcini <francesco@dolcini.it> > Sent: Wednesday, December 6, 2023 3:47 AM > To: David Lin <yu-hao.lin@nxp.com> > Cc: Francesco Dolcini <francesco@dolcini.it>; linux-wireless@vger.kernel.org; > linux-kernel@vger.kernel.org; briannorris@chromium.org; kvalo@kernel.org; > Pete Hsieh <tsung-hsien.hsieh@nxp.com> > Subject: Re: [EXT] Re: [PATCH v7 00/12] wifi: mwifiex: added code to support > host mlme. > > 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 Fri, Dec 01, 2023 at 11:05:47PM +0000, David Lin wrote: > > > On Tue, Nov 28, 2023 at 04:31:03PM +0800, David Lin wrote: > > > > 5. Address reviewer comments. > > > You should list the changes you did, something that generic is > > > forcing the reviewer to compare v7 vs v6 to known what changed. > > > > Can I summary what should I do and hopefully I can make agreement with > you: > > > > 1. Separate patch v7 6/12 as a single patch. > > 2. Merged all other patches as a single patch for host mlme. > > I would suggest to proceed the following way: > > 1. v8 of this series should have only 2 patches > - PATCH v8 1/2 : add host mle station support. > - PATCH v8 2/2 : add host mle AP support. > > Any kind of fix on these 2 new functionalities should be squashed in > these single 2 patches. No commit to add a functionality with a bug > that is fixed with a follow-up commit. If you discover bugs during > your testing this is great, just amend the original commit that > introduced it. > > I am assuming that is fair to implement station and AP support in > separated patches, please speak up if this is not the case. > > 2. PATCH v7 06/12: this should be send as a new separate patch, with a > Fixes: tag and Cc:stable. > > > So there should be no patch v8 and only have two patches, one for host > > mlme and another one to fix hostap restart issue. > > It's ok to have a v8, restarting another series will be even more confusing IMO. > Thanks for your suggestions. I will follow your suggestions to generate following patches. > > Thanks! > > Francesco
Patch v7: Changelogs since Patch v6: 1. Fix regression: Downlink throughput degraded by 70% in AP mode. 2. Fix issue: On STAUT, kernel Oops occurs when there is no association response from AP. 3. Fix issue: On STAUT, if AP leaves abruptly and deauth is missing, STA can't connect to AP anymore. 4. Fix regression: STA can't connect to AP when host_mlme is disabled (impact all chips). 5. Address reviewer comments. Patch v6: Correct mailing sequence. Patch v5: 1. Add host base MLME support to enable WPA3 functionalities for both STA and AP mode. 2. This feature (WPA3) required a firmware with corresponding Key API V2 support. 3. QA validation and regression have been covered for IW416. 4. This feature (WPA3) is currently enabled and verified only for IW416. 5. Changelogs since patch V4: a. Add WPA3 support for AP mode. b. Bug fix: In WPA3 STA mode, deice gets disconnected from AP when group rekey occurs. c. Bug fix: STAUT doesn't send WMM IE in association request when associate to a WMM-AP. Patch v4: 1. Refine code segment per review comment. 2. Add API to check firmware encryption key command version when host_mlme is enabled. Patch v3: Cleanup commit message. Patch v2: 1. Fix checkpatch error (pwe[1] -> pwe[0]). 2. Move module parameter 'host_mlme' to mwifiex_sdio_device structure. Default only enable for IW416. 3. Disable advertising NL80211_FEATURE_SAE if host_mlme is not enabled. David Lin (12): wifi: mwifiex: added code to support host mlme. wifi: mwifiex: fixed group rekey issue for WPA3. wifi: mwifiex: fixed reassocation issue for WPA3. wifi: mwifiex: fixed missing WMM IE for assoc req. wifi: mwifiex: supported host mlme for AP mode. wifi: mwifiex: added mac address for AP config. wifi: mwifiex: fixed TLV error for station add cmd. wifi: mwifiex: fixed the way to handle assoc timeout. wifi: mwifiex: fixed the way to handle link lost. wifi: mwifiex: fixed AP issue without host mlme. wifi: mwifiex: misc modifications for review comments. wifi: mwifiex: fixed compile and coding errors. .../net/wireless/marvell/mwifiex/cfg80211.c | 397 +++++++++++++++++- drivers/net/wireless/marvell/mwifiex/cmdevt.c | 35 +- drivers/net/wireless/marvell/mwifiex/decl.h | 25 +- drivers/net/wireless/marvell/mwifiex/fw.h | 61 +++ drivers/net/wireless/marvell/mwifiex/init.c | 6 + drivers/net/wireless/marvell/mwifiex/ioctl.h | 6 + drivers/net/wireless/marvell/mwifiex/join.c | 67 ++- drivers/net/wireless/marvell/mwifiex/main.c | 54 +++ drivers/net/wireless/marvell/mwifiex/main.h | 17 + drivers/net/wireless/marvell/mwifiex/scan.c | 6 + drivers/net/wireless/marvell/mwifiex/sdio.c | 13 + drivers/net/wireless/marvell/mwifiex/sdio.h | 2 + .../wireless/marvell/mwifiex/sta_cmdresp.c | 2 + .../net/wireless/marvell/mwifiex/sta_event.c | 36 +- .../net/wireless/marvell/mwifiex/sta_ioctl.c | 3 +- drivers/net/wireless/marvell/mwifiex/sta_tx.c | 9 +- .../net/wireless/marvell/mwifiex/uap_cmd.c | 180 ++++++++ drivers/net/wireless/marvell/mwifiex/util.c | 104 +++++ 18 files changed, 1003 insertions(+), 20 deletions(-) base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c