[PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size

[PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size

[Dmitry Antipov]

In 'iwl_pnvm_load()', assume that the power table size is always
equal to IWL_HARDCODED_REDUCE_POWER_SIZE. Compile tested only.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
I would gently ask Johannes to review this before taking any other
actions. This is intended for stable linux-6.1.y only in attempt to
avoid possible use of an uninitialized 'len' without backporting
https://lore.kernel.org/linux-wireless/20230606074310.889520-1-gregory.greenman@intel.com.
---
 drivers/net/wireless/intel/iwlwifi/fw/pnvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c b/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c
index b6d3ac6ed440..ddf7acd67e94 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c
+++ b/drivers/net/wireless/intel/iwlwifi/fw/pnvm.c
@@ -332,6 +332,9 @@ int iwl_pnvm_load(struct iwl_trans *trans,
 
 			goto skip_reduce_power;
 		}
+	} else {
+		/* see iwl_uefi_get_reduced_power() to check why */
+		len = IWL_HARDCODED_REDUCE_POWER_SIZE;
 	}
 
 	ret = iwl_trans_set_reduce_power(trans, data, len);
-- 
2.48.1

Re: [PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-3

Rule: The upstream commit ID must be specified with a separate line above the commit text.
Subject: [PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size
Link: https://lore.kernel.org/stable/20250129103120.1985802-1-dmantipov%40yandex.ru

Please ignore this mail if the patch is not relevant for upstream.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size

[Johannes Berg]

On Wed, 2025-01-29 at 13:31 +0300, Dmitry Antipov wrote:
> In 'iwl_pnvm_load()', assume that the power table size is always
> equal to IWL_HARDCODED_REDUCE_POWER_SIZE. Compile tested only.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> I would gently ask Johannes to review this before taking any other
> actions. This is intended for stable linux-6.1.y only in attempt to
> avoid possible use of an uninitialized 'len' without backporting

I don't see that there's uninitialized use of 'len'. Maybe some static
checkers aren't seeing through this, but the code is fine:

If iwl_pnvm_get_from_fs() is successful, then 'len' is initialized. If
it fails, we goto skip_parse.

There, if trans->reduce_power_loaded is false, 'len' again is either
initialized or we go to skip_reduce_power and never use 'len'.

If trans->reduce_power_loaded is true, then we get to
iwl_trans_pcie_ctx_info_gen3_set_reduce_power() which doesn't use 'len'
in this case.

I'd rather not fix this non-problem in a very confusing way.

johannes

Re: [PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size

[Dmitry Antipov]

On 1/29/25 1:48 PM, Johannes Berg wrote:

> I don't see that there's uninitialized use of 'len'. Maybe some static
> checkers aren't seeing through this, but the code is fine:
> 
> If iwl_pnvm_get_from_fs() is successful, then 'len' is initialized. If
> it fails, we goto skip_parse.
> 
> There, if trans->reduce_power_loaded is false, 'len' again is either
> initialized or we go to skip_reduce_power and never use 'len'.
> 
> If trans->reduce_power_loaded is true, then we get to
> iwl_trans_pcie_ctx_info_gen3_set_reduce_power() which doesn't use 'len'
> in this case.

Hm. As of 6.1.127, what I'm seeing is ('cat -n' annotated by me):

    258	int iwl_pnvm_load(struct iwl_trans *trans,
    259			  struct iwl_notif_wait_data *notif_wait)
    260	{
    261		u8 *data;
    262		size_t len;                                                     /* uninitialized */
    263		struct pnvm_sku_package *package;
    264		struct iwl_notification_wait pnvm_wait;
    265		static const u16 ntf_cmds[] = { WIDE_ID(REGULATORY_AND_NVM_GROUP,
    266							PNVM_INIT_COMPLETE_NTFY) };
    267		int ret;
    268	
    269		/* if the SKU_ID is empty, there's nothing to do */
    270		if (!trans->sku_id[0] && !trans->sku_id[1] && !trans->sku_id[2])
    271			return 0;
    272	
    273		/*
    274		 * If we already loaded (or tried to load) it before, we just
    275		 * need to set it again.
    276		 */
    277		if (trans->pnvm_loaded) {
    278			ret = iwl_trans_set_pnvm(trans, NULL, 0);
    279			if (ret)
    280				return ret;
    281			goto skip_parse;                                        /* taking goto */
    282		}
    283	
    284		/* First attempt to get the PNVM from BIOS */
    285		package = iwl_uefi_get_pnvm(trans, &len);
    286		if (!IS_ERR_OR_NULL(package)) {
    287			if (len >= sizeof(*package)) {
    288				/* we need only the data */
    289				len -= sizeof(*package);
    290				data = kmemdup(package->data, len, GFP_KERNEL);
    291			} else {
    292				data = NULL;
    293			}
    294	
    295			/* free package regardless of whether kmemdup succeeded */
    296			kfree(package);
    297	
    298			if (data)
    299				goto parse;
    300		}
    301	
    302		/* If it's not available, try from the filesystem */
    303		ret = iwl_pnvm_get_from_fs(trans, &data, &len);
    304		if (ret) {
    305			/*
    306			 * Pretend we've loaded it - at least we've tried and
    307			 * couldn't load it at all, so there's no point in
    308			 * trying again over and over.
    309			 */
    310			trans->pnvm_loaded = true;
    311	
    312			goto skip_parse;
    313		}
    314	
    315	parse:
    316		iwl_pnvm_parse(trans, data, len);
    317	
    318		kfree(data);
    319	
    320	skip_parse:
    321		data = NULL;
    322		/* now try to get the reduce power table, if not loaded yet */
    323		if (!trans->reduce_power_loaded) {                              /* the condition is false */
    324			data = iwl_uefi_get_reduced_power(trans, &len);
    325			if (IS_ERR_OR_NULL(data)) {
    326				/*
    327				 * Pretend we've loaded it - at least we've tried and
    328				 * couldn't load it at all, so there's no point in
    329				 * trying again over and over.
    330				 */
    331				trans->reduce_power_loaded = true;
    332	
    333				goto skip_reduce_power;
    334			}
    335		}
    336	
    337		ret = iwl_trans_set_reduce_power(trans, data, len);             /* len - ? */
    338		if (ret)
    339			IWL_DEBUG_FW(trans,
    340				     "Failed to set reduce power table %d\n",
    341				     ret);
    342		kfree(data);

Am I missing something?

Dmitry

Re: [PATCH 6.1] wifi: iwlwifi: assume known PNVM power table size

[Johannes Berg]

On Wed, 2025-01-29 at 14:50 +0300, Dmitry Antipov wrote:
> On 1/29/25 1:48 PM, Johannes Berg wrote:
> 
> > I don't see that there's uninitialized use of 'len'. Maybe some static
> > checkers aren't seeing through this, but the code is fine:
> > 
> > If iwl_pnvm_get_from_fs() is successful, then 'len' is initialized. If
> > it fails, we goto skip_parse.
> > 

[... reordering too ...]

> Am I missing something?

You missed this:

> > There, if trans->reduce_power_loaded is false, 'len' again is either
> > initialized or we go to skip_reduce_power and never use 'len'.
> > 
> > If trans->reduce_power_loaded is true, then we get to
> > iwl_trans_pcie_ctx_info_gen3_set_reduce_power() which doesn't use 'len'
> > in this case.


johannes