Re: [PATCH v5 18/22] wifi: nxpwifi: add core files

[Johannes Berg <johannes@sipsolutions.net>]

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.

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?

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.

> +/* CFG802.11

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

johannes