diff mbox series

ath9k: initialize arrays at compile time

Message ID 20220320152028.2263518-1-trix@redhat.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ath9k: initialize arrays at compile time | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Tom Rix March 20, 2022, 3:20 p.m. UTC 
From: Tom Rix <trix@redhat.com>

Early clearing of arrays with
memset(array, 0, size);
is equivilent to initializing the array in its decl with
array[size] = { 0 };

Since compile time is preferred over runtime,
convert the memsets to initializations.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_calib.c  |  6 ++----
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c |  4 +---
 drivers/net/wireless/ath/ath9k/ar9003_paprd.c  | 14 ++++++--------
 drivers/net/wireless/ath/ath9k/eeprom.c        |  3 +--
 drivers/net/wireless/ath/ath9k/eeprom_4k.c     |  4 +---
 drivers/net/wireless/ath/ath9k/eeprom_9287.c   |  4 +---
 drivers/net/wireless/ath/ath9k/eeprom_def.c    |  4 +---
 drivers/net/wireless/ath/ath9k/wow.c           |  7 ++-----
 8 files changed, 15 insertions(+), 31 deletions(-)

Comments

Joe Perches March 20, 2022, 4:16 p.m. UTC | #1 
On Sun, 2022-03-20 at 08:20 -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> Early clearing of arrays with
> memset(array, 0, size);
> is equivilent to initializing the array in its decl with
> array[size] = { 0 };

This is true. (typo of equivalent btw)

> Since compile time is preferred over runtime,
> convert the memsets to initializations.

But this is not.

These aren't static but are stack declarations so these can not
be "initialized at compile time".

Both are zeroed at runtime, perhaps with different instructions.
Sometimes with smaller code, sometimes larger.
Sometimes faster, sometimes not.

Anyway, I think the patch is good, but the commit description is not.

> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_calib.c b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
[]
> @@ -891,10 +891,9 @@ static void ar9003_hw_tx_iq_cal_outlier_detection(struct ath_hw *ah,
>  {
>  	int i, im, nmeasurement;
>  	int magnitude, phase;
> -	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS];
> +	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS] = { 0 };
>  	struct ath9k_hw_cal_data *caldata = ah->caldata;
>  
> -	memset(tx_corr_coeff, 0, sizeof(tx_corr_coeff));

etc...
John Crispin March 20, 2022, 4:48 p.m. UTC | #2 
On 20.03.22 16:20, trix@redhat.com wrote:
> array[size] = { 0 };

should this not be array[size] = { }; ?!

If I recall correctly { 0 } will only set the first element of the 
struct/array to 0 and leave random data in all others elements

	John
Sebastian Gottschall March 20, 2022, 5:17 p.m. UTC | #3 
Am 20.03.2022 um 17:48 schrieb John Crispin:
>
>
> On 20.03.22 16:20, trix@redhat.com wrote:
>> array[size] = { 0 };
>
> should this not be array[size] = { }; ?!
>
> If I recall correctly { 0 } will only set the first element of the 
> struct/array to 0 and leave random data in all others elements
>
>     John

You are right, john

Sebastian
Andreas Schwab March 20, 2022, 5:32 p.m. UTC | #4 
On Mär 20 2022, John Crispin wrote:

> If I recall correctly { 0 } will only set the first element of the
> struct/array to 0 and leave random data in all others elements

An initializer always initializes the _whole_ object.

The subject is also wrong, all initializers are executed at run time
(automatic variables cannot be initialized at compile time).
Joe Perches March 20, 2022, 5:36 p.m. UTC | #5 
On Sun, 2022-03-20 at 18:17 +0100, Sebastian Gottschall wrote:
> Am 20.03.2022 um 17:48 schrieb John Crispin:
> > 
> > 
> > On 20.03.22 16:20, trix@redhat.com wrote:
> > > array[size] = { 0 };
> > 
> > should this not be array[size] = { }; ?!
> > 
> > If I recall correctly { 0 } will only set the first element of the 
> > struct/array to 0 and leave random data in all others elements
> > 
> >     John
> 
> You are right, john

No.  The patch is fine.

Though generally the newer code in the kernel uses

	type dec[size] = {};

to initialize stack arrays.

array stack declarations not using 0

$ git grep -P '^\t(?:\w++\s*){1,2}\[\s*\w+\s*\]\s*=\s*\{\s*\};' -- '*.c' | wc -l
213

array stack declarations using 0

$ git grep -P '^\t(?:\w++\s*){1,2}\[\s*\w+\s*\]\s*=\s*\{\s*0\s*\};' -- '*.c' | wc -l
776

