On 1/22/2020 12:41 PM, Corentin Labbe wrote: > On Tue, Jan 21, 2020 at 02:20:27PM +0000, Iuliana Prodan wrote: >> On 1/21/2020 12:00 PM, Corentin Labbe wrote: >>> On Tue, Jan 21, 2020 at 01:32:29AM +0200, Iuliana Prodan wrote: >>>> Added support for executing multiple requests, in parallel, >>>> for crypto engine. >>>> A no_reqs is initialized and set in the new >>>> crypto_engine_alloc_init_and_set function. >>>> >>>> >>> >>> Hello >>> >>> In your model, who is running finalize_request() ? >> finalize_request() in CAAM, and in other drivers, is called on the _done >> callback (stm32, virtio and omap). >> >>> In caam it seems that you have a taskqueue dedicated for that but you cannot assume that all drivers will have this. >>> I think the crypto_engine should be sufficient by itself and does not need external thread/taskqueue. >>> >>> But in your case, it seems that you dont have the choice, since do_one_request does not "do" but simply enqueue the request in the "jobring". >>> >> But, do_one_request it shouldn't, necessary, execute the request. Is ok >> to enqueue it, since we have asynchronous requests. do_one_request is >> not blocking. >> >>> What about adding along prepare/do_one_request/unprepare a new enqueue()/can_do_more() function ? >>> >>> The stream will be: >>> retry: >>> optionnal prepare >>> optionnal enqueue >>> optionnal can_do_more() (goto retry) >>> optionnal do_one_request >>> >>> then >>> finalize() >>> optionnal unprepare >>> >> >> I'm planning to improve crypto-engine incrementally, so I'm taking one >> step at a time :) >> But I'm not sure if adding an enqueue operation is a good idea, since, >> my understanding, is that do_one_request is a non-blocking operation and >> it shouldn't execute the request. > > do_one_request is a blocking operation on amlogic/sun8i-ce/sun8i-ss and the "documentation" is clear "@do_one_request: do encryption for current request". > But I agree that is a bit small for a documentation. > Herbert, Baolin, What do you think about do_one_requet operation: is blocking or not? There are several drivers (stm32, omap, virtio, caam) that include crypto-engine, and uses do_one_request as non-blocking, only the ones mentioned and implemented by Corentin use do_one_request as blocking. >> >> IMO, the crypto-engine flow should be kept simple: >> 1. a request comes to hw -> this is doing transfer_request_to_engine; >> 2. CE enqueue the requests >> 3. on pump_requests: >> 3. a) optional prepare operation >> 3. b) sends the reqs to hw, by do_one_request operation. To wait for >> completion here it contradicts the asynchronous crypto API. > > There are no contradiction, the call is asynchronous for the user of the API. > >> do_one_request operation has a crypto_async_request type as argument. >> Note: Step 3. b) can be done several times, depending on size of hw queue. >> 4. in driver, when req is done: >> 4. a) optional unprepare operation >> 4. b) crypto_finalize_request is called >> > > Since Herbert say the same thing than me: > "Instead, we should just let the driver tell us when it is ready to accept more requests." > Let me insist on my proposal, I have updated my serie, and it should handle your case and mine. > I will send it within minutes. > Corentin, In your new proposal, a few patches include my modifications. The others include a solution that fits your drivers very well, but implies modifications in all the other 4 drivers. It's not backwards compatible. I believe it can be done better, so we won't need to modify, _at all_, the other drivers. I'm working on a new version for my RFC, that has the can_enqueue_more, as Herbert suggested, but I would really want to know how crypto-engine's do_one_request was thought: blocking or non-blocking? Just your driver(s) use it as blocking, the other examples use it to enqueue (don't block in waiting for request to finish). Thanks, Iulia
