| Message ID | 20250318050739.2239376-2-jeff.chen_1@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] wifi: mwifiex: Fix premature release of RF calibration data. | expand |
Hello Jeff, On Tue, Mar 18, 2025 at 01:07:38PM +0800, Jeff Chen wrote: > This patch resolves an issue where RF calibration data was being > released before the download process. Without this fix, the > external calibration data file would not be downloaded > at all. > > Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> The code looks ok to me, however I do not understand the commit you selected as fixes tag.
> -----Original Message----- > From: Francesco Dolcini <francesco@dolcini.it> > Sent: Thursday, March 20, 2025 12:29 AM > To: Jeff Chen <jeff.chen_1@nxp.com> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > briannorris@chromium.org; johannes@sipsolutions.net; francesco@dolcini.it; > Pete Hsieh <tsung-hsien.hsieh@nxp.com>; s.hauer@pengutronix.de > Subject: [EXT] Re: [PATCH v4 1/2] wifi: mwifiex: Fix premature release of RF > calibration data. > > > Hello Jeff, > > On Tue, Mar 18, 2025 at 01:07:38PM +0800, Jeff Chen wrote: > > This patch resolves an issue where RF calibration data was being > > released before the download process. Without this fix, the external > > calibration data file would not be downloaded at all. > > > > Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > The code looks ok to me, however I do not understand the commit you > selected as fixes tag. > > From what I understand releasing the data before using it was done since the > initial commit 388ec385d5ce ("mwifiex: add calibration data download > feature"). What am I missing? > > Francesco Hello Francesco, Thank you for reviewing the patch. You are correct-the Fixes tag I included was incorrect. After re-examining the issue, I found that the premature release of RF calibration data cannot be reproduced, which invalidates the problem statement for this patch. I have decided to withdraw the patch. I appreciate your feedback and attention to detail, which helped identify this oversight. Best regards, Jeff
On Tue, 2025-03-25 at 16:43 +0000, Jeff Chen wrote: > > > I have decided to withdraw the patch. I appreciate your feedback and attention to detail, > which helped identify this oversight. > This goes for _everyone_ on this thread... I applied this patch a long time ago. Whatever you need to fix, you need to send new patches. And I guess next time I'm not going to apply any patches for mwifiex however innocent they look ... thus making the situation of that driver even worse than it is now. So please get together and form a plan on how to maintain it. johannes
On Tue, Mar 25, 2025 at 05:45:10PM +0100, Johannes Berg wrote: > On Tue, 2025-03-25 at 16:43 +0000, Jeff Chen wrote: > > > > > > I have decided to withdraw the patch. I appreciate your feedback and attention to detail, > > which helped identify this oversight. > > > > This goes for _everyone_ on this thread... I applied this patch a long > time ago. Whatever you need to fix, you need to send new patches. If it needs withdrawn, I suppose Jeff should send a revert patch then. > And I guess next time I'm not going to apply any patches for mwifiex > however innocent they look ... thus making the situation of that driver > even worse than it is now. > > So please get together and form a plan on how to maintain it. My 2 cents: * Technically, I'm listed as maintainer still. I'm not always prompt, but I try to eventually get around to stuff (or at least see that Francesco reviews). I believe the previous implicit agreement would be that the wireless-drivers maintainer would wait for an Ack from sub-maintainer(s) before applying, unless they were truly trivial. I don't require that, of course, if you'd like to take things on your own Johannes, but that was my previous understanding. * I'm also used to seeing email replies when patches get applied. Kalle used to do that (presumably from some kind of push-time automation?), but I see you don't. You're of course free to do this however works best for you, but I find such emails useful for all interested parties (authors, reviewers, etc.). For example, if I thought the patches were controversial and were on my ToDo list, I'd probably speak up sooner. Brian
On Tue, Mar 25, 2025 at 04:43:33PM +0000, Jeff Chen wrote: > From: Francesco Dolcini <francesco@dolcini.it> > > On Tue, Mar 18, 2025 at 01:07:38PM +0800, Jeff Chen wrote: > > > This patch resolves an issue where RF calibration data was being > > > released before the download process. Without this fix, the external > > > calibration data file would not be downloaded at all. > > > > > > Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") > > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > > > The code looks ok to me, however I do not understand the commit you > > selected as fixes tag. > > > > From what I understand releasing the data before using it was done since the > > initial commit 388ec385d5ce ("mwifiex: add calibration data download > > feature"). What am I missing? > > Thank you for reviewing the patch. You are correct-the Fixes tag I included was incorrect. > After re-examining the issue, I found that the premature release of RF calibration data > cannot be reproduced, which invalidates the problem statement for this patch. > > I have decided to withdraw the patch. I appreciate your feedback and attention to detail, > which helped identify this oversight. To me the code change is correct, and it is also merged in wireless-next. No reason to drop it because of my comment on the fixes tag. Brian: are you ok with that? Francesco
On Wed, Mar 26, 2025 at 01:18:10PM +0100, Francesco Dolcini wrote: > On Tue, Mar 25, 2025 at 04:43:33PM +0000, Jeff Chen wrote: > > From: Francesco Dolcini <francesco@dolcini.it> > > > On Tue, Mar 18, 2025 at 01:07:38PM +0800, Jeff Chen wrote: > > > > This patch resolves an issue where RF calibration data was being > > > > released before the download process. Without this fix, the external > > > > calibration data file would not be downloaded at all. > > > > > > > > Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") > > > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > > > > > The code looks ok to me, however I do not understand the commit you > > > selected as fixes tag. > > > > > > From what I understand releasing the data before using it was done since the > > > initial commit 388ec385d5ce ("mwifiex: add calibration data download > > > feature"). What am I missing? > > > > Thank you for reviewing the patch. You are correct-the Fixes tag I included was incorrect. > > After re-examining the issue, I found that the premature release of RF calibration data > > cannot be reproduced, which invalidates the problem statement for this patch. > > > > I have decided to withdraw the patch. I appreciate your feedback and attention to detail, > > which helped identify this oversight. > > To me the code change is correct, and it is also merged in wireless-next. No > reason to drop it because of my comment on the fixes tag. > > Brian: are you ok with that? Oh, sorry, I don't think I really analyzed the nature of the reasons for "withdrawal". Yes, if it's just the Fixes tag, then reverting isn't even that helpful. I'm fine with keeping it as-is. Brian
> -----Original Message----- > From: Brian Norris <briannorris@chromium.org> > Sent: Wednesday, March 26, 2025 11:23 PM > > On Wed, Mar 26, 2025 at 01:18:10PM +0100, Francesco Dolcini wrote: > > On Tue, Mar 25, 2025 at 04:43:33PM +0000, Jeff Chen wrote: > > > From: Francesco Dolcini <francesco@dolcini.it> > > > > On Tue, Mar 18, 2025 at 01:07:38PM +0800, Jeff Chen wrote: > > > > > This patch resolves an issue where RF calibration data was being > > > > > released before the download process. Without this fix, the > > > > > external calibration data file would not be downloaded at all. > > > > > > > > > > Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") > > > > > Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> > > > > > > > > The code looks ok to me, however I do not understand the commit > > > > you selected as fixes tag. > > > > > > > > From what I understand releasing the data before using it was done > > > > since the initial commit 388ec385d5ce ("mwifiex: add calibration > > > > data download feature"). What am I missing? > > > > > > Thank you for reviewing the patch. You are correct-the Fixes tag I included > was incorrect. > > > After re-examining the issue, I found that the premature release of > > > RF calibration data cannot be reproduced, which invalidates the problem > statement for this patch. > > > > > > I have decided to withdraw the patch. I appreciate your feedback and > > > attention to detail, which helped identify this oversight. > > > > To me the code change is correct, and it is also merged in > > wireless-next. No reason to drop it because of my comment on the fixes tag. > > > > Brian: are you ok with that? > > Oh, sorry, I don't think I really analyzed the nature of the reasons for > "withdrawal". Yes, if it's just the Fixes tag, then reverting isn't even that helpful. > I'm fine with keeping it as-is. > > Brian Hi Johannes, Francesco, Brian, and all, Thank you for your feedback and for bringing attention to this discussion. Moving forward, I will work on improving the process to avoid similar issues and ensure better maintenance of the `mwifiex` driver. Thank you for your understanding and support. Best regards, Jeff
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c index 45eecb5f643b..b07cb302a00c 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -691,10 +691,6 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context) init_failed = true; done: - if (adapter->cal_data) { - release_firmware(adapter->cal_data); - adapter->cal_data = NULL; - } if (adapter->firmware) { release_firmware(adapter->firmware); adapter->firmware = NULL; diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c index e2800a831c8e..c0e6ce1a82fe 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -2293,9 +2293,13 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init) "marvell,caldata"); } - if (adapter->cal_data) + if (adapter->cal_data) { mwifiex_send_cmd(priv, HostCmd_CMD_CFG_DATA, HostCmd_ACT_GEN_SET, 0, NULL, true); + release_firmware(adapter->cal_data); + adapter->cal_data = NULL; + } + /* Read MAC address from HW */ ret = mwifiex_send_cmd(priv, HostCmd_CMD_GET_HW_SPEC,
This patch resolves an issue where RF calibration data was being released before the download process. Without this fix, the external calibration data file would not be downloaded at all. Fixes: d39fbc88956e ("mwifiex: remove cfg_data construction") Signed-off-by: Jeff Chen <jeff.chen_1@nxp.com> --- drivers/net/wireless/marvell/mwifiex/main.c | 4 ---- drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 6 +++++- 2 files changed, 5 insertions(+), 5 deletions(-)