netdev - Re: [PATCH iwl-next] ice: Disable Cage Max Power override

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dd439a56-81da-a7ed-84ed-c04afd50b836@intel.com>
Date: Tue, 22 Aug 2023 10:16:58 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-next] ice: Disable Cage Max Power override

On 8/22/23 09:34, Wojciech Drewek wrote:
> NVM module called "Cage Max Power override" allows to
> change max power in the cage. This can be achieved
> using external tools. The responsibility of the ice driver is to
> go back to the default settings whenever port split is done.
> This is achieved by clearing Override Enable bit in the
> NVM module. Override of the max power is disabled so the
> default value will be used.
> 
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>
> ---
>   .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  9 +++++
>   drivers/net/ethernet/intel/ice/ice_devlink.c  | 35 +++++++++++++++++++
>   drivers/net/ethernet/intel/ice/ice_nvm.c      |  2 +-
>   drivers/net/ethernet/intel/ice/ice_nvm.h      |  4 +++
>   4 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> index ffbe9d3a5d77..a3a49d922650 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> @@ -1569,6 +1569,15 @@ struct ice_aqc_nvm {
>   	__le32 addr_low;
>   };
>   
> +#define ICE_AQC_NVM_CMPO_MOD_ID			0x153
> +
> +/* Cage Max Power override NVM module */
> +struct ice_aqc_nvm_cmpo {
> +	__le16 length;
> +#define ICE_AQC_NVM_CMPO_ENABLE	BIT(8)
> +	__le16 cages_cfg[8];

[1] here

> +};
> +
>   #define ICE_AQC_NVM_START_POINT			0
>   
>   /* NVM Checksum Command (direct, 0x0706) */
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index 80dc5445b50d..e9300df9ef40 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -591,6 +591,36 @@ static void ice_devlink_port_options_print(struct ice_pf *pf)
>   	kfree(options);
>   }
>   
> +#define ICE_NUM_OF_CAGES 8

perhaps move this define to ice_adminq_cmd.h to benefit from it in 
struct definition just few lines above [1]
> +
> +/**
> + * ice_devlink_aq_clear_cmpo - clear Cage Max Power override
> + * @hw: pointer to the HW struct
> + *
> + * Read Cage Max Power override NVM module, clear override
> + * enable bit for each of the cages. Write the settings back to
> + * the NVM.

Read+clear+write is "how" or algorithm here, but doc should just stick 
to "what" most of the time. So I would just:
"Clear Cage Max Power override enable bit for each of the cages".

"how" part could be inside the function, just above "read" call.

> + */
> +static int
> +ice_devlink_aq_clear_cmpo(struct ice_hw *hw)
> +{
> +	struct ice_aqc_nvm_cmpo data;
> +	int ret, i;
> +
> +	ret = ice_aq_read_nvm(hw, ICE_AQC_NVM_CMPO_MOD_ID, 0, sizeof(data),
> +			      &data, true, false, NULL);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ICE_NUM_OF_CAGES; i++)
> +		data.cages_cfg[i] &= ~cpu_to_le16(ICE_AQC_NVM_CMPO_ENABLE);
> +
> +	/* Do not update the length word since it is not permitted */
> +	return ice_aq_update_nvm(hw, ICE_AQC_NVM_CMPO_MOD_ID, 2,
> +				 sizeof(data.cages_cfg), data.cages_cfg,
> +				 false, 0, NULL);
> +}
> +
>   /**
>    * ice_devlink_aq_set_port_option - Send set port option admin queue command
>    * @pf: the PF to print split port options
> @@ -623,6 +653,11 @@ ice_devlink_aq_set_port_option(struct ice_pf *pf, u8 option_idx,
>   		return -EIO;
>   	}
>   
> +	status = ice_devlink_aq_clear_cmpo(&pf->hw);
> +	if (status)
> +		dev_dbg(dev, "Failed to clear Cage Max Power override, err %d aq_err %d\n",
> +			status, pf->hw.adminq.sq_last_status);
> +
>   	status = ice_nvm_write_activate(&pf->hw, ICE_AQC_NVM_ACTIV_REQ_EMPR, NULL);
>   	if (status) {
>   		dev_dbg(dev, "ice_nvm_write_activate failed, err %d aq_err %d\n",
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index f6f52a248066..745f2459943f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> @@ -18,7 +18,7 @@
>    *
>    * Read the NVM using the admin queue commands (0x0701)
>    */
> -static int
> +int
>   ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
>   		void *data, bool last_command, bool read_shadow_ram,
>   		struct ice_sq_cd *cd)
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.h b/drivers/net/ethernet/intel/ice/ice_nvm.h
> index 774c2317967d..90f36e19e06b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.h
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.h
> @@ -15,6 +15,10 @@ struct ice_orom_civd_info {
>   int ice_acquire_nvm(struct ice_hw *hw, enum ice_aq_res_access_type access);
>   void ice_release_nvm(struct ice_hw *hw);
>   int
> +ice_aq_read_nvm(struct ice_hw *hw, u16 module_typeid, u32 offset, u16 length,
> +		void *data, bool last_command, bool read_shadow_ram,
> +		struct ice_sq_cd *cd);
> +int
>   ice_read_flat_nvm(struct ice_hw *hw, u32 offset, u32 *length, u8 *data,
>   		  bool read_shadow_ram);
>   int