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 <[email protected]> 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-90d278bd7...@oss.qualcomm.com?part=6
