On 9/20/19 4:43 AM, Kevin Wolf wrote:
> Am 19.09.2019 um 22:43 hat John Snow geschrieben:
>> I'd have to check -- because I can't say the AHCI emulator was designed
>> so much as congealed -- but you might need calls to ncq_finish.
>>
>> usually, ncq_cb handles the return from any NCQ command and will call
>> ncq_err and ncq_finish as appropriate to tidy up the command.
>>
>> It might be a mistake that execute_ncq_command issues ncq_err in the
>> `default` arm of the switch statement without a call to finish.
>>
>> If we do call ncq_finish from this context I'm not sure if we want
>> block_acct_done here unconditionally. We may not have started a block
>> accounting operation if we never started a backend operation. Everything
>> else looks about right to me.
>
> With that much uncertainty, the one thing I'm pretty certain of is that
> someone (TM) should write some qtests - if only to figure out what
> really happens.
>
For sure -- I handle the normative cases, but I don't test what happens
if you issue an unsupported NCQ command. (I don't know what real
hardware does right now, either. I'm sure I could read the spec and find
out, but don't have a testing setup that lets me analyze real hardware
anymore.)
I will have to defer this to someone (TM), but I suspect the code (that
I suspect was used as a basis for inserting a new error pathway in this
patch) is wrong and is missing a call to ncq_finish -- but that call
needs to not call the block accounting cleanup, because we didn't start
an operation in this case.
That's my Official Hunch.
--js
