On Wed, Jan 22, 2020 at 12:29:22PM +0000, Iuliana Prodan wrote: > 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. Others driver should work the same, I dont see what changes they need. As I said, I tested caam with my changes, it works the same. Please show me what changes they need to continue to work and proof that they are broken with my changes. > > 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? We know that both way works, so this is not really a problem.
