diff mbox series

[v2,11/17] wifi: cc33xx: Add init.c, init.h

Message ID 20240609182102.2950457-12-michael.nemanov@ti.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand

Commit Message

Michael Nemanov June 9, 2024, 6:20 p.m. UTC 
From: Michael Nemanov <Michael.Nemanov@ti.com>

High-level init code for new vifs
---
 drivers/net/wireless/ti/cc33xx/init.c | 236 ++++++++++++++++++++++++++
 drivers/net/wireless/ti/cc33xx/init.h |  15 ++
 2 files changed, 251 insertions(+)
 create mode 100644 drivers/net/wireless/ti/cc33xx/init.c
 create mode 100644 drivers/net/wireless/ti/cc33xx/init.h

Comments

Simon Horman June 15, 2024, 8:51 a.m. UTC | #1 
On Sun, Jun 09, 2024 at 09:20:56PM +0300, michael.nemanov@ti.com wrote:
> From: Michael Nemanov <Michael.Nemanov@ti.com>
> 
> High-level init code for new vifs
> ---
>  drivers/net/wireless/ti/cc33xx/init.c | 236 ++++++++++++++++++++++++++
>  drivers/net/wireless/ti/cc33xx/init.h |  15 ++
>  2 files changed, 251 insertions(+)
>  create mode 100644 drivers/net/wireless/ti/cc33xx/init.c
>  create mode 100644 drivers/net/wireless/ti/cc33xx/init.h
> 
> diff --git a/drivers/net/wireless/ti/cc33xx/init.c b/drivers/net/wireless/ti/cc33xx/init.c

...

