Message-ID: <aM2bmc49cJXDmcf3@nxpwireless-Inspiron-14-Plus-7440>
Date: Sat, 20 Sep 2025 02:06:17 +0800
From: Jeff Chen <jeff.chen_1@....com>
To: Johannes Berg <johannes@...solutions.net>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
	briannorris@...omium.org, francesco@...cini.it,
	tsung-hsien.hsieh@....com, s.hauer@...gutronix.de,
	brian.hsu@....com
Subject: Re: [PATCH v5 18/22] wifi: nxpwifi: add core files

On Thu, Sep 04, 2025 at 01:37:20 PM +0200, Johannes Berg wrote:
> On Mon, 2025-08-04 at 23:40 +0800, Jeff Chen wrote:
> > 
> > +/* 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,

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.

> 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.
 
> The whole function is also everything and the kitchen sink, could use
> some serious refactoring?
> 
> > +		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.

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

Powered by blists - more mailing lists