Re: [PATCH v9 4/7] bus: mhi: Add QDU100 Sahara variant and firmware fallback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mhi: sahara: patch changes behavior for AIC100/AIC200 despite commit message claims
--

commit 95400c2cb3bcfef8a4adb0c664417375ff8137c7
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

bus: mhi: Add QDU100 Sahara variant and firmware fallback

Extend the probe time variant selection to match the SAHARA MHI channel and
associate it with the QDU100 firmware folder. Add a firmware lookup fallback
for cases where an image does not have an explicit entry in the device's
firmware table.

> This change only affects devices matched on the SAHARA MHI channel and
> does not change behavior for existing AIC100 and AIC200 devices.

Is this description accurate?

The qaic controller driver configures aic100_sahara_fw and aic200_sahara_fw to
populate the fw_folder field as "aic100" and "aic200".

With the new fallback logic in sahara_find_image(), if an existing AIC device
requests an unknown image ID, it appears it will now trigger the new fallback
path and perform synchronous VFS lookups for "qcom/aic.../<id>", whereas
previously it would immediately fail with -EINVAL.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index ce4fcbd40baa1..a4b3ef197b303 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
[ ... ]
> @@ -200,8 +201,28 @@ static int sahara_find_image(struct sahara_context *context, u32 image_id)
>  	}
>  
>  	if (image_id >= context->table_size || !context->image_table[image_id]) {
> -		dev_err(&context->mhi_dev->dev, "request for unknown image: %d\n", image_id);
> -		return -EINVAL;
> +		if (!context->fw_folder) {
> +			dev_err(&context->mhi_dev->dev,
> +				"Request for unknown image: %u (no fw folder)\n", image_id);
> +			return -EINVAL;
> +		}
> +
> +		fw_path = kasprintf(GFP_KERNEL, "qcom/%s/%u",
> +				    context->fw_folder, image_id);
> +		if (!fw_path)
> +			return -ENOMEM;
> +
> +		ret = firmware_request_nowarn(&context->firmware,
> +					      fw_path,
> +					      &context->mhi_dev->dev);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Could this fallback path introduce a regression for AIC100 and AIC200
devices? It looks like those devices have a fw_folder configured, so they
would execute this filesystem lookup for unknown IDs rather than immediately
failing with -EINVAL as they did before this patch.

> +		kfree(fw_path);
> +		if (ret) {
> +			dev_err(&context->mhi_dev->dev,
> +				"request for unknown image: %d\n", image_id);
> +			return -EINVAL;
> +		}
> +		context->active_image_id = image_id;
> +		return 0;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-sahara_protocol_new_v2-v9-0-dc9f24357c4a@oss.qualcomm.com?part=4