Re: [PATCH v7 net-next 5/5] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Simon Horman <horms@kernel.org>]

On Mon, Mar 23, 2026 at 09:21:10AM +0530, Ratheesh Kannoth wrote:
> Flashing updated firmware on deployed devices is cumbersome. Provide a
> mechanism to load a custom KPU (Key Parse Unit) profile directly from
> the filesystem at module load time.
> 
> When the rvu_af module is loaded with the kpu_profile parameter, the
> specified profile is read from /lib/firmware/kpu and programmed into
> the KPU registers. Add npc_kpu_profile_cam2 for the extended cam format
> used by filesystem-loaded profiles and support ptype/ptype_mask in
> npc_config_kpucam when profile->from_fs is set.
> 
> Usage:
>   1. Copy the KPU profile file to /lib/firmware/kpu.
>   2. Build OCTEONTX2_AF as a module.
>   3. Load: insmod rvu_af.ko kpu_profile=<profile_name>
> 
> Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c

...

> @@ -1730,32 +1835,90 @@ static int npc_apply_custom_kpu(struct rvu *rvu,
>  		return -EINVAL;
>  	}
>  
> +	*fw_kpus = fw->kpus;
> +
> +	sfw = devm_kcalloc(rvu->dev, 1, sizeof(*sfw), GFP_KERNEL);
> +	if (!sfw)
> +		return -ENOMEM;
> +
> +	memcpy(sfw, fw, sizeof(*sfw));
> +
>  	profile->custom = 1;
> -	profile->name = fw->name;
> +	profile->name = sfw->name;
>  	profile->version = le64_to_cpu(fw->version);
> -	profile->mcam_kex_prfl.mkex = &fw->mkex;
> -	profile->lt_def = &fw->lt_def;
> +	profile->mcam_kex_prfl.mkex = &sfw->mkex;
> +	profile->lt_def = &sfw->lt_def;
> +
> +	/* Binary blob contains ikpu actions entries at start of data[0] */
> +	if (from_fs) {
> +		profile->ikpu2 = devm_kcalloc(rvu->dev, 1,
> +					      sizeof(ikpu_action_entries),
> +					      GFP_KERNEL);
> +		if (!profile->ikpu2)
> +			return -ENOMEM;
> +
> +		action = (struct npc_kpu_profile_action *)(fw->data + offset);
> +
> +		if (rvu->kpu_fwdata_sz < hdr_sz + sizeof(ikpu_action_entries))
> +			return -ENOMEM;
> +
> +		memcpy((void *)profile->ikpu2, action, sizeof(ikpu_action_entries));
> +		offset += sizeof(ikpu_action_entries);
> +	}
>  
>  	for (kpu = 0; kpu < fw->kpus; kpu++) {
>  		fw_kpu = (struct npc_kpu_fwdata *)(fw->data + offset);
> -		if (fw_kpu->entries > KPU_MAX_CST_ENT)
> -			dev_warn(rvu->dev,
> -				 "Too many custom entries on KPU%d: %d > %d\n",
> -				 kpu, fw_kpu->entries, KPU_MAX_CST_ENT);
> -		entries = min(fw_kpu->entries, KPU_MAX_CST_ENT);
> -		cam = (struct npc_kpu_profile_cam *)fw_kpu->data;
> -		offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam);
> +		if (!from_fs) {
> +			if (fw_kpu->entries > KPU_MAX_CST_ENT)
> +				dev_warn(rvu->dev,
> +					 "Too many custom entries on KPU%d: %d > %d\n",
> +					 kpu, fw_kpu->entries, KPU_MAX_CST_ENT);
> +			entries = min(fw_kpu->entries, KPU_MAX_CST_ENT);
> +			cam = (struct npc_kpu_profile_cam *)fw_kpu->data;
> +			offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam);
> +			action = (struct npc_kpu_profile_action *)(fw->data + offset);
> +			offset += fw_kpu->entries * sizeof(*action);
> +			if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
> +				dev_warn(rvu->dev,
> +					 "Profile size mismatch on KPU%i parsing.\n",
> +					 kpu + 1);
> +				return -EINVAL;
> +			}
> +			for (entry = 0; entry < entries; entry++) {
> +				profile->kpu[kpu].cam[entry] = cam[entry];
> +				profile->kpu[kpu].action[entry] = action[entry];
> +			}
> +			continue;
> +		}

Effectively the code below is the else arm of the condition above.
I would suggest that moving the contents of both arms into helpers
would lead to a cleaner implementation.

