Re: [PATCH 5/9] soc: apple: Add RTKit IPC library

["Sven Peter" <sven@svenpeter.dev>]

On Sat, Apr 2, 2022, at 20:30, Arnd Bergmann wrote:
> On Sat, Apr 2, 2022 at 2:56 PM Sven Peter <sven@svenpeter.dev> wrote:
>> On Tue, Mar 22, 2022, at 14:13, Arnd Bergmann wrote:
>> >> +static int apple_rtkit_worker(void *data)
>> >> +{
>> >> +       struct apple_rtkit *rtk = data;
>> >> +       struct apple_rtkit_work work;
>> >> +
>> >> +       while (!kthread_should_stop()) {
>> >> +               wait_event_interruptible(rtk->wq,
>> >> +                                        kfifo_len(&rtk->work_fifo) > 0 ||
>> >> +                                                kthread_should_stop());
>> >> +
>> >> +               if (kthread_should_stop())
>> >> +                       break;
>> >> +
>> >> +               while (kfifo_out_spinlocked(&rtk->work_fifo, &work, 1,
>> >> +                                           &rtk->work_lock) == 1) {
>> >> +                       switch (work.type) {
>> >> +                       case APPLE_RTKIT_WORK_MSG:
>> >> +                               apple_rtkit_rx(rtk, &work.msg);
>> >> +                               break;
>> >> +                       case APPLE_RTKIT_WORK_REINIT:
>> >> +                               apple_rtkit_do_reinit(rtk);
>> >> +                               break;
>> >> +                       }
>> >> +               }
>> >
>> > It looks like you add quite a bit of complexity by using a custom
>> > worker thread implementation. Can you explain what this is
>> > needed for? Isn't this roughly the same thing that one would
>> > get more easily with create_singlethread_workqueue()?
>>
>> I originally had just a workqueue here but I can only put
>> one instance of e.g. APPLE_RTKIT_WORK_MSG onto these.
>> There could however be a new incoming message while the previous
>> one is still being handled and I couldn't figure out a way
>> to handle that with workqueues without introducing a race.
>
> Are you trying to avoid dynamic allocation of the messages then
> and have no other place that you can embed it in?

Yeah, I didn't want to allocate anything because of the added
complexity here like you mentioned.

>
> If you kmalloc() a messages that embeds a work_struct, you can
> enqueue as many of those as you want, but the allocation adds
> complexity through the need for error handling etc.
>
> I wonder if you can change the mailbox driver to use a threaded
> irq handler, which I think should ensure that the callback here
> is run in process context, avoiding the need to defer execution
> within the rtkit driver.

Hrm, I just realized that's already the case. mailbox_client.h documents
rx_callback as "Atomic callback to provide client the data received"
but since we know rtkit will only ever be combined with that specific
Apple mailbox we could get away with just ignoring that.
It's a small hack but it might be worth it since it would reduce
the complexity inside rtkit.

Sven