[v1] ath9k: initialize arrays at compile time

[PATCH] ath9k: initialize arrays at compile time

Posted by trix@redhat.com 4 years, 3 months ago

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(-)

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
-- 
2.26.3

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by Joe Perches 4 years, 3 months ago

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...

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by John Crispin 4 years, 3 months ago


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

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by Sebastian Gottschall 4 years, 3 months ago

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

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by Joe Perches 4 years, 3 months ago

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.

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by Tom Rix 4 years, 3 months ago

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

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by Andreas Schwab 4 years, 3 months ago

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).

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

RE: [PATCH] ath9k: initialize arrays at compile time

Posted by David Laight 4 years, 3 months ago

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)

Re: [PATCH] ath9k: initialize arrays at compile time

Posted by Toke Høiland-Jørgensen 4 years, 3 months ago

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