> +int cc33xx_init_vif_specific(struct cc33xx *cc, struct ieee80211_vif *vif)
> +{
> +	struct cc33xx_vif *wlvif = cc33xx_vif_to_data(vif);
> +	struct conf_tx_ac_category *conf_ac;
> +	struct conf_tx_ac_category ac_conf[4];
> +	struct conf_tx_tid tid_conf[8];
> +	struct conf_tx_settings *tx_settings = &cc->conf.host_conf.tx;
> +	struct conf_tx_ac_category *p_wl_host_ac_conf = &tx_settings->ac_conf0;
> +	struct conf_tx_tid *p_wl_host_tid_conf = &tx_settings->tid_conf0;
> +	bool is_ap = (wlvif->bss_type == BSS_TYPE_AP_BSS);
> +	u8 ps_scheme = cc->conf.mac.ps_scheme;
> +	int ret, i;

...

> +	/* Default TID/AC configuration */
> +	WARN_ON(tx_settings->tid_conf_count != tx_settings->ac_conf_count);
> +	memcpy(ac_conf, p_wl_host_ac_conf, 4 * sizeof(struct conf_tx_ac_category));
> +	memcpy(tid_conf, p_wl_host_tid_conf, 8 * sizeof(struct conf_tx_tid));

Hi Michael,

allmodconfig builds on x86_64 with gcc-13 flag the following:

In file included from ./include/linux/string.h:374,
                 from ./include/linux/bitmap.h:13,
                 from ./include/linux/cpumask.h:13,
                 from ./arch/x86/include/asm/paravirt.h:21,
                 from ./arch/x86/include/asm/irqflags.h:60,
                 from ./include/linux/irqflags.h:18,
                 from ./include/linux/spinlock.h:59,
                 from ./include/linux/mmzone.h:8,
                 from ./include/linux/gfp.h:7,
                 from ./include/linux/firmware.h:8,
                 from drivers/net/wireless/ti/cc33xx/init.c:6:
In function 'fortify_memcpy_chk',
    inlined from 'cc33xx_init_vif_specific' at drivers/net/wireless/ti/cc33xx/init.c:156:2:
./include/linux/fortify-string.h:580:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
  580 |                         __read_overflow2_field(q_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'fortify_memcpy_chk',
    inlined from 'cc33xx_init_vif_specific' at drivers/net/wireless/ti/cc33xx/init.c:157:2:
./include/linux/fortify-string.h:580:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
  580 |                         __read_overflow2_field(q_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  CC [M]  drivers/net/wireless/ti/cc33xx/rx.o

I believe that this is because the destination for each of the two memcpy()
calls immediately above is too narrow - 1 structure wide instead of 4 or 8.

I think this can be resolved by either using:
1. struct_group in .../cc33xx/conf.h:struct conf_tx_settings
   to wrap ac_conf0 ... ac_conf3, and separately tid_conf0 ... tid_conf7.
2. Using arrays for ac_conf and tid_conf in
   .../cc33xx/conf.h:struct conf_tx_settings, in which case perhaps
   .../wlcore/conf.h:struct conf_tx_settings can be reused somehow
   (I did not check closely)?


Similar errors are flagged elsewhere in this series.
Please take a look at allmodconfig builds and make sure
no warnings are introduced.

Lastly, more related to the series as a whole than this patch in
particular, please consider running checkpatch.pl --codespell

Thanks!

...
Michael Nemanov June 20, 2024, 8:40 a.m. UTC | #2 
On 6/15/2024 11:51 AM, Simon Horman wrote:
...

> 
> Hi Michael,
> 
> allmodconfig builds on x86_64 with gcc-13 flag the following:
> 
> In file included from ./include/linux/string.h:374,
>                   from ./include/linux/bitmap.h:13,
>                   from ./include/linux/cpumask.h:13,
>                   from ./arch/x86/include/asm/paravirt.h:21,
>                   from ./arch/x86/include/asm/irqflags.h:60,
>                   from ./include/linux/irqflags.h:18,
>                   from ./include/linux/spinlock.h:59,
>                   from ./include/linux/mmzone.h:8,
>                   from ./include/linux/gfp.h:7,
>                   from ./include/linux/firmware.h:8,
>                   from drivers/net/wireless/ti/cc33xx/init.c:6:
> In function 'fortify_memcpy_chk',
>      inlined from 'cc33xx_init_vif_specific' at drivers/net/wireless/ti/cc33xx/init.c:156:2:
> ./include/linux/fortify-string.h:580:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
>    580 |                         __read_overflow2_field(q_size_field, size);
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In function 'fortify_memcpy_chk',
>      inlined from 'cc33xx_init_vif_specific' at drivers/net/wireless/ti/cc33xx/init.c:157:2:
> ./include/linux/fortify-string.h:580:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
>    580 |                         __read_overflow2_field(q_size_field, size);
>        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    CC [M]  drivers/net/wireless/ti/cc33xx/rx.o
> 
> I believe that this is because the destination for each of the two memcpy()
> calls immediately above is too narrow - 1 structure wide instead of 4 or 8.
> 
> I think this can be resolved by either using:
> 1. struct_group in .../cc33xx/conf.h:struct conf_tx_settings
>     to wrap ac_conf0 ... ac_conf3, and separately tid_conf0 ... tid_conf7.
> 2. Using arrays for ac_conf and tid_conf in
>     .../cc33xx/conf.h:struct conf_tx_settings, in which case perhaps
>     .../wlcore/conf.h:struct conf_tx_settings can be reused somehow
>     (I did not check closely)?
> 

Thank you for checking. I agree this code should be rewritten so it is 
more clear and w/o any warnings. Will fix.

I was unsuccessful reproducing the warning on my end. Tried with GCC 
13.2.0 (ARCH=x86_64, allmodconfig) and Arm GNU Toolchain 13.2 (ARCH=arm, 
allmodconfig) and only got errors in scan.c which I assume you refer to 
below (will also be fixed).

> 
> Similar errors are flagged elsewhere in this series.
> Please take a look at allmodconfig builds and make sure
> no warnings are introduced.
> 
> Lastly, more related to the series as a whole than this patch in
> particular, please consider running checkpatch.pl --codespell

Sure, will add checkpatch.pl --codespell to my tests.

Michael.
Simon Horman June 20, 2024, 4:30 p.m. UTC | #3 
On Thu, Jun 20, 2024 at 11:40:31AM +0300, Nemanov, Michael wrote:
> On 6/15/2024 11:51 AM, Simon Horman wrote:
> ...
> 
> > 
> > Hi Michael,
> > 
> > allmodconfig builds on x86_64 with gcc-13 flag the following:
> > 
> > In file included from ./include/linux/string.h:374,
> >                   from ./include/linux/bitmap.h:13,
> >                   from ./include/linux/cpumask.h:13,
> >                   from ./arch/x86/include/asm/paravirt.h:21,
> >                   from ./arch/x86/include/asm/irqflags.h:60,
> >                   from ./include/linux/irqflags.h:18,
> >                   from ./include/linux/spinlock.h:59,
> >                   from ./include/linux/mmzone.h:8,
> >                   from ./include/linux/gfp.h:7,
> >                   from ./include/linux/firmware.h:8,
> >                   from drivers/net/wireless/ti/cc33xx/init.c:6:
> > In function 'fortify_memcpy_chk',
> >      inlined from 'cc33xx_init_vif_specific' at drivers/net/wireless/ti/cc33xx/init.c:156:2:
> > ./include/linux/fortify-string.h:580:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> >    580 |                         __read_overflow2_field(q_size_field, size);
> >        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In function 'fortify_memcpy_chk',
> >      inlined from 'cc33xx_init_vif_specific' at drivers/net/wireless/ti/cc33xx/init.c:157:2:
> > ./include/linux/fortify-string.h:580:25: warning: call to '__read_overflow2_field' declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> >    580 |                         __read_overflow2_field(q_size_field, size);
> >        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    CC [M]  drivers/net/wireless/ti/cc33xx/rx.o
> > 
> > I believe that this is because the destination for each of the two memcpy()
> > calls immediately above is too narrow - 1 structure wide instead of 4 or 8.
> > 
> > I think this can be resolved by either using:
> > 1. struct_group in .../cc33xx/conf.h:struct conf_tx_settings
> >     to wrap ac_conf0 ... ac_conf3, and separately tid_conf0 ... tid_conf7.
> > 2. Using arrays for ac_conf and tid_conf in
> >     .../cc33xx/conf.h:struct conf_tx_settings, in which case perhaps
> >     .../wlcore/conf.h:struct conf_tx_settings can be reused somehow
> >     (I did not check closely)?
> > 
> 
> Thank you for checking. I agree this code should be rewritten so it is more
> clear and w/o any warnings. Will fix.
> 
> I was unsuccessful reproducing the warning on my end. Tried with GCC 13.2.0
> (ARCH=x86_64, allmodconfig) and Arm GNU Toolchain 13.2 (ARCH=arm,
> allmodconfig) and only got errors in scan.c which I assume you refer to
> below (will also be fixed).

Hi Michael,

I tried this again with GCC 13.2.0 on x86_64 with allmodconfig.
And I was able to see this with a W=1 (make W=1) build.

I don't think it is an important detail, but for reference,
I am using the compiler here, on an x86_64 host.
https://mirrors.edge.kernel.org/pub/tools/crosstool/

> > Similar errors are flagged elsewhere in this series.
> > Please take a look at allmodconfig builds and make sure
> > no warnings are introduced.
> > 
> > Lastly, more related to the series as a whole than this patch in
> > particular, please consider running checkpatch.pl --codespell
> 
> Sure, will add checkpatch.pl --codespell to my tests.

Great, thanks.
Michael Nemanov June 20, 2024, 4:52 p.m. UTC | #4 
On 6/20/2024 7:30 PM, Simon Horman wrote:

...

> Hi Michael,
> 
> I tried this again with GCC 13.2.0 on x86_64 with allmodconfig.
> And I was able to see this with a W=1 (make W=1) build.
> 

Oh it was the combination of CONFIG_FORTIFY_SOURCE=y (from allmodconfig) 
and W=1. Thanks, I see it now.

Michael.
diff mbox series

Patch

diff --git a/drivers/net/wireless/ti/cc33xx/init.c b/drivers/net/wireless/ti/cc33xx/init.c
new file mode 100644
index 000000000000..f338901ea96d
--- /dev/null
+++ b/drivers/net/wireless/ti/cc33xx/init.c
@@ -0,0 +1,236 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#include <linux/firmware.h>
+#include "acx.h"
+#include "cmd.h"
+#include "conf.h"
+#include "event.h"
+#include "tx.h"
+#include "init.h"
+
+static int cc33xx_init_phy_vif_config(struct cc33xx *cc,
+				      struct cc33xx_vif *wlvif)
+{
+	int ret;
+
+	ret = cc33xx_acx_slot(cc, wlvif, DEFAULT_SLOT_TIME);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int cc33xx_init_sta_beacon_filter(struct cc33xx *cc,
+					 struct cc33xx_vif *wlvif)
+{
+	int ret;
+
+	ret = cc33xx_acx_beacon_filter_table(cc, wlvif);
+	if (ret < 0)
+		return ret;
+
+	/* disable beacon filtering until we get the first beacon */
+	ret = cc33xx_acx_beacon_filter_opt(cc, wlvif, false);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int cc33xx_ap_init_templates(struct cc33xx *cc,
+				    struct ieee80211_vif *vif)
+{
+	struct cc33xx_vif *wlvif = cc33xx_vif_to_data(vif);
+	int ret;
+
+	/* when operating as AP we want to receive external beacons for
+	 * configuring ERP protection.
+	 */
+	ret = cc33xx_acx_beacon_filter_opt(cc, wlvif, false);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static void cc33xx_set_ba_policies(struct cc33xx *cc, struct cc33xx_vif *wlvif)
+{
+	/* Reset the BA RX indicators */
+	wlvif->ba_allowed = true;
+	cc->ba_rx_session_count = 0;
+
+	/* BA is supported in STA/AP modes */
+	wlvif->ba_support = (wlvif->bss_type != BSS_TYPE_AP_BSS &&
+				wlvif->bss_type != BSS_TYPE_STA_BSS);
+}
+
+/* vif-specifc initialization */
+static int cc33xx_init_sta_role(struct cc33xx *cc, struct cc33xx_vif *wlvif)
+{
+	int ret = cc33xx_acx_group_address_tbl(cc, true, NULL, 0);
+
+	if (ret < 0)
+		return ret;
+
+	/* Beacon filtering */
+	ret = cc33xx_init_sta_beacon_filter(cc, wlvif);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/* vif-specific initialization */
+static int cc33xx_init_ap_role(struct cc33xx *cc, struct cc33xx_vif *wlvif)
+{
+	int ret;
+
+	/* initialize Tx power */
+	ret = cc33xx_acx_tx_power(cc, wlvif, wlvif->power_level);
+	if (ret < 0)
+		return ret;
+
+	if (cc->radar_debug_mode)
+		cc33xx_cmd_generic_cfg(cc, wlvif,
+				       CC33XX_CFG_FEATURE_RADAR_DEBUG,
+				       cc->radar_debug_mode, 0);
+
+	return 0;
+}
+
+int cc33xx_init_vif_specific(struct cc33xx *cc, struct ieee80211_vif *vif)
+{
+	struct cc33xx_vif *wlvif = cc33xx_vif_to_data(vif);
+	struct conf_tx_ac_category *conf_ac;
+	struct conf_tx_ac_category ac_conf[4];
+	struct conf_tx_tid tid_conf[8];
+	struct conf_tx_settings *tx_settings = &cc->conf.host_conf.tx;
+	struct conf_tx_ac_category *p_wl_host_ac_conf = &tx_settings->ac_conf0;
+	struct conf_tx_tid *p_wl_host_tid_conf = &tx_settings->tid_conf0;
+	bool is_ap = (wlvif->bss_type == BSS_TYPE_AP_BSS);
+	u8 ps_scheme = cc->conf.mac.ps_scheme;
+	int ret, i;
+
+	/* consider all existing roles before configuring psm. */
+
+	if (cc->ap_count == 0 && is_ap) { /* first AP */
+		ret = cc33xx_acx_sleep_auth(cc, CC33XX_PSM_ELP);
+		if (ret < 0)
+			return ret;
+
+		/* unmask ap events */
+		cc->event_mask |= cc->ap_event_mask;
+
+	/* first STA, no APs */
+	} else if (cc->sta_count == 0 && cc->ap_count == 0 && !is_ap) {
+		u8 sta_auth = cc->conf.host_conf.conn.sta_sleep_auth;
+		/* Configure for power according to debugfs */
+		if (sta_auth != CC33XX_PSM_ILLEGAL)
+			ret = cc33xx_acx_sleep_auth(cc, sta_auth);
+		/* Configure for ELP power saving */
+		else
+			ret = cc33xx_acx_sleep_auth(cc, CC33XX_PSM_ELP);
+
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Mode specific init */
+	if (is_ap) {
+		ret = cc33xx_init_ap_role(cc, wlvif);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = cc33xx_init_sta_role(cc, wlvif);
+		if (ret < 0)
+			return ret;
+	}
+
+	cc33xx_init_phy_vif_config(cc, wlvif);
+
+	/* Default TID/AC configuration */
+	WARN_ON(tx_settings->tid_conf_count != tx_settings->ac_conf_count);
+	memcpy(ac_conf, p_wl_host_ac_conf, 4 * sizeof(struct conf_tx_ac_category));
+	memcpy(tid_conf, p_wl_host_tid_conf, 8 * sizeof(struct conf_tx_tid));
+
+	for (i = 0; i < tx_settings->tid_conf_count; i++) {
+		conf_ac =  &ac_conf[i];
+
+		/* If no ps poll is used, send legacy ps scheme in cmd */
+		if (ps_scheme == PS_SCHEME_NOPSPOLL)
+			ps_scheme = PS_SCHEME_LEGACY;
+
+		ret = cc33xx_tx_param_cfg(cc, wlvif, conf_ac->ac,
+					  conf_ac->cw_min, conf_ac->cw_max,
+					  conf_ac->aifsn, conf_ac->tx_op_limit,
+					  false, ps_scheme, conf_ac->is_mu_edca,
+					  conf_ac->mu_edca_aifs,
+					  conf_ac->mu_edca_ecw_min_max,
+					  conf_ac->mu_edca_timer);
+
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Mode specific init - post mem init */
+	if (is_ap)
+		ret = cc33xx_ap_init_templates(cc, vif);
+
+	if (ret < 0)
+		return ret;
+
+	/* Configure initiator BA sessions policies */
+	cc33xx_set_ba_policies(cc, wlvif);
+
+	return 0;
+}
+
+int cc33xx_hw_init(struct cc33xx *cc)
+{
+	cc33xx_acx_init_mem_config(cc);
+
+	cc33xx_debug(DEBUG_TX, "available tx blocks: %d", 16);
+	cc->last_fw_rls_idx = 0;
+	cc->partial_rx.status = CURR_RX_START;
+	return 0;
+}
+
+int cc33xx_download_ini_params_and_wait(struct cc33xx *cc)
+{
+	struct cc33xx_cmd_ini_params_download *cmd;
+	size_t command_size = ALIGN((sizeof(*cmd) + sizeof(cc->conf)), 4);
+	int ret;
+
+	cc33xx_set_max_buffer_size(cc, INI_MAX_BUFFER_SIZE);
+
+	cc33xx_debug(DEBUG_ACX,
+		     "Downloading INI configurations to FW, payload Length: %zu",
+		     sizeof(cc->conf));
+
+	cmd = kzalloc(command_size, GFP_KERNEL);
+	if (!cmd) {
+		cc33xx_set_max_buffer_size(cc, CMD_MAX_BUFFER_SIZE);
+		return -ENOMEM;
+	}
+
+	cmd->length = cpu_to_le32(sizeof(cc->conf));
+
+	/* copy INI file params payload */
+	memcpy((cmd->payload), &cc->conf, sizeof(cc->conf));
+
+	ret = cc33xx_cmd_send(cc, CMD_DOWNLOAD_INI_PARAMS,
+			      cmd, command_size, 0);
+	if (ret < 0) {
+		cc33xx_warning("download INI params to FW command sending failed: %d",
+			       ret);
+	} else {
+		cc33xx_debug(DEBUG_BOOT, "INI Params downloaded successfully");
+	}
+
+	cc33xx_set_max_buffer_size(cc, CMD_MAX_BUFFER_SIZE);
+	kfree(cmd);
+	return ret;
+}
diff --git a/drivers/net/wireless/ti/cc33xx/init.h b/drivers/net/wireless/ti/cc33xx/init.h
new file mode 100644
index 000000000000..b0bc6a548611
--- /dev/null
+++ b/drivers/net/wireless/ti/cc33xx/init.h
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#ifndef __INIT_H__
+#define __INIT_H__
+
+#include "cc33xx.h"
+
+int cc33xx_hw_init(struct cc33xx *cc);
+int cc33xx_download_ini_params_and_wait(struct cc33xx *cc);
+int cc33xx_init_vif_specific(struct cc33xx *cc, struct ieee80211_vif *vif);
+
+#endif /* __INIT_H__ */