| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <88809ae495005b8295d0c46261041c73a225359f.camel@sipsolutions.net>
Date: Thu, 04 Sep 2025 13:22:21 +0200
From: Johannes Berg <johannes@...solutions.net>
To: Jeff Chen <jeff.chen_1@....com>, linux-wireless@...r.kernel.org
Cc: 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 01/22] wifi: nxpwifi: add 802.11n files
On Mon, 2025-08-04 at 23:39 +0800, Jeff Chen wrote:
> +static bool
> +nxpwifi_is_tx_ba_stream_ptr_valid(struct nxpwifi_private *priv,
> + struct nxpwifi_tx_ba_stream_tbl *tx_tbl_ptr)
> +{
> + struct nxpwifi_tx_ba_stream_tbl *tx_ba_tsr_tbl;
> + bool ret = false;
> + int tid;
> +
> + tid = tx_tbl_ptr->tid;
> + rcu_read_lock();
> + list_for_each_entry_rcu(tx_ba_tsr_tbl, &priv->tx_ba_stream_tbl_ptr[tid], list) {
> + if (tx_ba_tsr_tbl == tx_tbl_ptr) {
> + ret = true;
> + break;
> + }
> + }
> + rcu_read_unlock();
> + return ret;
that might be nicer with guard(rcu)() :)
> +/* This function returns the pointer to an entry in BA Stream
> + * table which matches the given RA/TID pair.
> + */
> +struct nxpwifi_tx_ba_stream_tbl *
> +nxpwifi_get_ba_tbl(struct nxpwifi_private *priv, int tid, u8 *ra)
> +{
> + struct nxpwifi_tx_ba_stream_tbl *tx_ba_tsr_tbl, *found = NULL;
> +
> + list_for_each_entry_rcu(tx_ba_tsr_tbl, &priv->tx_ba_stream_tbl_ptr[tid], list) {
> + if (ether_addr_equal_unaligned(tx_ba_tsr_tbl->ra, ra) &&
> + tx_ba_tsr_tbl->tid == tid) {
> + found = tx_ba_tsr_tbl;
> + break;
> + }
> + }
> + return found;
> +}
and this could just return directly out of the loop, and not need the
'found' variable?
> + /* We don't wait for the response of this command */
> + ret = nxpwifi_send_cmd(priv, HOST_CMD_11N_ADDBA_REQ,
> + 0, 0, &add_ba_req, false);
> +
> + return ret;
doesn't need 'ret' either
> +int nxpwifi_send_delba(struct nxpwifi_private *priv, int tid, u8 *peer_mac,
> + int initiator)
> +{
> + struct host_cmd_ds_11n_delba delba;
> + int ret;
> + u16 del_ba_param_set;
> +
> + memset(&delba, 0, sizeof(delba));
> +
> + del_ba_param_set = tid << DELBA_TID_POS;
> +
> + if (initiator)
> + del_ba_param_set |= IEEE80211_DELBA_PARAM_INITIATOR_MASK;
> + else
> + del_ba_param_set &= ~IEEE80211_DELBA_PARAM_INITIATOR_MASK;
> +
> + delba.del_ba_param_set = cpu_to_le16(del_ba_param_set);
> + memcpy(&delba.peer_mac_addr, peer_mac, ETH_ALEN);
> +
> + /* We don't wait for the response of this command */
> + ret = nxpwifi_send_cmd(priv, HOST_CMD_11N_DELBA,
> + HOST_ACT_GEN_SET, 0, &delba, false);
> +
> + return ret;
same here
> +/* This function sends delba to specific tid
> + */
> +void nxpwifi_11n_delba(struct nxpwifi_private *priv, int tid)
> +{
> + struct nxpwifi_rx_reorder_tbl *rx_reor_tbl_ptr;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(rx_reor_tbl_ptr, &priv->rx_reorder_tbl_ptr[tid], list) {
> + if (rx_reor_tbl_ptr->tid == tid) {
> + dev_dbg(priv->adapter->dev,
> + "Send delba to tid=%d, %pM\n",
> + tid, rx_reor_tbl_ptr->ta);
> + nxpwifi_send_delba(priv, tid, rx_reor_tbl_ptr->ta, 0);
> + goto exit;
> + }
> + }
> +exit:
> + rcu_read_unlock();
guard() is nice to not have such patterns
> +static inline u8
> +nxpwifi_is_station_ampdu_allowed(struct nxpwifi_private *priv,
> + struct nxpwifi_ra_list_tbl *ptr, int tid)
> +{
> + struct nxpwifi_sta_node *node;
> + u8 ret;
> +
> + rcu_read_lock();
> + node = nxpwifi_get_sta_entry(priv, ptr->ra);
> + if (unlikely(!node))
> + ret = false;
> + else
> + ret = (node->ampdu_sta[tid] != BA_STREAM_NOT_ALLOWED) ? true : false;
> + rcu_read_unlock();
> + return ret;
also here with guard() you don't need the variable and it's probably
easier to read/understand since it could just be
guard(rcu)();
node = ...;
if (!node) return false;
if (node->... == NOT_ALLOWED) return false;
return true;
(modulo whitespace, obviously)
> +}
> +
> +/* This function checks whether AMPDU is allowed or not for a particular TID. */
> +static inline u8
> +nxpwifi_is_ampdu_allowed(struct nxpwifi_private *priv,
> + struct nxpwifi_ra_list_tbl *ptr, int tid)
"is allowed" sounds boolean, the function returns boolean, but is
declared as returning u8.
> +/* This function checks whether AMSDU is allowed or not for a particular TID.
> + */
> +static inline u8
> +nxpwifi_is_amsdu_allowed(struct nxpwifi_private *priv, int tid)
same
> +{
> + return (((priv->aggr_prio_tbl[tid].amsdu != BA_STREAM_NOT_ALLOWED) &&
> + (priv->is_data_rate_auto || !(priv->bitmap_rates[2] & 0x03)))
> + ? true : false);
might be nice to write that with multiple if statements perhaps, less
obfuscated ;-)
> +}
> +
> +/* This function checks whether a space is available for new BA stream or not.
> + */
> +static inline u8
> +nxpwifi_space_avail_for_new_ba_stream(struct nxpwifi_adapter *adapter)
same, since it doesn't return how much space is available, just bool
> +/* This function checks whether associated station is 11n enabled
> + */
> +static inline int nxpwifi_is_sta_11n_enabled(struct nxpwifi_private *priv,
> + struct nxpwifi_sta_node *node)
and here it's int, which is probably not much better than u8 for what
should be bool
(I'd almost think you could even instruct an LLM to find these so I'm
going to stop looking for them.)
Side note on the comment style: we removed quite a while ago the special
networking requirement for
/* long ...
* comment
*/
so feel free to
/*
* long ...
* comment
*/
as everywhere else in the kernel.
But again I'm just briefly scanning through this (and will continue),
I'm not going to really review it in depth.
(One thing I also noticed is some misspellings of "MHz" as "Mhz", not
sure where though.)
johannes
Powered by blists - more mailing lists