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

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

On Tue, Mar 22, 2022, at 14:13, Arnd Bergmann wrote:
> On Mon, Mar 21, 2022 at 5:50 PM Sven Peter <sven@svenpeter.dev> wrote:
>>
>> Apple SoCs such as the M1 come with multiple embedded co-processors
>> running proprietary firmware. Communication with those is established
>> over a simple mailbox using the RTKit IPC protocol.
>>
>> Signed-off-by: Sven Peter <sven@svenpeter.dev>
>
>> +
>> +#define rtk_err(format, arg...) dev_err(rtk->dev, "RTKit: " format, ##arg)
>> +#define rtk_warn(format, arg...) dev_warn(rtk->dev, "RTKit: " format, ##arg)
>> +#define rtk_info(format, arg...) dev_info(rtk->dev, "RTKit: " format, ##arg)
>> +#define rtk_dbg(format, arg...) dev_dbg(rtk->dev, "RTKit: " format, ##arg)
>
> I generally don't like the custom printing macros, please just open-code
> the prints where they are used, that makes it easier for other kernel
> developers to see exactly what is being printed.
>

Sure, I'll remove them.

>> +enum { APPLE_RTKIT_WORK_MSG,
>> +       APPLE_RTKIT_WORK_REINIT,
>> +};
>> +
>> +enum { APPLE_RTKIT_PWR_STATE_OFF = 0x00,
>> +       APPLE_RTKIT_PWR_STATE_SLEEP = 0x01,
>> +       APPLE_RTKIT_PWR_STATE_GATED = 0x02,
>> +       APPLE_RTKIT_PWR_STATE_QUIESCED = 0x10,
>> +       APPLE_RTKIT_PWR_STATE_ON = 0x20,
>> +};
>
> This is an odd indentation style, I would insert a newline after the 'enum {'

Yeah, I blame clang-format and me not double-checking the result for that one.
I'll add the newline.

>
>> +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.


>
>> +#if IS_ENABLED(CONFIG_APPLE_RTKIT)
>
> Instead of allowing the interface to be used without CONFIG_APPLE_RTKIT,
> I think it is sufficient to allow the driver itself to be built with
> CONFIG_COMPILE_TEST (as you already do), and then have
> drivers using it marked as 'depends on APPLE_RTKIT'
> unconditionally.

That's indeed much simpler and what I wanted to achieve. I just couldn't
figure out how to do it. I'll use your approach! 

>
>> +/*
>> + * Initializes the internal state required to handle RTKit. This
>> + * should usually be called within _probe.
>> + *
>> + * @dev: Pointer to the device node this coprocessor is assocated with
>> + * @cookie: opaque cookie passed to all functions defined in rtkit_ops
>> + * @mbox_name: mailbox name used to communicate with the co-processor
>> + * @mbox_idx: mailbox index to be used if mbox_name is NULL
>> + * @ops: pointer to rtkit_ops to be used for this co-processor
>> + */
>> +struct apple_rtkit *apple_rtkit_init(struct device *dev, void *cookie,
>> +                                    const char *mbox_name, int mbox_idx,
>> +                                    const struct apple_rtkit_ops *ops);
>> +
>> +/*
>> + * Dev-res managed version of apple_rtkit_init.
>> + */
>> +struct apple_rtkit *devm_apple_rtkit_init(struct device *dev, void *cookie,
>> +                                         const char *mbox_name, int mbox_idx,
>> +                                         const struct apple_rtkit_ops *ops);
>
> Do we need to export both of these?

No, only devm_apple_rtkit_init needs to be exported.


Thanks,


Sven