> From: Francesco Dolcini <[email protected]>
> Sent: Friday, December 1, 2023 7:49 PM
> To: David Lin <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Pete
> Hsieh <[email protected]>
> 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
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