Message-ID: <aNz/vOlApzVzMLZy@nxpwireless-Inspiron-14-Plus-7440>
Date: Wed, 1 Oct 2025 18:17:32 +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 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:
> > 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, 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.
> 
> > 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