Re: [PATCH 5/5] firmware: qcom: scm: Add wait-queue handling logic

[Rajendra Nayak]

On 7/14/2022 6:27 AM, Guru Das Srinagesh wrote:
> On Jul 01 2022 16:51, Rajendra Nayak wrote:
> > On 7/1/2022 4:32 PM, Rajendra Nayak wrote:
> > > On 6/28/2022 1:14 AM, Guru Das Srinagesh wrote:
> > > > Add logic to handle QCOM_SCM_WAITQ_SLEEP or QCOM_SCM_WAITQ_WAKE return
> > > > codes.
> > > >
> > > > Scenario 1: Requests made by 2 different VMs:
> > > >
> > > >     VM_1                     VM_2                            Firmware
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >       │      REQUEST_1         │                                 │
> > > >       ├────────────────────────┼─────────────────────────────────┤
> > > >       │                        │                                 │
> > > >       │                        │                              ┌──┼──┐
> > > >       │                        │                              │  │  │
> > > >       │                        │     REQUEST_2                │  │  │
> > > >       │                        ├──────────────────────────────┼──┤  │
> > > >       │                        │                              │  │  │Resource
> > > >       │                        │                              │  │  │is busy
> > > >       │                        │       {WQ_SLEEP}             │  │  │
> > > >       │                        │◄─────────────────────────────┼──┤  │
> > > >       │                        │  wq_ctx, smc_call_ctx        │  │  │
> > > >       │                        │                              └──┼──┘
> > > >       │   REQUEST_1 COMPLETE   │                                 │
> > > >       │◄───────────────────────┼─────────────────────────────────┤
> > > >       │                        │                                 │
> > > >       │                        │         IRQ                     │
> > > >       │                        │◄─-------------------------------│
> > > >       │                        │                                 │
> > > >       │                        │      get_wq_ctx()               │
> > > >       │                        ├────────────────────────────────►│
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >       │                        │◄────────────────────────────────┤
> > > >       │                        │   wq_ctx, flags, and            │
> > > >       │                        │        more_pending             │
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >       │                        │ wq_resume(smc_call_ctx)         │
> > > >       │                        ├────────────────────────────────►│
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >       │                        │      REQUEST_2 COMPLETE         │
> > > >       │                        │◄────────────────────────────────┤
> > > >       │                        │                                 │
> > > >       │                        │                                 │
> > > >
> > > > Scenario 2: Two Requests coming in from same VM:
> > > >
> > > >     VM_1                                                     Firmware
> > > >       │                                                          │
> > > >       │                                                          │
> > > >       │                                                          │
> > > >       │                                                          │
> > > >       │      REQUEST_1                                           │
> > > >       ├──────────────────────────────────────────────────────────┤
> > > >       │                                                          │
> > > >       │                                                     ┌────┼───┐
> > > >       │                                                     │    │   │
> > > >       │                                                     │    │   │
> > > >       │                                                     │    │   │
> > > >       │      REQUEST_2                                      │    │   │
> > > >       ├─────────────────────────────────────────────────────┼───►│   │
> > > >       │                                                     │    │   │Resource
> > > >       │                                                     │    │   │is busy
> > > >       │      {WQ_SLEEP}                                     │    │   │
> > > >       │◄────────────────────────────────────────────────────┼────┤   │
> > > >       │      wq_ctx, req2_smc_call_ctx                      │    │   │
> > > >       │                                                     │    │   │
> > > >       │                                                     └────┼───┘
> > > >       │                                                          │
> > > >       │      {WQ_WAKE}                                           │
> > > >       │◄─────────────────────────────────────────────────────────┤
> > > >       │      wq_ctx, req1_smc_call_ctx, flags                    │
> > >
> > >
> > > This is perhaps the same thing I asked on the previous patch,
> > > I am guessing {WQ_WAKE} is returned in respone to REQUEST_1?
> > > How do you know in this case if REQUEST_1 was a success or failure?
> >
> > Ok looking at this some more, I think what we are saying is that the FW returns
> > {WQ_WAKE} to REQUEST_1, we then call wq_wake_ack and the return of
> > *that* will tell if REQUEST_1 was success or failure?
> > Did I get it right?
>
> Yes, that is correct. I should have added an explanatory note in the commit
> message to this effect:
>
>
>       │      {WQ_WAKE}                         <-- Return value  │
>       │◄─────────────────────────────────────────────────────────┤
>       │      wq_ctx, req1_smc_call_ctx, flags  <-- Its payload   │
>
> What this means is that the WQ_WAKE is sent by the FW to VM1 (direction of
> arrow is from right to left) and that the additional data packed as payload
> indicate that it is meant for REQUEST_1 (`req1_smc_call_ctx`).
>
> Hopefully this will help understand the diagram better.

Ok thanks for the explanation, I actually had a few more comments down in that
patch which you did not answer, can you clarify them too?

> > +        } else if ((long)res->a0 < 0) {
> > +            /* Error, simply return to caller */
> > +            break;

if my understanding above is correct, shouldn't we do a
> > +            if (wq)
> > +                scm_waitq_flag_handler(wq, flags);
in the error case also?

Also why no just scm_waitq_flag_handler(wq, flags); before fill_wq_wake_ack_args(smc, smc_call_ctx);?

> Thank you.
>
> Guru Das.