himci: finish the request when the FIFO reset times out - #59
Conversation
Same one-line fault as #57 on hisilicon-hi3516cv500, in this branch's copy of the driver. himci_request() has ten ways out and nine of them reach request_end, which calls himci_finish_request() and through it mmc_request_done(). The tenth, in the FIFO-reset loop on the data path, is a bare return. Nothing else completes the request. The MMC core is sitting in mmc_wait_for_req() on an uninterruptible wait_for_completion(), so the caller becomes an unkillable D state, and because the host is never released every later request queues behind it. On a camera that is recording stopping and staying stopped until someone cuts the power. Set the data error the way every other timeout in this driver does and jump to request_end. There is no DMA to unwind: himci_idma_start() is below this point. Found by reading, not by triggering it; see #57 for why forcing the path costs a board nobody can reach. The path leaves "fifo reset is timeout!" in dmesg, which is what to grep for if it is ever seen in the field. Compile-tested with hi3516av100 and hi3516dv100' kernel config (CONFIG_HIMCI=y, arm-openipc-linux-musleabi, make drivers/mmc/host/himci/himci.o) with no new warnings.
PR Summary by Qodohimci: Complete requests after FIFO reset timeout
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
From review of the previous commit, and a fault in it. himci_setup_data() does two things that have to be undone: it maps the scatterlist for the device, and it takes host->data. himci_data_done() is the only thing that gives either back. Jumping to request_end from the FIFO-reset timeout skipped it, so the buffer went back to the MMC core still mapped for the device, and host->data was left pointing at a request that had just been completed. Call it, with no status bits, so it unmaps, clears host->data, zeroes bytes_xfered and keeps the -ETIMEDOUT set above rather than deciding an error of its own. Still no himci_idma_stop(): the engine is genuinely not running there, because himci_idma_start() is below this point. Worth noting while this is open: the command-error paths further down have the same shape. When himci_exec_cmd() fails or the command completes with an error, the data branch calls himci_idma_stop() and falls through to request_end without himci_data_done(), so those leak the mapping too. That is older than this change and is left alone here rather than folded into a one-line fix. Compile-tested as before, no new warnings.
|
Right, and it is a fault in the fix rather than in the old code. Addressed in
It now calls Worth recording while this is open, since it is the same class and I looked: Compile-tested as before, no new warnings. |
The same one-line fault as #57, in this branch's copy of
himci.c.himci_request()has ten ways out. Nine reachrequest_end, which callshimci_finish_request()and through itmmc_request_done(). The tenth — theFIFO-reset loop on the data path — is a bare
return, so nothing completes therequest: the MMC core waits forever on an uninterruptible
wait_for_completion(), the caller becomes an unkillable D state, andbecause the host is never released every later request queues behind it. On a
camera that is recording stopping until someone cuts the power.
The fix sets the data error the way every other timeout in this driver does
(
-ETIMEDOUT) and jumps torequest_end. No DMA to unwind —himci_idma_start()is below this point.
Found by reading, not by triggering. #57 explains why forcing the path costs
a board nobody can reach; the same applies here. The path leaves
fifo reset is timeout!in dmesg, which is what to grep for if it shows up inthe field.
Scope
I checked which branches actually ship this driver rather than patching every
one that contains the text.
CONFIG_HIMCI=yappears in the firmware's boardconfigs for hi3516av100 and hi3516dv100 — this branch — and for hi3516cv500 (#57). It is set nowhere
for
hisilicon-hi3516ev200orhisilicon-hi3536dv100, and neither branch evencarries a himci variant its SoC selects, so the driver cannot be built there and
those two are left alone.
Testing
Compile-tested with the hi3516av100 board kernel config (
CONFIG_HIMCI=y,arm-openipc-linux-musleabi,make drivers/mmc/host/himci/himci.o), no new warnings. No hardware for thisSoC here; the change is textually identical to the one on #57, which was
exercised on an hi3516av300 to the extent that normal SD traffic, a CMD13
passthrough and a 512-byte CMD56 read all behave unchanged.