On Sat, Sep 20, 2025 at 02:06:17 AM +0800, Jeff Chen wrote:

On Thu, Sep 04, 2025 at 01:37:20 PM +0200, Johannes Berg wrote:

quoted

On Mon, 2025-08-04 at 23:40 +0800, Jeff Chen wrote:

quoted

+/* The main process.
+ *
+ * This function is the main procedure of the driver and handles various driver
+ * operations. It runs in a loop and provides the core functionalities.
+ *
+ * The main responsibilities of this function are -
+ *      - Ensure concurrency control
+ *      - Handle pending interrupts and call interrupt handlers
+ *      - Wake up the card if required
+ *      - Handle command responses and call response handlers
+ *      - Handle events and call event handlers
+ *      - Execute pending commands
+ *      - Transmit pending data packets
+ */
+void nxpwifi_main_process(struct nxpwifi_adapter *adapter)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&adapter->main_proc_lock, flags);
+
+	/* Check if already processing */
+	if (adapter->nxpwifi_processing || adapter->main_locked) {
+		adapter->more_task_flag = true;
+		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
+		return;
+	}
+
+	adapter->nxpwifi_processing = true;
+	spin_unlock_irqrestore(&adapter->main_proc_lock, flags);

This makes me very nervous, it at least means it's super hard to
understand when this may or may not be running ... It's also the sort of
custom locking that's kind of frowned upon.

Hi Johannes, may I have your thoughts on the proposed plan to remove
custom locking and rely on workqueue.

thanks,
Jeff
 

Hi Johannes,

Thanks for the detailed feedback. We agree this is hard to reason about.
The use of "main_locked" and "more_task_flag" is a workaround to avoid
reentrancy and race conditions between SDIO interrupt and workqueue execution.
However, it introduces implicit state transitions that are difficult to follow.

quoted

Could this not be with wiphy mutex and be very clear? Though maybe you
wouldn't want TX to go through that ... and maybe it can't since sdio
calls it? But that seems odd, why is it both a worker and called for
every interrupt? Should it even be a single function for those two
cases?

Also it sets more_task_flag when it's entered while already running, but
that's just weird? Should other work coming in really get processed by
the SDIO interrupt processing?

It seems to me this is one of those awful design things inherited by
mwifiex that just happens to work? Can you document it well? If so maybe
do that and that can say why it really needs to be this way. If not, you
should probably change it completely and redesign it from first
principles, i.e. figure out what it has to do and build it accordingly?

We plan to remove this custom locking and instead rely solely on the workqueue
model. Specifically:

- SDIO interrupt will only queue "main_work", not call "nxpwifi_main_process()"
  directly.
- "nxpwifi_main_process()" will be the single consumer of all driver-side tasks.
- Interrupt status will be latched and processed in "nxpwifi_main_process()" to
  ensure no events are missed.

This change will eliminate the need for, "more_task_flag" and "main_proc_lock",
reduce concurrency complexity.

To better reflect its actual purpose, the main_locked flag will be renamed to
iface_changing. This flag is specifically used to prevent nxpwifi_main_process()
from running while cfg80211_ops.change_virtual_intf() is executing.

To ensure proper synchronization, iface_changing is always set/unset under
wiphy_lock(), which is held during change_virtual_intf(). In nxpwifi_main_process(),
we only read iface_changing, so to make this safe, we also hold wiphy_lock() while
reading it. This avoids races and makes the locking model clearer.
 

quoted

The whole function is also everything and the kitchen sink, could use
some serious refactoring?

quoted

+		if (adapter->delay_null_pkt && !adapter->cmd_sent &&
+		    !adapter->curr_cmd && !is_command_pending(adapter) &&
+		    (nxpwifi_wmm_lists_empty(adapter) &&
+		     nxpwifi_bypass_txlist_empty(adapter) &&
+		     skb_queue_empty(&adapter->tx_data_q))) {
+			if (!nxpwifi_send_null_packet
+			    (nxpwifi_get_priv(adapter, NXPWIFI_BSS_ROLE_STA),
+			     NXPWIFI_TxPD_POWER_MGMT_NULL_PACKET |
+			     NXPWIFI_TxPD_POWER_MGMT_LAST_PACKET)) {
+				adapter->delay_null_pkt = false;
+				adapter->ps_state = PS_STATE_SLEEP;
+			}
+			break;
+		}
+	} while (true);

Sao that ... those conditions are awful? If this were a separate
function at least you could write it in multiple lines with return
true/false there.

Absolutely agreed. The function is doing too much. We plan to refactor it into smaller,
purpose-specific helpers.

quoted

quoted

+/* CFG802.11

(side note: there's really no such thing as "CFG802.11" FWIW, it was
always just called "cfg80211")

johannes