On Wed, Nov 6, 2019 at 9:25 AM Tero Kristo <t-kristo@xxxxxx> wrote: > > On 06/11/2019 08:39, Gilad Ben-Yossef wrote: > > Hi, > > > > > > On Thu, Oct 17, 2019 at 3:26 PM Tero Kristo <t-kristo@xxxxxx> wrote: > >> > >> Currently crypto_wait_req waits indefinitely for an async crypto request > >> to complete. This is bad as it can cause for example the crypto test > >> manager to hang without any notification as to why it has happened. > >> Instead of waiting indefinitely, add a 1 second timeout to the call, > >> and provide a warning print if a timeout happens. > > > > While the incentive is clear and positive, this suggested solution > > creates problems of its own. > > In many (most?) cases where we are waiting here, we are waiting for a > > DMA operation to finish from hardware. > > Exiting while this pending DMA operation is not finished, even with a > > proper error return value, is dangerous because > > unless the calling code takes great care to not release the memory the > > DMA is being done from/to, this can have disastrous effects. > > > > As Eric has already mentioned, one second might seem like a long time, > > but we don't really know if it is enough. > > > > How about adding a second API (ig. crypto_wait_req_timeout) which > > supports a calee specified timeout where > > the calle knows how to correctly deal with timeout and port the > > relevant call sites to use this? > > Yeah, that would work for me. I guess we could just swap the testmgr to > use this timeout API, as it is quite clear it should timeout rather than > wait indefinitely, and afaics, the data buffers it uses are limited > size. It doesn't really matter for it whether the timeout is 1 second or > 10 seconds, as long as it eventually times out. As long as you avoid releasing the memory used on timeout, that should work well, I think. Gilad
