[PATCH 0/6] wifi: ath10k: use flexible arrays

[PATCH 0/6] wifi: ath10k: use flexible arrays

[Jeff Johnson]

Clean up the last remaining zero-length and one-element arrays in
ath10k to comply with:
https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays

With these cleanups done the ath10k-check script no longer reports any
issues.

---
Jeff Johnson (6):
      wifi: ath10k: use flexible array in struct wmi_host_mem_chunks
      wifi: ath10k: use flexible arrays for WMI start scan TLVs
      wifi: ath10k: remove struct wmi_pdev_chanlist_update_event
      wifi: ath10k: remove unused template structs
      wifi: ath10k: use flexible array in struct wmi_tdls_peer_capabilities
      wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

 drivers/net/wireless/ath/ath10k/wmi.c | 10 +++----
 drivers/net/wireless/ath/ath10k/wmi.h | 55 +++++++++--------------------------
 2 files changed, 17 insertions(+), 48 deletions(-)
---
base-commit: 7133b072dfbfac8763ffb017642c9c894894c50d
change-id: 20231212-wmi_host_mem_chunks_flexarray-78264e146731

[PATCH 1/6] wifi: ath10k: use flexible array in struct wmi_host_mem_chunks

[Jeff Johnson]

Currently struct wmi_host_mem_chunks defines:
	struct host_memory_chunk items[1];

Per the guidance in [1] this should be a flexible array. However there
is a documented requirement:
	some fw revisions require at least 1 chunk regardless of count

To satisfy this requirement, follow the guidance from [2] and wrap the
array in a union which contains both the flexible array and a single
instance of the underlying struct. Since the footprint of the struct
is unchanged, no additional driver changes are required.

