Re: [PATCH v7 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] sahara: unintended firmware fallback behavior for AIC100/AIC200
--

commit 02c795f9adf45b209ef5da56b858e0490ac4667d
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

bus: mhi: Add QDU100 Sahara variant and firmware fallback

This commit extends the Sahara driver to recognize QDU100 devices and adds
a firmware lookup fallback for unspecified image IDs. It allows QDU100
devices to request required images to be provisioned by the platform.

> bus: mhi: Add QDU100 Sahara variant and firmware fallback
>
> The Sahara driver currently selects a firmware image table based on the
> attached device, but it does not recognize QDU100 devices that expose the
> protocol on the SAHARA MHI channel. As a result, the host cannot associate
> QDU100 devices with the correct firmware namespace during image transfer.
>
> 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 allows required images to be provisioned by
> the platform.
>
> This change only affects devices matched on the SAHARA MHI channel and
> does not change behavior for existing AIC100 and AIC200 devices.

[Severity: Medium]
Is this statement accurate?

Since fw_folder is populated for existing AIC100 devices (for example, in
drivers/accel/qaic/mhi_controller.c where aic100_sahara_fw sets
fw_folder = "aic100"), the new fallback lookup in sahara_find_image() will
also apply to them. If an AIC100 target requests an unspecified image ID, it
will now trigger a filesystem lookup instead of being immediately rejected.

Does this unintentionally change the behavior for existing devices?

> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index e339c67e236af..7fe5158c17e98 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
[ ... ]
> @@ -201,8 +202,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);

[Severity: Medium]
Could this unintentionally execute for AIC100 and AIC200 devices?

Because aic100_sahara_fw sets .fw_folder = "aic100", context->fw_folder will
be valid for these devices. An unknown image_id request from an AIC100 device
will fall through to this path and perform a filesystem lookup instead of
returning -EINVAL immediately.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-sahara_protocol_new_v2-v7-0-90d278bd7414@oss.qualcomm.com?part=4