On Tue, Mar 22, 2022, at 14:13, Arnd Bergmann wrote:
> On Mon, Mar 21, 2022 at 5:50 PM Sven Peter <sven@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
>> +
>> +#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
