| Message ID | 20240820-mwifiex-cleanup-v1-0-320d8de4a4b7@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | wifi: mwifiex: cleanup driver | expand |
Hello Sasha, thanks for the patches. Il 20 agosto 2024 13:55:25 CEST, Sascha Hauer <s.hauer@pengutronix.de> ha scritto: >This series has a bunch of cleanup and bugfix patches for the mwifiex >driver Would it make sense to sort the patch series in such a way that bug fixes can be backported to stable/long-term versions? Are those bug you could reproduce or just something you noticed while looking into the driver? ... > 24 files changed, 365 insertions(+), 729 deletions(-) I had a quick look at the series, and it looks fine to me, I'll try to have a proper look in the next couple of weeks. What I wonder is what's the risk of introducing some subtle bugs because of firmware differences, what device/firmware combination were you able to test? Francesco
Sascha Hauer <s.hauer@pengutronix.de> writes: > This series has a bunch of cleanup and bugfix patches for the mwifiex > driver. > [...] > --- > Sascha Hauer (31): BTW 30 patches is a lot to review. A good rule of thumb is to have max 12 patches per patchset, maybe a bit more if the patches are small. Splitting this into two patchsets would make it a lot more pleasent for the reviewers.
On Tue, Aug 20, 2024 at 03:34:12PM +0200, Francesco Dolcini wrote: > Hello Sasha, > thanks for the patches. > > Il 20 agosto 2024 13:55:25 CEST, Sascha Hauer <s.hauer@pengutronix.de> ha scritto: > >This series has a bunch of cleanup and bugfix patches for the mwifiex > >driver > > Would it make sense to sort the patch series in such a way that bug > fixes can be backported to stable/long-term versions? I anticipated this question, but I was too lazy this time. Given that Kalle asked to split up this series I'll reorder this series and send the bug fixes first. > Are those bug > you could reproduce or just something you noticed while looking into > the driver? There are bugs I noticed because the driver didn't work as expected which made me dive into this code. There are others that I only noticed by looking at the code. Sascha
On Tue, Aug 20, 2024 at 08:42:38PM +0300, Kalle Valo wrote: > Sascha Hauer <s.hauer@pengutronix.de> writes: > > > This series has a bunch of cleanup and bugfix patches for the mwifiex > > driver. > > > > [...] > > > --- > > Sascha Hauer (31): > > BTW 30 patches is a lot to review. A good rule of thumb is to have max > 12 patches per patchset, maybe a bit more if the patches are small. > Splitting this into two patchsets would make it a lot more pleasent for > the reviewers. Ok, I'll split up this series into smaller chunks. Be prepared there are more to come, this driver is really full of cruft... Sascha
On Tue, Aug 20, 2024 at 03:34:12PM +0200, Francesco Dolcini wrote: > Hello Sasha, > thanks for the patches. > > Il 20 agosto 2024 13:55:25 CEST, Sascha Hauer <s.hauer@pengutronix.de> ha scritto: > >This series has a bunch of cleanup and bugfix patches for the mwifiex > >driver > > > > 24 files changed, 365 insertions(+), 729 deletions(-) > > I had a quick look at the series, and it looks fine to me, I'll try to > have a proper look in the next couple of weeks. What I wonder is > what's the risk of introducing some subtle bugs because of firmware > differences, what device/firmware combination were you able to test? I tested this on a SD8978 aka iw416 SDIO card. I just tested this on a IW61x SDIO card (mainline support for this one still in my queue) and it didn't work, I'll investigate. Other than that I also have a PCIe card I'll test this on next time. I don't know there firmware versions currently, but I used the latest ones available in linux-firmware. Sascha
Sascha Hauer <s.hauer@pengutronix.de> writes: > On Tue, Aug 20, 2024 at 08:42:38PM +0300, Kalle Valo wrote: >> Sascha Hauer <s.hauer@pengutronix.de> writes: >> >> > This series has a bunch of cleanup and bugfix patches for the mwifiex >> > driver. >> > >> >> [...] >> >> > --- >> > Sascha Hauer (31): >> >> BTW 30 patches is a lot to review. A good rule of thumb is to have max >> 12 patches per patchset, maybe a bit more if the patches are small. >> Splitting this into two patchsets would make it a lot more pleasent for >> the reviewers. > > Ok, I'll split up this series into smaller chunks. Be prepared there are > more to come Awesome, keep them coming! > this driver is really full of cruft... Honestly I have not not looked at the driver in detail but that's what everyone are saying. So I appreciate you doing this. And I wish NXP would help you in the effort, this a great way to learn how things work in upstream.
This series has a bunch of cleanup and bugfix patches for the mwifiex driver. There's one topic which goes through the whole series: an instance of the mwifiex driver is referred to as an adapter (struct mwifiex_adapter). An adapter has an array of struct mwifiex_private pointers, each of them belongs to a virtual interface. Many functions throughout the driver take a struct mwifiex_private * as context, but are not specific to a virtual interface, but affect the whole adapter. In many places where a priv * is needed but none is available the code obtains a priv * by doing a: priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); Depending on where it is used this is either a bug or could at least be cleaned up. The driver currently has the send command function int mwifiex_send_cmd(struct mwifiex_private *priv, u16 cmd_no, u16 cmd_action, u32 cmd_oid, void *data_buf, bool sync) This series introduces a second send command function which takes the adapter as context: int mwifiex_adapter_send_cmd(struct mwifiex_adapter *adapter, u16 cmd_no, u16 cmd_action, u32 cmd_oid, void *data_buf, bool sync) Many places in the driver are converted to use this function instead and are also converted to take a struct mwifiex_adapter * as context themselves which makes the code better readable. Some places use this to obtain a priv *: priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA); This will return the first priv that is configured for station mode. Depending on the driver configuration there can be none or many privs in station mode, so priv can unexpectedly be NULL or in case there are multiple privs in station mode we might have to iterate over all of them. This is cleaned up in this series. The remaining patches are driveby catches I found when looking through the code. One patch worth mentioning here is "wifi: mwifiex: fix MAC address handling". Without it the mwifiex driver sets a new MAC address during a change_virtual_intf operation which is pretty unexpected by userspace and keeps the driver from working when a virtual interface is altered from its default mode it is initialized with. I've already sent "wifi: mwifiex: remove unnecessary priv checks" as a separate patch, I included here again as this series depends on it, but it's unchanged to the version I have sent earlier. Sascha Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- Sascha Hauer (31): wifi: mwifiex: remove unnecessary checks for valid priv wifi: mwifiex: use adapter as context pointer for mwifiex_hs_activated_event() wifi: mwifiex: drop HostCmd_CMD_802_11_MAC_ADDRESS response handling wifi: mwifiex: drop unnecessary initialization wifi: mwifiex: make region_code_mapping_t const wifi: mwifiex: use mwifiex_deauthenticate_all() wifi: mwifiex: pass adapter to mwifiex_dnld_cmd_to_fw() wifi: mwifiex: simplify mwifiex_setup_ht_caps() wifi: mwifiex: deduplicate code in mwifiex_cmd_tx_rate_cfg() wifi: mwifiex: fix indention wifi: mwifiex: use priv index as bss_num wifi: mwifiex: fix MAC address handling wifi: mwifiex: drop driver internal AP/STA limit counting wifi: mwifiex: iterate over privs in mwifiex_process_assoc_resp() wifi: mwifiex: add missing locking wifi: mwifiex: make locally used function static wifi: mwifiex: fix multiple station handling wifi: mwifiex: make mwifiex_enable_hs() safe for multiple station mode wifi: mwifiex: add function to send command specific to the adapter wifi: mwifiex: pass adapter to host sleep functions wifi: mwifiex: associate tx_power to the adapter wifi: mwifiex: pass adapter to mwifiex_init_shutdown_fw() wifi: mwifiex: pass adapter to mwifiex_disable_auto_ds() wifi: mwifiex: make txpwr specific to adapter wifi: mwifiex: return error on unexpected bss_num wifi: mwifiex: coalesce rules are adapter specific wifi: mwifiex: do not use mwifiex_get_priv() in mwifiex_dnld_sleep_confirm_cmd() wifi: mwifiex: move rx_ant/tx_ant to adapter wifi: mwifiex: pass adapter to mwifiex_fw_dump_event() wifi: mwifiex: move common settings out of switch/case wifi: mwifiex: allow to set MAC address in add_virtual_intf() drivers/net/wireless/marvell/mwifiex/11n.c | 2 - drivers/net/wireless/marvell/mwifiex/11n.h | 4 +- .../net/wireless/marvell/mwifiex/11n_rxreorder.c | 23 +- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 297 ++++----------------- drivers/net/wireless/marvell/mwifiex/cfp.c | 4 +- drivers/net/wireless/marvell/mwifiex/cmdevt.c | 123 +++++---- drivers/net/wireless/marvell/mwifiex/debugfs.c | 8 +- drivers/net/wireless/marvell/mwifiex/decl.h | 10 - drivers/net/wireless/marvell/mwifiex/init.c | 76 ++---- drivers/net/wireless/marvell/mwifiex/join.c | 3 +- drivers/net/wireless/marvell/mwifiex/main.c | 120 +++------ drivers/net/wireless/marvell/mwifiex/main.h | 112 +++----- drivers/net/wireless/marvell/mwifiex/pcie.c | 10 +- drivers/net/wireless/marvell/mwifiex/scan.c | 2 - drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +- drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 53 ++-- drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 70 ++--- drivers/net/wireless/marvell/mwifiex/sta_event.c | 3 +- drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 60 ++--- drivers/net/wireless/marvell/mwifiex/txrx.c | 18 +- drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 2 +- drivers/net/wireless/marvell/mwifiex/usb.c | 18 +- drivers/net/wireless/marvell/mwifiex/util.c | 48 ++-- drivers/net/wireless/marvell/mwifiex/wmm.c | 19 +- 24 files changed, 365 insertions(+), 729 deletions(-) --- base-commit: daaf0dd0398d5e93b7304f35184ca182ed583681 change-id: 20240820-mwifiex-cleanup-a2559d29e612 Best regards,