Re: [PATCH iwl-next v2 2/2] ice: reject partial NVM update on eRoT adapters

[Tomasz Lichwala <tomasz.lichwala@linux.intel.com>]

On 8.10.2026 16:14, Aleksandr Loktionov wrote:
> diff --git a/drivers/net/ethernet/intel/ice/ice_fw_update.c b/drivers/net/ethernet/intel/ice/ice_fw_update.c
> index 3631461..9da7df6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fw_update.c
> +++ b/drivers/net/ethernet/intel/ice/ice_fw_update.c
> @@ -220,12 +232,27 @@ ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
>  	struct ice_pf *pf = priv->pf;
>  	struct ice_hw *hw = &pf->hw;
>  	size_t length;
> +	u8 comp_bit;
>  	int status;
>  
>  	switch (component->identifier) {
>  	case NVM_COMP_ID_OROM:
> +		comp_bit = ICE_FWU_COMP_OROM;
> +		break;
>  	case NVM_COMP_ID_NVM:
> +		comp_bit = ICE_FWU_COMP_NVM;
> +		break;
>  	case NVM_COMP_ID_NETLIST:
> +		comp_bit = ICE_FWU_COMP_NETLIST;
> +		break;
> +	case NVM_COMP_ID_MANIFEST:
> +		if (!priv->erot_present) {
> +			dev_err(dev, "Manifest component is not supported on adapters without eRoT\n");
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Manifest component is not supported on adapters without eRoT");
> +			return -EOPNOTSUPP;
> +		}
> +		comp_bit = ICE_FWU_COMP_MANIFEST;
>  		break;
>  	default:
>  		dev_err(dev, "Unable to update due to a firmware component with unknown ID %u\n",
> @@ -234,6 +261,14 @@ ice_send_component_table(struct pldmfw *context, struct pldmfw_component *compon
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (priv->erot_present && (priv->seen_components & comp_bit)) {
> +		dev_err(dev, "Duplicate component in PLDM image: component ID 0x%02x\n",

Nit: same dual dev_err()/extack reporting pattern already flagged for the Manifest-without-eRoT case above - please fix both occurrences together.

> +			component->identifier);
> +		NL_SET_ERR_MSG_MOD(extack, "Duplicate component in PLDM image");
> +		return -EOPNOTSUPP;
> +	}
> +	priv->seen_components |= comp_bit;
> +
>  	length = struct_size(comp_tbl, cvs, component->version_len);
>  	comp_tbl = kzalloc(length, GFP_KERNEL);
>  	if (!comp_tbl)
> @@ -648,6 +683,12 @@ ice_flash_component(struct pldmfw *context, struct pldmfw_component *component)
>  	u8 flag;
>  	int err;
>  
> +	if (priv->erot_present && priv->seen_components != ICE_FWU_COMP_ALL_EROT) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "eRoT adapter requires all four components (NVM, OROM, NetList, Manifest) in a single update");
> +		return -EINVAL;

Nit: duplicate-component rejection uses -EOPNOTSUPP while this (equally malformed-image) case uses -EINVAL. Consider using one consistently.

> +	}
> +
>  	switch (component->identifier) {
>  	case NVM_COMP_ID_OROM:
>  		module = ICE_SR_1ST_OROM_BANK_PTR;
> @@ -667,6 +708,13 @@ ice_flash_component(struct pldmfw *context, struct pldmfw_component *component)
>  		reset_level = NULL;
>  		name = "fw.netlist";
>  		break;
> +	case NVM_COMP_ID_MANIFEST:
> +		module = ICE_SR_PQC_MANIFEST_BANK_PTR;
> +		/* The AQ activation-select mask has no Manifest bit. */
> +		flag = 0;
> +		reset_level = NULL;
> +		name = "fw.pqc_manifest";

ICE_AQC_NVM_ACTIV_SEL_MASK only covers bits 3-5 (NVM/OROM/NetList), so there's no activation bit for Manifest. ice_get_pending_updates()/ice_cancel_pending_update() also have no notion of a pending Manifest update, so canceling a pending 4-component eRoT update reverts NVM/OROM/NetList but leaves Manifest state undefined. Please confirm the firmware activation contract for Manifest before merging - the cover letter itself says this is unconfirmed and not hardware-validated.

> +		break;
>  	default:
>  		/* This should not trigger, since we check the id before
>  		 * sending the component table to firmware.
> @@ -706,6 +754,12 @@ static int ice_finalize_update(struct pldmfw *context)
>  	struct devlink *devlink;
>  	int err;
>  
> +	if (priv->erot_present && priv->seen_components != ICE_FWU_COMP_ALL_EROT) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "eRoT adapter requires all four components (NVM, OROM, NetList, Manifest) in a single update");
> +		return -EINVAL;
> +	}
> +
>  	/* Finally, notify firmware to activate the written NVM banks */
>  	err = ice_switch_flash_banks(pf, priv->activate_flags,
>  				     &priv->emp_reset_available, extack);
> @@ -981,9 +1035,9 @@ ice_cancel_pending_update(struct ice_pf *pf, const char *component,
>   * formatted image that matches this device.
>   *
>   * Extract the device record Package Data and Component Tables and send them
> - * to the firmware. Extract and write the flash data for each of the three
> - * main flash components, "fw.mgmt", "fw.undi", and "fw.netlist". Notify
> - * firmware once the data is written to the inactive banks.
> + * to the firmware. Extract and write the flash data for the selected firmware
> + * components: "fw.mgmt", "fw.undi", "fw.netlist", and, when firmware reports
> + * eRoT presence, "fw.pqc_manifest". Notify firmware once the data is written.
>   *
>   * Returns: zero on success or a negative error code on failure.
>   */
> @@ -1013,12 +1067,37 @@ int ice_devlink_flash_update(struct devlink *devlink,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	if (!hw->dev_caps.common_cap.nvm_unified_update && !ice_is_recovery_mode(hw)) {
> +	struct ice_hw_dev_caps *dev_caps __free(kfree) =
> +		kzalloc_obj(*dev_caps);
> +	if (!dev_caps)
> +		return -ENOMEM;
> +
> +	err = ice_discover_dev_caps(hw, dev_caps);

This duplicates the ice_discover_dev_caps() call already done inside ice_get_pending_updates(), reached right after via ice_cancel_pending_update() — two AQ list-caps round-trips per flash attempt. Also note ice_switch_flash_banks()/ice_write_one_nvm_block() still read the cached hw->dev_caps.common_cap.* instead of this fresh snapshot, so the "per-update snapshot" isn't actually used consistently within the same update.

> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Cannot determine eRoT state: unable to read firmware device capabilities");
> +		return err;
> +	}
> +
> +	if (dev_caps->common_cap.external_pqc_rot_invalid) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Invalid eRoT capability reported by firmware");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!dev_caps->common_cap.nvm_unified_update && !ice_is_recovery_mode(hw)) {
>  		NL_SET_ERR_MSG_MOD(extack, "Current firmware does not support unified update");
>  		return -EOPNOTSUPP;
>  	}
>  
>  	memset(&priv, 0, sizeof(priv));
> +	priv.erot_present = dev_caps->common_cap.external_pqc_rot_present;
> +
> +	if (params->component && priv.erot_present) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "eRoT adapter does not support single component update");
> +		return -EOPNOTSUPP;
> +	}
>  
>  	if (params->component && strcmp(params->component, "fw.mgmt") == 0) {
>  		priv.context.mode = PLDMFW_UPDATE_MODE_SINGLE_COMPONENT;
		priv.context.component_identifier = NVM_COMP_ID_NVM;
	} else if (params->component) {
		return -EOPNOTSUPP;
	}

	/* the E822 device needs a slightly different ops */
	if (hw->mac_type == ICE_MAC_GENERIC)
		priv.context.ops = &ice_fwu_ops_e822;
	else
		priv.context.ops = &ice_fwu_ops_e810;
	priv.context.dev = dev;
	priv.extack = extack;
	priv.pf = pf;
	priv.activate_flags = preservation;

	devlink_flash_update_status_notify(devlink, "Preparing to flash", NULL, 0, 0);

	err = ice_cancel_pending_update(pf, NULL, extack);

This unconditionally reverts any previously staged pending update before the new completeness/duplicate checks run (they only fire later, inside the pldmfw callbacks). So a malformed eRoT image is rejected "before any bank is erased" as documented, but only after a valid prior pending update has already been reverted. Worth calling out explicitly in the docs, or checking completeness before canceling.

	if (err)
		return err;


> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
> index 1fede0f..43f3208 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -1151,6 +1151,7 @@ struct ice_aq_get_set_rss_lut_params {
>  #define ICE_SR_OROM_BANK_SIZE		0x45
>  #define ICE_SR_NETLIST_BANK_PTR		0x46
>  #define ICE_SR_NETLIST_BANK_SIZE	0x47
> +#define ICE_SR_PQC_MANIFEST_BANK_PTR	0x4E
>  #define ICE_SR_SECTOR_SIZE_IN_WORDS	0x800
>  
>  /* CSS Header words */

Thanks,
Tomasz