Re: [PATCH v7 6/7] bus: mhi: Capture DDR training data via command mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] bus: mhi: sahara: sleep in atomic context in MHI DL callback
- [High] bus: mhi: sahara: DMA buffer tx[0] overwritten by back-to-back commands
- [High] bus: mhi: sahara: data race on training_nbuf causes dropped training data
- [High] bus: mhi: sahara: RX buffer starvation for payloads exceeding SAHARA_NUM_CMD_BUF
- [High] bus: mhi: sahara: Use-After-Free on driver unbind due to workqueue scheduling
- [Medium] bus: mhi: sahara: kzalloc failure leaves device state desynchronized
- [Low] bus: mhi: sahara: unsigned comparison < 0 is always false
--

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

bus: mhi: Capture DDR training data via command mode

During early boot, devices may perform DDR training and produce training
data that can be reused on subsequent boots to reduce initialization
time. The Sahara protocol provides a command mode flow to transfer this
training data to the host, but the driver currently does not handle
command mode and drops the training payload.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index 4606950555721..11f65cbfa66b8 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
[ ... ]
> @@ -335,6 +426,153 @@ static void sahara_hello(struct sahara_context *context)
[ ... ]
> +static void sahara_command_execute(struct sahara_context *context, u32 client_command)
> +{
> +	int ret;
> +
> +	context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_CMD);
> +	context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXECUTE_LENGTH);
> +	context->tx[0]->command_execute.client_command = cpu_to_le32(client_command);
> +
> +	ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
> +			    SAHARA_COMMAND_EXECUTE_LENGTH, MHI_EOT);
[ ... ]
> +static void sahara_command_execute_data(struct sahara_context *context, u32 client_command)
> +{
> +	int ret;
> +
> +	context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_DATA_CMD);
> +	context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXEC_DATA_LENGTH);
> +	context->tx[0]->command_exec_data.client_command = cpu_to_le32(client_command);
> +
> +	ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
> +			    SAHARA_COMMAND_EXEC_DATA_LENGTH, MHI_EOT);
[ ... ]
> +static void sahara_command_execute_resp(struct sahara_context *context)
> +{
> +	struct device *dev = &context->mhi_dev->mhi_cntrl->mhi_dev->dev;
> +	struct sahara_cntrl_training_data *ct;
> +	u32 client_cmd, resp_len;
> +	int ret;
> +	u64 remaining;
> +	u32 i;
> +
> +	if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
> +	    le32_to_cpu(context->rx->command_execute_resp.response_length) < 0) {

[Severity: Low]
Since le32_to_cpu() returns an unsigned 32-bit integer, will this check
for < 0 ever evaluate to true, or is it dead code?

> +		dev_err(&context->mhi_dev->dev,
> +			"Malformed command execute resp packet - length %d\n",
> +			le32_to_cpu(context->rx->length));
> +		return;
> +	}
> +
> +	client_cmd = le32_to_cpu(context->rx->command_execute_resp.client_command);
> +	resp_len = le32_to_cpu(context->rx->command_execute_resp.response_length);
> +
> +	sahara_command_execute_data(context, client_cmd);
> +
> +	if (client_cmd == SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST) {
> +		sahara_command_execute(context, SAHARA_EXEC_CMD_GET_TRAINING_DATA);

[Severity: High]
Can the second command locklessly overwrite the shared tx[0] buffer while
the first DMA transfer from sahara_command_execute_data() is still in
flight?

> +		return;
> +	}
> +
> +	if (client_cmd != SAHARA_EXEC_CMD_GET_TRAINING_DATA)
> +		return;
> +
> +	ct = sahara_cntrl_training_get(dev);
> +	if (!ct) {
> +		context->is_cmd_mode = false;
> +		sahara_switch_mode_to_img_tx(context);
> +		return;
> +	}
> +
> +	mutex_lock(&ct->lock);
> +	kfree(ct->data);
> +	ct->data = kzalloc(resp_len, GFP_KERNEL);
> +	ct->size = resp_len;
> +	ct->copied = 0;
> +	ct->receiving = true;
> +	mutex_unlock(&ct->lock);
> +
> +	if (!ct->data) {
> +		context->is_cmd_mode = false;
> +		sahara_switch_mode_to_img_tx(context);
> +		return;
> +	}

[Severity: Medium]
If this allocation fails, does returning early leave the device
transmitting raw training data (triggered earlier by
sahara_command_execute_data) that will later be misinterpreted as regular
commands?

> +
> +	context->training_size = resp_len;
> +	context->training_rcvd = 0;
> +	context->receiving_training_data = true;
> +
> +	remaining = resp_len;
> +	for (i = 0; i < SAHARA_NUM_CMD_BUF && remaining; i++) {

[Severity: High]
If the requested response length exceeds SAHARA_NUM_CMD_BUF, will the MHI
channel exhaust its RX buffers and stall since the DL callback does not
dynamically requeue more buffers to consume the remaining data?

> +		size_t pkt = min_t(size_t, remaining, SAHARA_PACKET_MAX_SIZE);
> +
> +		ret = mhi_queue_buf(context->mhi_dev, DMA_FROM_DEVICE,
> +				    context->cmd_buff[i], pkt,
> +				    (remaining <= pkt) ? MHI_EOT : MHI_CHAIN);
> +		if (ret)
> +			break;
> +
> +		remaining -= pkt;
> +	}
> +
> +	context->training_nbuf = i;

[Severity: High]
Since mhi_queue_buf() gives the hardware access to the buffer, could a
concurrent interrupt fire before training_nbuf is updated? If so, would
the callback see a stale value and drop the training data?

> +}
[ ... ]
> @@ -884,6 +1139,7 @@ static void sahara_mhi_remove(struct mhi_device *mhi_dev)
>  
>  	cancel_work_sync(&context->fw_work);
>  	cancel_work_sync(&context->dump_work);
> +	cancel_work_sync(&context->cmd_work);
>  	vfree(context->mem_dump);
>  	sahara_release_image(context);
>  	mhi_unprepare_from_transfer(mhi_dev);

[Severity: High]
If an incoming packet triggers the completion callback between cancelling
the work and shutting down the channel (via mhi_unprepare_from_transfer),
could the callback reschedule the work item which would later dereference
the freed context pointer?

>  }
>  
> @@ -900,15 +1156,71 @@ static void sahara_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>  static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>  {
>  	struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
> +	struct sahara_cntrl_training_data *ct;
[ ... ]
> +		ct = sahara_cntrl_training_get(dev);
> +		if (!ct)
> +			return;
>  
> -	if (!mhi_result->transaction_status) {
> -		context->rx_size = mhi_result->bytes_xferd;
> -		if (context->is_mem_dump_mode)
> -			schedule_work(&context->dump_work);
> -		else
> -			schedule_work(&context->fw_work);
> +		for (i = 0; i < context->training_nbuf; i++) {
> +			if (mhi_result->buf_addr == context->cmd_buff[i]) {
> +				mutex_lock(&ct->lock);

[Severity: Critical]
Is it safe to acquire this sleeping mutex in the atomic softirq context of
the MHI DL callback?

Additionally, sahara_cntrl_training_get() allocates memory with
GFP_KERNEL:

ct = devres_alloc(sahara_cntrl_training_release, sizeof(*ct), GFP_KERNEL);

Could this trigger a 'scheduling while atomic' panic upon receiving data?

> +				copy = min_t(size_t, mhi_result->bytes_xferd,
> +					     ct->size - ct->copied);
> +				memcpy((u8 *)ct->data + ct->copied,
> +				       mhi_result->buf_addr, copy);

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