> +
> +		entries = min(fw_kpu->entries, rvu->hw->npc_kpu_entries);
> +		dev_info(rvu->dev,
> +			 "Loading %u entries on KPU%d\n", entries, kpu);
> +
> +		cam2 = (struct npc_kpu_profile_cam2 *)fw_kpu->data;
> +		offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam2);
>  		action = (struct npc_kpu_profile_action *)(fw->data + offset);
>  		offset += fw_kpu->entries * sizeof(*action);
>  		if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
>  			dev_warn(rvu->dev,
> -				 "Profile size mismatch on KPU%i parsing.\n",
> +				 "profile size mismatch on kpu%i parsing.\n",
>  				 kpu + 1);
>  			return -EINVAL;
>  		}
> +
> +		profile->kpu[kpu].cam_entries2 = entries;
> +		profile->kpu[kpu].action_entries2 = entries;
> +		ret = npc_alloc_kpu_cam2_n_action2(rvu, kpu, entries);
> +		if (ret) {
> +			dev_warn(rvu->dev,
> +				 "profile entry allocation failed for kpu=%d for %d entries\n",
> +				 kpu, entries);
> +			return -EINVAL;
> +		}
> +
>  		for (entry = 0; entry < entries; entry++) {
> -			profile->kpu[kpu].cam[entry] = cam[entry];
> -			profile->kpu[kpu].action[entry] = action[entry];
> +			profile->kpu[kpu].cam2[entry] = cam2[entry];
> +			profile->kpu[kpu].action2[entry] = action[entry];
>  		}

Perhaps it applies to the existing code as well. But I think
the new code to handle parsing images read from the file system
could benefit from bounds checking. Basically passing in the length
of the firmware image as an argument and ensuring that data beyond
that length isn't accessed.

>  	}
>  
> @@ -1852,7 +2015,10 @@ void npc_load_kpu_profile(struct rvu *rvu)
>  	struct npc_kpu_profile_adapter *profile = &rvu->kpu;
>  	const char *kpu_profile = rvu->kpu_pfl_name;
>  	const struct firmware *fw = NULL;
> -	bool retry_fwdb = false;
> +	int len, ret, fw_kpus = 0;
> +	char *path;
> +
> +	profile->from_fs = false;
>  
>  	/* If user not specified profile customization */
>  	if (!strncmp(kpu_profile, def_pfl_name, KPU_NAME_LEN))
> @@ -1865,27 +2031,57 @@ void npc_load_kpu_profile(struct rvu *rvu)
>  	 * Firmware database method.
>  	 * Default KPU profile.
>  	 */
> -	if (!request_firmware_direct(&fw, kpu_profile, rvu->dev)) {
> -		dev_info(rvu->dev, "Loading KPU profile from firmware: %s\n",
> -			 kpu_profile);
> +
> +	/* No support for filesystem KPU loading */
> +	if (is_cn20k(rvu->pdev))
> +		goto load_image_fwdb;
> +
> +#define PDIR "kpu/"

Please make the name of this define less generic - e.g. prefix
it with the name of the driver. And please put it towards
the top of this file.

> +	len = strlen(kpu_profile) + sizeof(PDIR);
> +	path = kmalloc(len, GFP_KERNEL);
> +	if (!path)
> +		return;
> +
> +	strscpy(path, PDIR, len);
> +	strcat(path, kpu_profile);
> +	if (!request_firmware_direct(&fw, path, rvu->dev)) {
> +		dev_info(rvu->dev, "Loading KPU profile from filesystem: %s\n",
> +			 path);
>  		rvu->kpu_fwdata = kzalloc(fw->size, GFP_KERNEL);
>  		if (rvu->kpu_fwdata) {
>  			memcpy(rvu->kpu_fwdata, fw->data, fw->size);
>  			rvu->kpu_fwdata_sz = fw->size;
>  		}
>  		release_firmware(fw);
> -		retry_fwdb = true;
> -		goto program_kpu;
> +		kfree(path);
> +
> +		ret = npc_apply_custom_kpu(rvu, profile, true, &fw_kpus);
> +		kfree(rvu->kpu_fwdata);
> +		rvu->kpu_fwdata = NULL;
> +
> +		if (ret) {
> +			rvu->kpu_fwdata_sz = 0;
> +			npc_prepare_default_kpu(rvu, profile);
> +			dev_err(rvu->dev,
> +				"Loading KPU profile from filesystem failed\n");
> +			goto load_image_fwdb;
> +		}
> +
> +		/* Cn20k does not support filesystem loading */
> +		rvu->kpu.kpus = fw_kpus;
> +		profile->from_fs = true;
> +		return;
>  	}
> +	kfree(path);
>  
>  load_image_fwdb:

I know that it was here before this patch, so this is a suggestion for a
follow-up: as this isn't a fast path I think it would be good to refactor
this code so it doesn't use goto labels as part of it's control of
conditional execution for non-error paths.

>  	/* Loading the KPU profile using firmware database */
>  	if (npc_load_kpu_profile_fwdb(rvu, kpu_profile))
>  		goto revert_to_default;
>  
> -program_kpu:
>  	/* Apply profile customization if firmware was loaded. */
> -	if (!rvu->kpu_fwdata_sz || npc_apply_custom_kpu(rvu, profile)) {
> +	if (!rvu->kpu_fwdata_sz ||
> +	    npc_apply_custom_kpu(rvu, profile, false, &fw_kpus)) {
>  		/* If image from firmware filesystem fails to load or invalid
>  		 * retry with firmware database method.
>  		 */

...