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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 03 Apr 2022 12:45:21 +0200
From:   "Sven Peter" <sven@...npeter.dev>
To:     "Arnd Bergmann" <arnd@...db.de>
Cc:     "Hector Martin" <marcan@...can.st>,
        "Alyssa Rosenzweig" <alyssa@...enzweig.io>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Keith Busch" <kbusch@...nel.org>, "axboe@...com" <axboe@...com>,
        "hch@....de" <hch@....de>, "sagi@...mberg.me" <sagi@...mberg.me>,
        "Marc Zyngier" <maz@...nel.org>, DTML <devicetree@...r.kernel.org>,
        "Linux ARM" <linux-arm-kernel@...ts.infradead.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        linux-nvme@...ts.infradead.org
Subject: Re: [PATCH 5/9] soc: apple: Add RTKit IPC library



On Sat, Apr 2, 2022, at 20:30, Arnd Bergmann wrote:
> On Sat, Apr 2, 2022 at 2:56 PM Sven Peter <sven@...npeter.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