Re: [PATCH 01/17] Add cc33xx.h, cc33xx_i.h

Krzysztof Kozlowski
2024-05-22T09:38:56Z

On 21/05/2024 19:18, michael.nemanov@ti.com wrote:
> From: Michael Nemanov <Michael.Nemanov@ti.com>
>
> These are header files with definitions common to the entire driver.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

>
> Signed-off-by: Michael Nemanov <michael.nemanov@ti.com>
> ---
> drivers/net/wireless/ti/cc33xx/cc33xx.h | 481 ++++++++++++++++++++++
> drivers/net/wireless/ti/cc33xx/cc33xx_i.h | 459 +++++++++++++++++++++
> 2 files changed, 940 insertions(+)
> create mode 100644 drivers/net/wireless/ti/cc33xx/cc33xx.h
> create mode 100644 drivers/net/wireless/ti/cc33xx/cc33xx_i.h
>
> diff --git a/drivers/net/wireless/ti/cc33xx/cc33xx.h b/drivers/net/wireless/ti/cc33xx/cc33xx.h
> new file mode 100644
> index 000000000000..3a2e56af4da7
> --- /dev/null
> +++ b/drivers/net/wireless/ti/cc33xx/cc33xx.h
> @@ -0,0 +1,481 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#ifndef __CC33XX_H__
> +#define __CC33XX_H__
> +
> +#include "cc33xx_i.h"
> +#include "rx.h"
> +
> +/* Wireless Driver Version */
> +#define DRV_VERSION "1.7.0.108"
> +
> +/* The maximum number of Tx descriptors in all chip families */
> +#define CC33XX_MAX_TX_DESCRIPTORS 32
> +
> +#define CC33XX_CMD_MAX_SIZE (896)
> +#define CC33XX_INI_PARAM_COMMAND_SIZE (16UL)
> +#define CC33XX_INI_CMD_MAX_SIZE (CC33X_CONF_SIZE + CC33XX_INI_PARAM_COMMAND_SIZE + sizeof(int))
> +
> +#define CC33XX_CMD_BUFFER_SIZE ((CC33XX_INI_CMD_MAX_SIZE > CC33XX_CMD_MAX_SIZE)\
> + ? CC33XX_INI_CMD_MAX_SIZE : CC33XX_CMD_MAX_SIZE)
> +
> +#define CC33XX_NUM_MAC_ADDRESSES 3
> +
> +#define CC33XX_AGGR_BUFFER_SIZE (8 * PAGE_SIZE)
> +
> +#define CC33XX_NUM_TX_DESCRIPTORS 32
> +#define CC33XX_NUM_RX_DESCRIPTORS 32
> +
> +#define CC33XX_RX_BA_MAX_SESSIONS 13
> +
> +#define CC33XX_MAX_AP_STATIONS 16
> +
> +struct cc33xx_tx_hw_descr;
> +struct cc33xx_rx_descriptor;
> +struct partial_rx_frame;
> +struct core_fw_status;
> +struct core_status;
> +
> +enum wl_rx_buf_align;
> +
> +struct driver_fw_versions {
> + const char *driver_ver;
> + struct cc33xx_acx_fw_versions *fw_ver;
> +};
> +
> +struct cc33xx_stats {
> + void *fw_stats;
> + unsigned long fw_stats_update;
> + unsigned int retry_count;
> + unsigned int excessive_retries;
> +};
> +
> +struct cc33xx_ant_diversity {
> + u8 diversity_enable;
> + s8 rssi_threshold;
> + u8 default_antenna;
> + u8 padding;
> +};
> +
> +struct cc33xx {
> + bool initialized;
> + struct ieee80211_hw *hw;
> + bool mac80211_registered;
> +
> + struct device *dev;
> + struct platform_device *pdev;
> +
> + struct cc33xx_if_operations *if_ops;
> +
> + int wakeirq;
> +
> + spinlock_t cc_lock; /* Protects critical sections */

Which ones. Your comment is entirely useless, just to satisfy checkpatch.

> +
> + enum cc33xx_state state;
> + bool plt;
> + enum plt_mode plt_mode;
> + u8 plt_role_id;
> + u8 fem_manuf;
> + u8 last_vif_count;
> + struct mutex mutex; /* Protect all CC33xx operations */

?!? So double lock?

> + struct core_status *core_status;
> + u8 last_fw_rls_idx;
> + u8 command_result[CC33XX_CMD_MAX_SIZE];
> + u16 result_length;
> + struct partial_rx_frame partial_rx;
> +
> + unsigned long flags;
> +
> + void *nvs_mac_addr;
> + size_t nvs_mac_addr_len;
> + struct cc33xx_fw_download *fw_download;
> +
> + struct mac_address addresses[CC33XX_NUM_MAC_ADDRESSES];
> +
> + unsigned long links_map[BITS_TO_LONGS(CC33XX_MAX_LINKS)];
> + unsigned long roles_map[BITS_TO_LONGS(CC33XX_MAX_ROLES)];
> + unsigned long roc_map[BITS_TO_LONGS(CC33XX_MAX_ROLES)];
> + unsigned long rate_policies_map[BITS_TO_LONGS(CC33XX_MAX_RATE_POLICIES)];
> +
> + u8 session_ids[CC33XX_MAX_LINKS];
> +
> + struct list_head wlvif_list;
> +
> + u8 sta_count;
> + u8 ap_count;
> +
> + struct cc33xx_acx_mem_map *target_mem_map;
> +
> + /* Accounting for allocated / available TX blocks on HW */
> +
> + u32 tx_blocks_available;
> + u32 tx_allocated_blocks;
> +
> + /* Accounting for allocated / available Tx packets in HW */
> +
> + u32 tx_allocated_pkts[NUM_TX_QUEUES];
> +
> + /* Time-offset between host and chipset clocks */
> +
> + /* Frames scheduled for transmission, not handled yet */
> + int tx_queue_count[NUM_TX_QUEUES];
> + unsigned long queue_stop_reasons[NUM_TX_QUEUES * CC33XX_NUM_MAC_ADDRESSES];
> +
> + /* Frames received, not handled yet by mac80211 */
> + struct sk_buff_head deferred_rx_queue;
> +
> + /* Frames sent, not returned yet to mac80211 */
> + struct sk_buff_head deferred_tx_queue;
> +
> + struct work_struct tx_work;
> + struct workqueue_struct *freezable_wq;
> +
> + /*freezable wq for netstack_work*/
> + struct workqueue_struct *freezable_netstack_wq;
> +
> + /* Pending TX frames */
> + unsigned long tx_frames_map[BITS_TO_LONGS(CC33XX_MAX_TX_DESCRIPTORS)];
> + struct sk_buff *tx_frames[CC33XX_MAX_TX_DESCRIPTORS];
> + int tx_frames_cnt;
> +
> + /* FW Rx counter */
> + u32 rx_counter;
> +
> + /* Intermediate buffer, used for packet aggregation */
> + u8 *aggr_buf;
> + u32 aggr_buf_size;
> + size_t max_transaction_len;
> +
> + /* Reusable dummy packet template */
> + struct sk_buff *dummy_packet;
> +
> + /* Network stack work */
> + struct work_struct netstack_work;
> + /* FW log buffer */
> + u8 *fwlog;
> +
> + /* Number of valid bytes in the FW log buffer */
> + ssize_t fwlog_size;
> +
> + /* Hardware recovery work */
> + struct work_struct recovery_work;
> +
> + struct work_struct irq_deferred_work;
> +
> + /* Reg domain last configuration */
> + DECLARE_BITMAP(reg_ch_conf_last, 64);
> + /* Reg domain pending configuration */
> + DECLARE_BITMAP(reg_ch_conf_pending, 64);
> +
> + /* Lock-less list for deferred event handling */
> + struct llist_head event_list;
> + /* The mbox event mask */
> + u32 event_mask;
> + /* events to unmask only when ap interface is up */
> + u32 ap_event_mask;
> +
> + /* Are we currently scanning */
> + struct cc33xx_vif *scan_wlvif;
> + struct cc33xx_scan scan;
> + struct delayed_work scan_complete_work;
> +
> + struct ieee80211_vif *roc_vif;
> + struct delayed_work roc_complete_work;
> +
> + struct cc33xx_vif *sched_vif;
> +
> + u8 mac80211_scan_stopped;
> +
> + /* The current band */
> + enum nl80211_band band;
> +
> + /* in dBm */
> + int power_level;
> +
> + struct cc33xx_stats stats;
> +
> + __le32 *buffer_32;
> +
> + /* Current chipset configuration */
> + struct cc33xx_conf_file conf;
> +
> + bool enable_11a;
> +
> + /* bands supported by this instance of cc33xx */
> + struct ieee80211_supported_band bands[CC33XX_NUM_BANDS];
> +
> + /* wowlan trigger was configured during suspend.
> + * (currently, only "ANY" and "PATTERN" trigger is supported)
> + */
> +
> + bool keep_device_power;
> +
> + /* AP-mode - links indexed by HLID. The global and broadcast links
> + * are always active.
> + */
> + struct cc33xx_link links[CC33XX_MAX_LINKS];
> +
> + /* number of currently active links */
> + int active_link_count;
> +
> + /* AP-mode - a bitmap of links currently in PS mode according to FW */
> + unsigned long ap_fw_ps_map;
> +
> + /* AP-mode - a bitmap of links currently in PS mode in mac80211 */
> + unsigned long ap_ps_map;
> +
> + /* Quirks of specific hardware revisions */
> + unsigned int quirks;
> +
> + /* number of currently active RX BA sessions */
> + int ba_rx_session_count;
> +
> + /* AP-mode - number of currently connected stations */
> + int active_sta_count;
> +
> + /* last wlvif we transmitted from */
> + struct cc33xx_vif *last_wlvif;
> +
> + /* work to fire when Tx is stuck */
> + struct delayed_work tx_watchdog_work;
> +
> + /* HW HT (11n) capabilities */
> + struct ieee80211_sta_ht_cap ht_cap[CC33XX_NUM_BANDS];
> +
> + /* the current dfs region */
> + enum nl80211_dfs_regions dfs_region;
> + bool radar_debug_mode;
> +
> + /* RX Data filter rule state - enabled/disabled */
> + /* used in CONFIG PM AND W8 Code */
> + unsigned long rx_filter_enabled[BITS_TO_LONGS(CC33XX_MAX_RX_FILTERS)];
> +
> + /* mutex for protecting the tx_flush function */
> + struct mutex flush_mutex;
> +
> + /* sleep auth value currently configured to FW */
> + int sleep_auth;
> +
> + /*ble_enable value - 1=enabled, 0=disabled. */
> + int ble_enable;
> +
> + /* parameters for joining a TWT agreement */
> + int min_wake_duration_usec;
> + int min_wake_interval_mantissa;
> + int min_wake_interval_exponent;
> + int max_wake_interval_mantissa;
> + int max_wake_interval_exponent;
> +
> + /* the number of allocated MAC addresses in this chip */
> + int num_mac_addr;
> +
> + /* sta role index - if 0 - wlan0 primary station interface,
> + * if 1 - wlan2 - secondary station interface
> + */
> + u8 sta_role_idx;
> +
> + u16 max_cmd_size;
> +
> + struct completion nvs_loading_complete;
> + struct completion command_complete;
> +
> + /* dynamic fw traces */
> + u32 dynamic_fw_traces;
> +
> + /* buffer for sending commands to FW */
> + u8 cmd_buf[CC33XX_CMD_BUFFER_SIZE];
> +
> + /* number of keys requiring extra spare mem-blocks */
> + int extra_spare_key_count;

This entire struct is quite unmanageable...

> +
> + u8 efuse_mac_address[ETH_ALEN];
> +
> + u32 fuse_rom_structure_version;
> + u32 device_part_number;
> + u32 pg_version;
> + u8 disable_5g;
> + u8 disable_6g;

Please fix trivial whitespace issues.

This driver looks like having basic issues unresolved, really basic. I
advise to get internal TI review first, before you will be using
community resources for these trivial things.

Please also confirm that you also fixed all warnings from:
1. checkpatch --strict
2. smatch
3. sparse
4. coccinelle/coccicheck

Best regards,
Krzysztof