Refer to the c standard section on initialization 6.7.8 subsections 19 and 21

19

The initialization shall occur in initializer list order, each initializer provided for a
particular subobject overriding any previously listed initializer for the same subobject
all subobjects that are not initialized explicitly shall be initialized implicitly the same as
objects that have static storage duration.

...

21

If there are fewer initializers in a brace-enclosed list than there are elements or members
of an aggregate, or fewer characters in a string literal used to initialize an array of known
size than there are elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage duration.
David Laight March 20, 2022, 6:46 p.m. UTC | #6 
From: trix@redhat.com <trix@redhat.com>
> Sent: 20 March 2022 15:20
> 
> Early clearing of arrays with
> memset(array, 0, size);
> is equivilent to initializing the array in its decl with
> array[size] = { 0 };
> 
> Since compile time is preferred over runtime,
> convert the memsets to initializations.
...
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> index dc24da1ff00b1..39fcc158cb159 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> @@ -891,10 +891,9 @@ static void ar9003_hw_tx_iq_cal_outlier_detection(struct ath_hw *ah,
>  {
>  	int i, im, nmeasurement;
>  	int magnitude, phase;
> -	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS];
> +	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS] = { 0 };

For a two dimensional array that needs to be {{0}} (or {}).
And, since there is only one definitions of 'coeff' it can
be static!
(Currently on 96 bytes - si not a real problem on-stack.)

Although I just failed to find the lock that stops
concurrent execution on multiple cpu.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Toke Høiland-Jørgensen March 21, 2022, 1:31 p.m. UTC | #7 
trix@redhat.com writes:

> From: Tom Rix <trix@redhat.com>
>
> Early clearing of arrays with
> memset(array, 0, size);
> is equivilent to initializing the array in its decl with
> array[size] = { 0 };
>
> Since compile time is preferred over runtime,
> convert the memsets to initializations.
>
> Signed-off-by: Tom Rix <trix@redhat.com>

Cf the discussion in the replies, please resubmit with empty
initialisers ({}), and fix up the commit message.

-Toke
Tom Rix March 23, 2022, 1:13 p.m. UTC | #8 
On 3/20/22 10:36 AM, Joe Perches wrote:
> On Sun, 2022-03-20 at 18:17 +0100, Sebastian Gottschall wrote:
>> Am 20.03.2022 um 17:48 schrieb John Crispin:
>>>
>>> On 20.03.22 16:20, trix@redhat.com wrote:
>>>> array[size] = { 0 };
>>> should this not be array[size] = { }; ?!
>>>
>>> If I recall correctly { 0 } will only set the first element of the
>>> struct/array to 0 and leave random data in all others elements
>>>
>>>      John
>> You are right, john
> No.  The patch is fine.
>
> Though generally the newer code in the kernel uses
>
> 	type dec[size] = {};
>
> to initialize stack arrays.
>
> array stack declarations not using 0
>
> $ git grep -P '^\t(?:\w++\s*){1,2}\[\s*\w+\s*\]\s*=\s*\{\s*\};' -- '*.c' | wc -l
> 213
>
> array stack declarations using 0
>
> $ git grep -P '^\t(?:\w++\s*){1,2}\[\s*\w+\s*\]\s*=\s*\{\s*0\s*\};' -- '*.c' | wc -l
> 776
>
> Refer to the c standard section on initialization 6.7.8 subsections 19 and 21
>
> 19
>
> The initialization shall occur in initializer list order, each initializer provided for a
> particular subobject overriding any previously listed initializer for the same subobject
> all subobjects that are not initialized explicitly shall be initialized implicitly the same as
> objects that have static storage duration.
>
> ...
>
> 21
>
> If there are fewer initializers in a brace-enclosed list than there are elements or members
> of an aggregate, or fewer characters in a string literal used to initialize an array of known
> size than there are elements in the array, the remainder of the aggregate shall be
> initialized implicitly the same as objects that have static storage duration.
>
Joe,

Thanks for providing these sections for c reference !

I will update the commit log and replace { 0 } with { }

Tom
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_calib.c b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
index dc24da1ff00b1..39fcc158cb159 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
@@ -891,10 +891,9 @@  static void ar9003_hw_tx_iq_cal_outlier_detection(struct ath_hw *ah,
 {
 	int i, im, nmeasurement;
 	int magnitude, phase;
-	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS];
+	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS] = { 0 };
 	struct ath9k_hw_cal_data *caldata = ah->caldata;
 
