Message ID | 20240609182102.2950457-12-michael.nemanov@ti.com (mailing list archive) |
---|---|
State | Awaiting Upstream |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Not a local patch |
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! ...
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.
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.
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 --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__ */
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