No functional changes, compile tested only.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
[2] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/wmi.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 9146df98fcee..833ce0251a2c 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3069,7 +3069,10 @@ struct host_memory_chunk {
 struct wmi_host_mem_chunks {
 	__le32 count;
 	/* some fw revisions require at least 1 chunk regardless of count */
-	struct host_memory_chunk items[1];
+	union {
+		struct host_memory_chunk item;
+		DECLARE_FLEX_ARRAY(struct host_memory_chunk, items);
+	};
 } __packed;
 
 struct wmi_init_cmd {

-- 
2.42.0

Re: [PATCH 1/6] wifi: ath10k: use flexible array in struct wmi_host_mem_chunks

[Kees Cook]

On Wed, Dec 13, 2023 at 09:06:39AM -0800, Jeff Johnson wrote:
> Currently struct wmi_host_mem_chunks defines:
> 	struct host_memory_chunk items[1];
> 
> Per the guidance in [1] this should be a flexible array. However there
> is a documented requirement:
> 	some fw revisions require at least 1 chunk regardless of count
> 
> To satisfy this requirement, follow the guidance from [2] and wrap the
> array in a union which contains both the flexible array and a single
> instance of the underlying struct. Since the footprint of the struct
> is unchanged, no additional driver changes are required.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> [2] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

This looks like the right approach here.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 1/6] wifi: ath10k: use flexible array in struct wmi_host_mem_chunks

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> Currently struct wmi_host_mem_chunks defines:
> 	struct host_memory_chunk items[1];
> 
> Per the guidance in [1] this should be a flexible array. However there
> is a documented requirement:
> 	some fw revisions require at least 1 chunk regardless of count
> 
> To satisfy this requirement, follow the guidance from [2] and wrap the
> array in a union which contains both the flexible array and a single
> instance of the underlying struct. Since the footprint of the struct
> is unchanged, no additional driver changes are required.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> [2] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>   drivers/net/wireless/ath/ath10k/wmi.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 9146df98fcee..833ce0251a2c 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -3069,7 +3069,10 @@ struct host_memory_chunk {
>   struct wmi_host_mem_chunks {
>   	__le32 count;
>   	/* some fw revisions require at least 1 chunk regardless of count */
> -	struct host_memory_chunk items[1];
> +	union {
> +		struct host_memory_chunk item;
> +		DECLARE_FLEX_ARRAY(struct host_memory_chunk, items);
> +	};
>   } __packed;
>   
>   struct wmi_init_cmd {
> 

Re: [PATCH 1/6] wifi: ath10k: use flexible array in struct wmi_host_mem_chunks

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> wrote:

> Currently struct wmi_host_mem_chunks defines:
>         struct host_memory_chunk items[1];
> 
> Per the guidance in [1] this should be a flexible array. However there
> is a documented requirement:
>         some fw revisions require at least 1 chunk regardless of count
> 
> To satisfy this requirement, follow the guidance from [2] and wrap the
> array in a union which contains both the flexible array and a single
> instance of the underlying struct. Since the footprint of the struct
> is unchanged, no additional driver changes are required.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> [2] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

6 patches applied to ath-next branch of ath.git, thanks.

d2eb318f4b6b wifi: ath10k: use flexible array in struct wmi_host_mem_chunks
72ca7c4073ac wifi: ath10k: use flexible arrays for WMI start scan TLVs
26eb704a46f8 wifi: ath10k: remove struct wmi_pdev_chanlist_update_event
b0c0794b05ec wifi: ath10k: remove unused template structs
cb188e862c1c wifi: ath10k: use flexible array in struct wmi_tdls_peer_capabilities
6b9923f1f6d1 wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20231213-wmi_host_mem_chunks_flexarray-v1-1-92922d92fa2c@quicinc.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/6] wifi: ath10k: use flexible array in struct wmi_host_mem_chunks

[Gustavo A. R. Silva]

> 6 patches applied to ath-next branch of ath.git, thanks.

Awesome! :)

Thanks
--
Gustavo

[PATCH 2/6] wifi: ath10k: use flexible arrays for WMI start scan TLVs

[Jeff Johnson]

Currently ath10k defines the following struct:
	struct wmi_start_scan_tlvs {
		u8 tlvs[0];
	} __packed;

Per the guidance in [1] this should be a flexible array. However, a
direct replace to u8 tlvs[] results in the compilation error:
       flexible array member in a struct with no named members

This is because C99 6.7.2.1 (16) requires that a structure containing
a flexible array member must have more than one named member.

So rather than defining a separate struct wmi_start_scan_tlvs which
contains the flexible tlvs[] array, just define the tlvs[] array where
struct wmi_start_scan_tlvs is being used.

No functional changes, compile tested only.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c |  8 ++++----
 drivers/net/wireless/ath/ath10k/wmi.h | 11 ++---------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 88befe92f95d..4d5aadbc7159 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -6927,14 +6927,14 @@ void ath10k_wmi_put_start_scan_common(struct wmi_start_scan_common *cmn,
 }
 
 static void
-ath10k_wmi_put_start_scan_tlvs(struct wmi_start_scan_tlvs *tlvs,
+ath10k_wmi_put_start_scan_tlvs(u8 *tlvs,
 			       const struct wmi_start_scan_arg *arg)
 {
 	struct wmi_ie_data *ie;
 	struct wmi_chan_list *channels;
 	struct wmi_ssid_list *ssids;
 	struct wmi_bssid_list *bssids;
-	void *ptr = tlvs->tlvs;
+	void *ptr = tlvs;
 	int i;
 
 	if (arg->n_channels) {
@@ -7012,7 +7012,7 @@ ath10k_wmi_op_gen_start_scan(struct ath10k *ar,
 	cmd = (struct wmi_start_scan_cmd *)skb->data;
 
 	ath10k_wmi_put_start_scan_common(&cmd->common, arg);
-	ath10k_wmi_put_start_scan_tlvs(&cmd->tlvs, arg);
+	ath10k_wmi_put_start_scan_tlvs(cmd->tlvs, arg);
 
 	cmd->burst_duration_ms = __cpu_to_le32(0);
 
@@ -7041,7 +7041,7 @@ ath10k_wmi_10x_op_gen_start_scan(struct ath10k *ar,
 	cmd = (struct wmi_10x_start_scan_cmd *)skb->data;
 
 	ath10k_wmi_put_start_scan_common(&cmd->common, arg);
-	ath10k_wmi_put_start_scan_tlvs(&cmd->tlvs, arg);
+	ath10k_wmi_put_start_scan_tlvs(cmd->tlvs, arg);
 
 	ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi 10x start scan\n");
 	return skb;
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 833ce0251a2c..52a409ff94e7 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -3218,23 +3218,16 @@ struct wmi_start_scan_common {
 	__le32 scan_ctrl_flags;
 } __packed;
 
-struct wmi_start_scan_tlvs {
-	/* TLV parameters. These includes channel list, ssid list, bssid list,
-	 * extra ies.
-	 */
-	u8 tlvs[0];
-} __packed;
-
 struct wmi_start_scan_cmd {
 	struct wmi_start_scan_common common;
 	__le32 burst_duration_ms;
-	struct wmi_start_scan_tlvs tlvs;
+	u8 tlvs[];
 } __packed;
 
 /* This is the definition from 10.X firmware branch */
 struct wmi_10x_start_scan_cmd {
 	struct wmi_start_scan_common common;
-	struct wmi_start_scan_tlvs tlvs;
+	u8 tlvs[];
 } __packed;
 
 struct wmi_ssid_arg {

-- 
2.42.0

Re: [PATCH 2/6] wifi: ath10k: use flexible arrays for WMI start scan TLVs

[Kees Cook]

On Wed, Dec 13, 2023 at 09:06:40AM -0800, Jeff Johnson wrote:
> Currently ath10k defines the following struct:
> 	struct wmi_start_scan_tlvs {
> 		u8 tlvs[0];
> 	} __packed;
> 
> Per the guidance in [1] this should be a flexible array. However, a
> direct replace to u8 tlvs[] results in the compilation error:
>        flexible array member in a struct with no named members
> 
> This is because C99 6.7.2.1 (16) requires that a structure containing
> a flexible array member must have more than one named member.
> 
> So rather than defining a separate struct wmi_start_scan_tlvs which
> contains the flexible tlvs[] array, just define the tlvs[] array where
> struct wmi_start_scan_tlvs is being used.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Yeah, this too looks like the right approach: keeping the flex array
close instead of externalized into a no-op struct.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 2/6] wifi: ath10k: use flexible arrays for WMI start scan TLVs

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> Currently ath10k defines the following struct:
> 	struct wmi_start_scan_tlvs {
> 		u8 tlvs[0];
> 	} __packed;
> 
> Per the guidance in [1] this should be a flexible array. However, a
> direct replace to u8 tlvs[] results in the compilation error:
>         flexible array member in a struct with no named members
> 
> This is because C99 6.7.2.1 (16) requires that a structure containing
> a flexible array member must have more than one named member.
> 
> So rather than defining a separate struct wmi_start_scan_tlvs which
> contains the flexible tlvs[] array, just define the tlvs[] array where
> struct wmi_start_scan_tlvs is being used.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>   drivers/net/wireless/ath/ath10k/wmi.c |  8 ++++----
>   drivers/net/wireless/ath/ath10k/wmi.h | 11 ++---------
>   2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 88befe92f95d..4d5aadbc7159 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -6927,14 +6927,14 @@ void ath10k_wmi_put_start_scan_common(struct wmi_start_scan_common *cmn,
>   }
>   
>   static void
> -ath10k_wmi_put_start_scan_tlvs(struct wmi_start_scan_tlvs *tlvs,
> +ath10k_wmi_put_start_scan_tlvs(u8 *tlvs,
>   			       const struct wmi_start_scan_arg *arg)
>   {
>   	struct wmi_ie_data *ie;
>   	struct wmi_chan_list *channels;
>   	struct wmi_ssid_list *ssids;
>   	struct wmi_bssid_list *bssids;
> -	void *ptr = tlvs->tlvs;
> +	void *ptr = tlvs;
>   	int i;
>   
>   	if (arg->n_channels) {
> @@ -7012,7 +7012,7 @@ ath10k_wmi_op_gen_start_scan(struct ath10k *ar,
>   	cmd = (struct wmi_start_scan_cmd *)skb->data;
>   
>   	ath10k_wmi_put_start_scan_common(&cmd->common, arg);
> -	ath10k_wmi_put_start_scan_tlvs(&cmd->tlvs, arg);
> +	ath10k_wmi_put_start_scan_tlvs(cmd->tlvs, arg);
>   
>   	cmd->burst_duration_ms = __cpu_to_le32(0);
>   
> @@ -7041,7 +7041,7 @@ ath10k_wmi_10x_op_gen_start_scan(struct ath10k *ar,
>   	cmd = (struct wmi_10x_start_scan_cmd *)skb->data;
>   
>   	ath10k_wmi_put_start_scan_common(&cmd->common, arg);
> -	ath10k_wmi_put_start_scan_tlvs(&cmd->tlvs, arg);
> +	ath10k_wmi_put_start_scan_tlvs(cmd->tlvs, arg);
>   
>   	ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi 10x start scan\n");
>   	return skb;
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 833ce0251a2c..52a409ff94e7 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -3218,23 +3218,16 @@ struct wmi_start_scan_common {
>   	__le32 scan_ctrl_flags;
>   } __packed;
>   
> -struct wmi_start_scan_tlvs {
> -	/* TLV parameters. These includes channel list, ssid list, bssid list,
> -	 * extra ies.
> -	 */
> -	u8 tlvs[0];
> -} __packed;
> -
>   struct wmi_start_scan_cmd {
>   	struct wmi_start_scan_common common;
>   	__le32 burst_duration_ms;
> -	struct wmi_start_scan_tlvs tlvs;
> +	u8 tlvs[];
>   } __packed;
>   
>   /* This is the definition from 10.X firmware branch */
>   struct wmi_10x_start_scan_cmd {
>   	struct wmi_start_scan_common common;
> -	struct wmi_start_scan_tlvs tlvs;
> +	u8 tlvs[];
>   } __packed;
>   
>   struct wmi_ssid_arg {
> 

[PATCH 3/6] wifi: ath10k: remove struct wmi_pdev_chanlist_update_event

[Jeff Johnson]

Currently struct wmi_pdev_chanlist_update_event defines:
	  struct wmi_channel channel_list[1];

Per the guidance in [1] this should be a flexible array. However
during conversion it was discovered that this struct is not used, so
just remove the entire struct.

No functional changes, compile tested only.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/wmi.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 52a409ff94e7..37a7d421bd86 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -4256,13 +4256,6 @@ struct wmi_peer_sta_ps_state_chg_event {
 	__le32 peer_ps_state;
 } __packed;
 
-struct wmi_pdev_chanlist_update_event {
-	/* number of channels */
-	__le32 num_chan;
-	/* array of channels */
-	struct wmi_channel channel_list[1];
-} __packed;
-
 #define WMI_MAX_DEBUG_MESG (sizeof(u32) * 32)
 
 struct wmi_debug_mesg_event {

-- 
2.42.0

Re: [PATCH 3/6] wifi: ath10k: remove struct wmi_pdev_chanlist_update_event

[Kees Cook]

On Wed, Dec 13, 2023 at 09:06:41AM -0800, Jeff Johnson wrote:
> Currently struct wmi_pdev_chanlist_update_event defines:
> 	  struct wmi_channel channel_list[1];
> 
> Per the guidance in [1] this should be a flexible array. However
> during conversion it was discovered that this struct is not used, so
> just remove the entire struct.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Removal of unused structs is good. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 3/6] wifi: ath10k: remove struct wmi_pdev_chanlist_update_event

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> Currently struct wmi_pdev_chanlist_update_event defines:
> 	  struct wmi_channel channel_list[1];
> 
> Per the guidance in [1] this should be a flexible array. However
> during conversion it was discovered that this struct is not used, so
> just remove the entire struct.

Less code is always great. :)

> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>   drivers/net/wireless/ath/ath10k/wmi.h | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 52a409ff94e7..37a7d421bd86 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -4256,13 +4256,6 @@ struct wmi_peer_sta_ps_state_chg_event {
>   	__le32 peer_ps_state;
>   } __packed;
>   
> -struct wmi_pdev_chanlist_update_event {
> -	/* number of channels */
> -	__le32 num_chan;
> -	/* array of channels */
> -	struct wmi_channel channel_list[1];
> -} __packed;
> -
>   #define WMI_MAX_DEBUG_MESG (sizeof(u32) * 32)
>   
>   struct wmi_debug_mesg_event {
> 

[PATCH 4/6] wifi: ath10k: remove unused template structs

[Jeff Johnson]

Currently both the wmi_bcn_tmpl_cmd and wmi_prb_tmpl_cmd structs
define:
	  u8 data[1];

Per the guidance in [1] both instances of this should be flexible
arrays. However during conversion it was discovered that neither of
these structs are actually used, so just remove them.

No functional changes, compile tested only.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/wmi.h | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 37a7d421bd86..e16410e348ca 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -5782,30 +5782,6 @@ struct wmi_bcn_prb_info {
 	/* app IE */
 } __packed;
 
-struct wmi_bcn_tmpl_cmd {
-	/* unique id identifying the VDEV, generated by the caller */
-	__le32 vdev_id;
-	/* TIM IE offset from the beginning of the template. */
-	__le32 tim_ie_offset;
-	/* beacon probe capabilities and IEs */
-	struct wmi_bcn_prb_info bcn_prb_info;
-	/* beacon buffer length */
-	__le32 buf_len;
-	/* variable length data */
-	u8 data[1];
-} __packed;
-
-struct wmi_prb_tmpl_cmd {
-	/* unique id identifying the VDEV, generated by the caller */
-	__le32 vdev_id;
-	/* beacon probe capabilities and IEs */
-	struct wmi_bcn_prb_info bcn_prb_info;
-	/* beacon buffer length */
-	__le32 buf_len;
-	/* Variable length data */
-	u8 data[1];
-} __packed;
-
 enum wmi_sta_ps_mode {
 	/* enable power save for the given STA VDEV */
 	WMI_STA_PS_MODE_DISABLED = 0,

-- 
2.42.0

Re: [PATCH 4/6] wifi: ath10k: remove unused template structs

[Kees Cook]

On Wed, Dec 13, 2023 at 09:06:42AM -0800, Jeff Johnson wrote:
> Currently both the wmi_bcn_tmpl_cmd and wmi_prb_tmpl_cmd structs
> define:
> 	  u8 data[1];
> 
> Per the guidance in [1] both instances of this should be flexible
> arrays. However during conversion it was discovered that neither of
> these structs are actually used, so just remove them.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Unused! :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 4/6] wifi: ath10k: remove unused template structs

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> Currently both the wmi_bcn_tmpl_cmd and wmi_prb_tmpl_cmd structs
> define:
> 	  u8 data[1];
> 
> Per the guidance in [1] both instances of this should be flexible
> arrays. However during conversion it was discovered that neither of
> these structs are actually used, so just remove them.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>   drivers/net/wireless/ath/ath10k/wmi.h | 24 ------------------------
>   1 file changed, 24 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 37a7d421bd86..e16410e348ca 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -5782,30 +5782,6 @@ struct wmi_bcn_prb_info {
>   	/* app IE */
>   } __packed;
>   
> -struct wmi_bcn_tmpl_cmd {
> -	/* unique id identifying the VDEV, generated by the caller */
> -	__le32 vdev_id;
> -	/* TIM IE offset from the beginning of the template. */
> -	__le32 tim_ie_offset;
> -	/* beacon probe capabilities and IEs */
> -	struct wmi_bcn_prb_info bcn_prb_info;
> -	/* beacon buffer length */
> -	__le32 buf_len;
> -	/* variable length data */
> -	u8 data[1];
> -} __packed;
> -
> -struct wmi_prb_tmpl_cmd {
> -	/* unique id identifying the VDEV, generated by the caller */
> -	__le32 vdev_id;
> -	/* beacon probe capabilities and IEs */
> -	struct wmi_bcn_prb_info bcn_prb_info;
> -	/* beacon buffer length */
> -	__le32 buf_len;
> -	/* Variable length data */
> -	u8 data[1];
> -} __packed;
> -
>   enum wmi_sta_ps_mode {
>   	/* enable power save for the given STA VDEV */
>   	WMI_STA_PS_MODE_DISABLED = 0,
> 

[PATCH 5/6] wifi: ath10k: use flexible array in struct wmi_tdls_peer_capabilities

[Jeff Johnson]

Currently struct wmi_tdls_peer_capabilities defines:
	struct wmi_channel peer_chan_list[1];

Per the guidance in [1] this should be a flexible array, and at one
point Gustavo was trying to fix this [2], but had questions about the
correct behavior when the associated peer_chan_len is 0.

I have been unable to determine if firmware requires that at least one
record be present even if peer_chan_len is 0. But since that is the
current behavior, follow the example from [3] and replace the
one-element array with a union that contains both a flexible array and
a single instance of the array element. This results in a struct that
has the same footprint as the original, so no other driver changes are
required.

No functional changes, compile tested only.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
[2] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/
[3] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/wmi.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index e16410e348ca..b64b6e214bae 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -7162,7 +7162,13 @@ struct wmi_tdls_peer_capabilities {
 	__le32 is_peer_responder;
 	__le32 pref_offchan_num;
 	__le32 pref_offchan_bw;
-	struct wmi_channel peer_chan_list[1];
+	union {
+		/* to match legacy implementation allocate room for
+		 * at least one record even if peer_chan_len is 0
+		 */
+		struct wmi_channel peer_chan_min_allocation;
+		DECLARE_FLEX_ARRAY(struct wmi_channel, peer_chan_list);
+	};
 } __packed;
 
 struct wmi_10_4_tdls_peer_update_cmd {

-- 
2.42.0

Re: [PATCH 5/6] wifi: ath10k: use flexible array in struct wmi_tdls_peer_capabilities

[Kees Cook]

On Wed, Dec 13, 2023 at 09:06:43AM -0800, Jeff Johnson wrote:
> Currently struct wmi_tdls_peer_capabilities defines:
> 	struct wmi_channel peer_chan_list[1];
> 
> Per the guidance in [1] this should be a flexible array, and at one
> point Gustavo was trying to fix this [2], but had questions about the
> correct behavior when the associated peer_chan_len is 0.
> 
> I have been unable to determine if firmware requires that at least one
> record be present even if peer_chan_len is 0. But since that is the
> current behavior, follow the example from [3] and replace the
> one-element array with a union that contains both a flexible array and
> a single instance of the array element. This results in a struct that
> has the same footprint as the original, so no other driver changes are
> required.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> [2] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/
> [3] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Again, good to keep the struct the same size.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 5/6] wifi: ath10k: use flexible array in struct wmi_tdls_peer_capabilities

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> Currently struct wmi_tdls_peer_capabilities defines:
> 	struct wmi_channel peer_chan_list[1];
> 
> Per the guidance in [1] this should be a flexible array, and at one
> point Gustavo was trying to fix this [2], but had questions about the
> correct behavior when the associated peer_chan_len is 0.
> 
> I have been unable to determine if firmware requires that at least one
> record be present even if peer_chan_len is 0. But since that is the
> current behavior, follow the example from [3] and replace the
> one-element array with a union that contains both a flexible array and
> a single instance of the array element. This results in a struct that
> has the same footprint as the original, so no other driver changes are
> required.
> 
> No functional changes, compile tested only.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> [2] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/
> [3] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>   drivers/net/wireless/ath/ath10k/wmi.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index e16410e348ca..b64b6e214bae 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -7162,7 +7162,13 @@ struct wmi_tdls_peer_capabilities {
>   	__le32 is_peer_responder;
>   	__le32 pref_offchan_num;
>   	__le32 pref_offchan_bw;
> -	struct wmi_channel peer_chan_list[1];
> +	union {
> +		/* to match legacy implementation allocate room for
> +		 * at least one record even if peer_chan_len is 0
> +		 */
> +		struct wmi_channel peer_chan_min_allocation;
> +		DECLARE_FLEX_ARRAY(struct wmi_channel, peer_chan_list);
> +	};
>   } __packed;
>   
>   struct wmi_10_4_tdls_peer_update_cmd {
> 

[PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

[Jeff Johnson]

In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.

No functional changes, compile tested only.

[1] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 4d5aadbc7159..0cfd9484c45e 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
 	if (!skb)
 		return ERR_PTR(-ENOMEM);
 
-	memset(skb->data, 0, sizeof(*cmd));
-
 	cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data;
 	cmd->vdev_id = __cpu_to_le32(arg->vdev_id);
 	ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);

-- 
2.42.0

Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

[Kees Cook]

On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote:
> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.

Is .gen_tdls_peer_update only ever called after a fresh allocation? It
wasn't obvious to me as I tried to follow the call paths. Is there harm
in leaving this?

-Kees

> 
> No functional changes, compile tested only.
> 
> [1] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath10k/wmi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 4d5aadbc7159..0cfd9484c45e 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
>  	if (!skb)
>  		return ERR_PTR(-ENOMEM);
>  
> -	memset(skb->data, 0, sizeof(*cmd));
> -
>  	cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data;
>  	cmd->vdev_id = __cpu_to_le32(arg->vdev_id);
>  	ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);
> 
> -- 
> 2.42.0
> 

-- 
Kees Cook

Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

[Jeff Johnson]

On 12/13/2023 11:16 AM, Kees Cook wrote:
> On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote:
>> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
>> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
>> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.
> 
> Is .gen_tdls_peer_update only ever called after a fresh allocation? It
> wasn't obvious to me as I tried to follow the call paths. Is there harm
> in leaving this?

The only harm is a slight increase in code size and cpu cycles.

However note the skb allocation is done within
ath10k_wmi_10_4_gen_tdls_peer_update() itself, just before the code
being removed:
	skb = ath10k_wmi_alloc_skb(ar, len);
	if (!skb)
		return ERR_PTR(-ENOMEM);

And in ath10k_wmi_alloc_skb() we have:
	memset(skb->data, 0, round_len);

So the memset() being removed is always redundant.

/jeff

Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

[Kees Cook]

On Wed, Dec 13, 2023 at 11:36:08AM -0800, Jeff Johnson wrote:
> On 12/13/2023 11:16 AM, Kees Cook wrote:
> > On Wed, Dec 13, 2023 at 09:06:44AM -0800, Jeff Johnson wrote:
> >> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
> >> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
> >> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.
> > 
> > Is .gen_tdls_peer_update only ever called after a fresh allocation? It
> > wasn't obvious to me as I tried to follow the call paths. Is there harm
> > in leaving this?
> 
> The only harm is a slight increase in code size and cpu cycles.
> 
> However note the skb allocation is done within
> ath10k_wmi_10_4_gen_tdls_peer_update() itself, just before the code
> being removed:
> 	skb = ath10k_wmi_alloc_skb(ar, len);
> 	if (!skb)
> 		return ERR_PTR(-ENOMEM);
> 
> And in ath10k_wmi_alloc_skb() we have:
> 	memset(skb->data, 0, round_len);
> 
> So the memset() being removed is always redundant.

LOL. I see now. I missed that was was looking outside the function! :P

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 6/6] wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> In [1] it was identified that in ath10k_wmi_10_4_gen_tdls_peer_update()
> the memset(skb->data, 0, sizeof(*cmd)) is unnecessary since function
> ath10k_wmi_alloc_skb() already zeroes skb->data, so remove it.
> 
> No functional changes, compile tested only.
> 
> [1] https://lore.kernel.org/linux-wireless/626ae2e7-66f8-423b-b17f-e75c1a6d29b3@embeddedor.com/
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks!
--
Gustavo

> ---
>   drivers/net/wireless/ath/ath10k/wmi.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 4d5aadbc7159..0cfd9484c45e 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -8918,8 +8918,6 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
>   	if (!skb)
>   		return ERR_PTR(-ENOMEM);
>   
> -	memset(skb->data, 0, sizeof(*cmd));
> -
>   	cmd = (struct wmi_10_4_tdls_peer_update_cmd *)skb->data;
>   	cmd->vdev_id = __cpu_to_le32(arg->vdev_id);
>   	ether_addr_copy(cmd->peer_macaddr.addr, arg->addr);
> 

Re: [PATCH 0/6] wifi: ath10k: use flexible arrays

[Gustavo A. R. Silva]

On 12/13/23 11:06, Jeff Johnson wrote:
> Clean up the last remaining zero-length and one-element arrays in
> ath10k to comply with:
> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> 
> With these cleanups done the ath10k-check script no longer reports any
> issues.

This is really great!

Thank you for taking care of this. :)
--
Gustavo

> 
> ---
> Jeff Johnson (6):
>        wifi: ath10k: use flexible array in struct wmi_host_mem_chunks
>        wifi: ath10k: use flexible arrays for WMI start scan TLVs
>        wifi: ath10k: remove struct wmi_pdev_chanlist_update_event
>        wifi: ath10k: remove unused template structs
>        wifi: ath10k: use flexible array in struct wmi_tdls_peer_capabilities
>        wifi: ath10k: remove duplicate memset() in 10.4 TDLS peer update
> 
>   drivers/net/wireless/ath/ath10k/wmi.c | 10 +++----
>   drivers/net/wireless/ath/ath10k/wmi.h | 55 +++++++++--------------------------
>   2 files changed, 17 insertions(+), 48 deletions(-)
> ---
> base-commit: 7133b072dfbfac8763ffb017642c9c894894c50d
> change-id: 20231212-wmi_host_mem_chunks_flexarray-78264e146731
>