-	memset(tx_corr_coeff, 0, sizeof(tx_corr_coeff));
 	for (i = 0; i < MAX_MEASUREMENT / 2; i++) {
 		tx_corr_coeff[i * 2][0] = tx_corr_coeff[(i * 2) + 1][0] =
 					AR_PHY_TX_IQCAL_CORR_COEFF_B0(i);
@@ -1155,10 +1154,9 @@  static void ar9003_hw_tx_iq_cal_post_proc(struct ath_hw *ah,
 static void ar9003_hw_tx_iq_cal_reload(struct ath_hw *ah)
 {
 	struct ath9k_hw_cal_data *caldata = ah->caldata;
-	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS];
+	u32 tx_corr_coeff[MAX_MEASUREMENT][AR9300_MAX_CHAINS] = { 0 };
 	int i, im;
 
-	memset(tx_corr_coeff, 0, sizeof(tx_corr_coeff));
 	for (i = 0; i < MAX_MEASUREMENT / 2; i++) {
 		tx_corr_coeff[i * 2][0] = tx_corr_coeff[(i * 2) + 1][0] =
 					AR_PHY_TX_IQCAL_CORR_COEFF_B0(i);
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index b0a4ca3559fd8..55fdee5ec93be 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -5451,14 +5451,12 @@  static void ath9k_hw_ar9300_set_txpower(struct ath_hw *ah,
 	struct ath_common *common = ath9k_hw_common(ah);
 	struct ar9300_eeprom *eep = &ah->eeprom.ar9300_eep;
 	struct ar9300_modal_eep_header *modal_hdr;
-	u8 targetPowerValT2[ar9300RateSize];
+	u8 targetPowerValT2[ar9300RateSize] = { 0 };
 	u8 target_power_val_t2_eep[ar9300RateSize];
 	u8 targetPowerValT2_tpc[ar9300RateSize];
 	unsigned int i = 0, paprd_scale_factor = 0;
 	u8 pwr_idx, min_pwridx = 0;
 
-	memset(targetPowerValT2, 0 , sizeof(targetPowerValT2));
-
 	/*
 	 * Get target powers from EEPROM - our baseline for TX Power
 	 */
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_paprd.c b/drivers/net/wireless/ath/ath9k/ar9003_paprd.c
index 34e1009402846..d9c5b6bb5db07 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_paprd.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_paprd.c
@@ -419,13 +419,16 @@  static inline int find_proper_scale(int expn, int N)
 static bool create_pa_curve(u32 *data_L, u32 *data_U, u32 *pa_table, u16 *gain)
 {
 	unsigned int thresh_accum_cnt;
-	int x_est[NUM_BIN + 1], Y[NUM_BIN + 1], theta[NUM_BIN + 1];
+	int x_est[NUM_BIN + 1] = { 0 };
+	int Y[NUM_BIN + 1] = { 0 };
+	int theta[NUM_BIN + 1] = { 0 };
 	int PA_in[NUM_BIN + 1];
 	int B1_tmp[NUM_BIN + 1], B2_tmp[NUM_BIN + 1];
 	unsigned int B1_abs_max, B2_abs_max;
 	int max_index, scale_factor;
-	int y_est[NUM_BIN + 1];
-	int x_est_fxp1_nonlin, x_tilde[NUM_BIN + 1];
+	int y_est[NUM_BIN + 1] = { 0 };
+	int x_est_fxp1_nonlin;
+	int x_tilde[NUM_BIN + 1] = { 0 };
 	unsigned int x_tilde_abs;
 	int G_fxp, Y_intercept, order_x_by_y, M, I, L, sum_y_sqr, sum_y_quad;
 	int Q_x, Q_B1, Q_B2, beta_raw, alpha_raw, scale_B;
@@ -439,11 +442,6 @@  static bool create_pa_curve(u32 *data_L, u32 *data_U, u32 *pa_table, u16 *gain)
 	thresh_accum_cnt = 16;
 	scale_factor = 5;
 	max_index = 0;
-	memset(theta, 0, sizeof(theta));
-	memset(x_est, 0, sizeof(x_est));
-	memset(Y, 0, sizeof(Y));
-	memset(y_est, 0, sizeof(y_est));
-	memset(x_tilde, 0, sizeof(x_tilde));
 
 	for (i = 0; i < NUM_BIN; i++) {
 		s32 accum_cnt, accum_tx, accum_rx, accum_ang;
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.c b/drivers/net/wireless/ath/ath9k/eeprom.c
index efb7889142d47..061d33921495c 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom.c
@@ -480,7 +480,7 @@  void ath9k_hw_get_gain_boundaries_pdadcs(struct ath_hw *ah,
 		[AR5416_MAX_PWR_RANGE_IN_HALF_DB];
 
 	u8 *pVpdL, *pVpdR, *pPwrL, *pPwrR;
-	u8 minPwrT4[AR5416_NUM_PD_GAINS];
+	u8 minPwrT4[AR5416_NUM_PD_GAINS] = { 0 };
 	u8 maxPwrT4[AR5416_NUM_PD_GAINS];
 	int16_t vpdStep;
 	int16_t tmpVal;
@@ -500,7 +500,6 @@  void ath9k_hw_get_gain_boundaries_pdadcs(struct ath_hw *ah,
 	else
 		intercepts = AR5416_PD_GAIN_ICEPTS;
 
-	memset(&minPwrT4, 0, AR5416_NUM_PD_GAINS);
 	ath9k_hw_get_channel_centers(ah, chan, &centers);
 
 	for (numPiers = 0; numPiers < availPiers; numPiers++) {
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index e8c2cc03be0cb..1d295d7fa0848 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -583,12 +583,10 @@  static void ath9k_hw_4k_set_txpower(struct ath_hw *ah,
 	struct ath_regulatory *regulatory = ath9k_hw_regulatory(ah);
 	struct ar5416_eeprom_4k *pEepData = &ah->eeprom.map4k;
 	struct modal_eep_4k_header *pModal = &pEepData->modalHeader;
-	int16_t ratesArray[Ar5416RateSize];
+	int16_t ratesArray[Ar5416RateSize] = { 0 };
 	u8 ht40PowerIncForPdadc = 2;
 	int i;
 
-	memset(ratesArray, 0, sizeof(ratesArray));
-
 	if (ath9k_hw_4k_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2)
 		ht40PowerIncForPdadc = pModal->ht40PowerIncForPdadc;
 
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index 3caa149b10131..b068e15226022 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -711,12 +711,10 @@  static void ath9k_hw_ar9287_set_txpower(struct ath_hw *ah,
 	struct ath_regulatory *regulatory = ath9k_hw_regulatory(ah);
 	struct ar9287_eeprom *pEepData = &ah->eeprom.map9287;
 	struct modal_eep_ar9287_header *pModal = &pEepData->modalHeader;
-	int16_t ratesArray[Ar5416RateSize];
+	int16_t ratesArray[Ar5416RateSize] = { 0 };
 	u8 ht40PowerIncForPdadc = 2;
 	int i;
 
-	memset(ratesArray, 0, sizeof(ratesArray));
-
 	if (ath9k_hw_ar9287_get_eeprom_rev(ah) >= AR9287_EEP_MINOR_VER_2)
 		ht40PowerIncForPdadc = pModal->ht40PowerIncForPdadc;
 
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index 9729a69d3e2e3..b5ee261c86382 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -1150,12 +1150,10 @@  static void ath9k_hw_def_set_txpower(struct ath_hw *ah,
 	struct ar5416_eeprom_def *pEepData = &ah->eeprom.def;
 	struct modal_eep_header *pModal =
 		&(pEepData->modalHeader[IS_CHAN_2GHZ(chan)]);
-	int16_t ratesArray[Ar5416RateSize];
+	int16_t ratesArray[Ar5416RateSize] = { 0 };
 	u8 ht40PowerIncForPdadc = 2;
 	int i, cck_ofdm_delta = 0;
 
-	memset(ratesArray, 0, sizeof(ratesArray));
-
 	if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2)
 		ht40PowerIncForPdadc = pModal->ht40PowerIncForPdadc;
 
diff --git a/drivers/net/wireless/ath/ath9k/wow.c b/drivers/net/wireless/ath/ath9k/wow.c
index 8d0b1730a9d5b..3d39c7ec1da30 100644
--- a/drivers/net/wireless/ath/ath9k/wow.c
+++ b/drivers/net/wireless/ath/ath9k/wow.c
@@ -53,11 +53,8 @@  static int ath9k_wow_add_disassoc_deauth_pattern(struct ath_softc *sc)
 	struct ath_common *common = ath9k_hw_common(ah);
 	int pattern_count = 0;
 	int ret, i, byte_cnt = 0;
-	u8 dis_deauth_pattern[MAX_PATTERN_SIZE];
-	u8 dis_deauth_mask[MAX_PATTERN_SIZE];
-
-	memset(dis_deauth_pattern, 0, MAX_PATTERN_SIZE);
-	memset(dis_deauth_mask, 0, MAX_PATTERN_SIZE);
+	u8 dis_deauth_pattern[MAX_PATTERN_SIZE] = { 0 };
+	u8 dis_deauth_mask[MAX_PATTERN_SIZE] = { 0 };
 
 	/*
 	 * Create Dissassociate / Deauthenticate packet filter