From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7D576C433EF for ; Tue, 22 Mar 2022 13:14:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234643AbiCVNPr (ORCPT ); Tue, 22 Mar 2022 09:15:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49392 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234548AbiCVNPm (ORCPT ); Tue, 22 Mar 2022 09:15:42 -0400 Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.17.13]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C883E7657; Tue, 22 Mar 2022 06:14:11 -0700 (PDT) Received: from mail-wm1-f42.google.com ([209.85.128.42]) by mrelayeu.kundenserver.de (mreue108 [213.165.67.113]) with ESMTPSA (Nemesis) id 1MdNPq-1o5i2U0Hse-00ZMsx; Tue, 22 Mar 2022 14:14:10 +0100 Received: by mail-wm1-f42.google.com with SMTP id r190-20020a1c2bc7000000b0038a1013241dso1677405wmr.1; Tue, 22 Mar 2022 06:14:09 -0700 (PDT) X-Gm-Message-State: AOAM533cgpzCjEn8RKb+oacCxiOqclq7sNEBUv1wx13rcYhb0cCZY9Sf 6z7GdX4qWofWV4rjg0cCqO99IBmnIu4Xs/+oxWs= X-Google-Smtp-Source: ABdhPJyHDWv/mDC8WwrAR2wIUWCLvYl74QCY8Bcoo4lV+G5Q4bMm4glI5YH/lSfsRyVIlxl4wVhx+pOkDz4xDrytxAc= X-Received: by 2002:a05:600c:4e11:b0:38c:bd19:e72c with SMTP id b17-20020a05600c4e1100b0038cbd19e72cmr1599974wmq.174.1647954849655; Tue, 22 Mar 2022 06:14:09 -0700 (PDT) MIME-Version: 1.0 References: <20220321165049.35985-1-sven@svenpeter.dev> <20220321165049.35985-6-sven@svenpeter.dev> In-Reply-To: <20220321165049.35985-6-sven@svenpeter.dev> From: Arnd Bergmann Date: Tue, 22 Mar 2022 14:13:53 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 5/9] soc: apple: Add RTKit IPC library To: Sven Peter Cc: Hector Martin , Alyssa Rosenzweig , Rob Herring , Arnd Bergmann , Keith Busch , Jens Axboe , Christoph Hellwig , Sagi Grimberg , Marc Zyngier , DTML , Linux ARM , Linux Kernel Mailing List , linux-nvme@lists.infradead.org Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:2bBe4cyn3++cwdW/0rs3Hoz8DBuyuGh94rq9xa3h/Q6bApTSrfY ciBXTMWnVq9a5Xq03Pb3yMhPSS+qsMnk+uHi+2G5tNhkE+nqVZZ0cxS4jOcC+eVIaRgN5t2 LHCe8O3qi239if3Uuh3SpA4qrmYc8rYmqzxihzkknqPBGqZj7YR9hfVPXBPhV3E3R/FceQr DelYM8HEErILnBlgDObhQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:8GIhNQ+L9zA=:88M/Tj2uslOfKziF5bGUEx ndkAQDGpGYjiNfMnIPDzl+ccdtQUu1Ia4F91AK/cCCjy3QO1lb+MWNou+vrl5baSdfW9yM7Wj agfBbSfCXedENbHWA2kEwKfnhsgC/dtTO9ZYGswPyeeh4gqA6vEeXmYf3oAJ6UVI0DR6v5Hwq dBGhe1st3MftRG/HH8G/L6P5lM0Pj9b7m0+atjxjz8T7GStEJU0Qxy2/kTTPWzScmZdFzknMX S5bFy7OHoIFsscCGGwF7TO02IQJ1qKU0TPWbr1A0w6bMqtcZZjVSeLHkYYIrmmDNImInDEnYl /MxexH4G7+lg2f0sSnB0UXn0Idf0dHbfxon7hun6VBjD4JxRvMn7+8cGhEqAPfYwLeZBv+CMJ X7G0tRpILH5Gd/N/D3ORxJJVqE5McKK3u4/wjTnwupoEi/vbSiqXCfTa7CtHX1Zf/ZNSmdduT Yre8x5U0gesKT9qLFkd6lam7/1f6d7TkjH4280wPiGNuNRMZWlzqGVY/H5MDiO3jVuNl8pl65 wsT7rQLa+y8M9FGQswGpvEoSMHI18x4jM6p33//1bT5g0VaO5FPQVV/IK6GMTLG5LBZpKv28v /osGUKYzcmuhrLLwtfb3P+3LLG+W/gNzfQ3bse+zq+eGftKbD1xSpKtYuT3L+QIb3vsVY7TXi J0M0dOyxpIUaG7DEf9gMobC4SogephW1TLy3eRP3jssWj/Qhd4OPehlr2y07/lDbZnUk= Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 21, 2022 at 5:50 PM Sven Peter 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 > + > +#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. > +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 {' > +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()? > +#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. > +/* > + * 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? Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4FA1DC433F5 for ; Tue, 22 Mar 2022 13:15:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=0yZwMqcgMUreBUM/cB+7IfUQgddjw4Vyu5RB1kkh3HE=; b=nTfvlc0hVDo+Mm Mkx/Ypa/o99YKbJ5KURBvCJi7hIVXQk7LF8JIac7gmgtJfg1CrTFrgNjxVj0qrXn0mFABf7ybqIVx NJFCymnurM5G+d0c9wA11MpSsqN7/PbHGXBeVyrZ21zKwYgmILZUe8ZomJ/ezgd7Qq0c60tPxSJ42 pV0T10z2IjaVnkhEcZbRuirhQonIATYlEuEw5CzXRrRhk4woEDTEZdESs8llnian4JbtEbW1uX2vb XYnyJd2UtWu5vZzjA1lPvbg+kDD1PlYRvhrM0SEm1JpE/waNZIpnVY6JAr8qy4WX8IsR/EF0YO7DL gM6xLU4s2lxk1Lf3658A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWeLA-00B2Pd-QY; Tue, 22 Mar 2022 13:14:24 +0000 Received: from mout.kundenserver.de ([212.227.17.13]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nWeL1-00B2Nn-TE; Tue, 22 Mar 2022 13:14:17 +0000 Received: from mail-wm1-f49.google.com ([209.85.128.49]) by mrelayeu.kundenserver.de (mreue109 [213.165.67.113]) with ESMTPSA (Nemesis) id 1MD9Kp-1nNbmP2BOr-00970J; Tue, 22 Mar 2022 14:14:12 +0100 Received: by mail-wm1-f49.google.com with SMTP id p12-20020a05600c430c00b0038cbdf52227so837511wme.2; Tue, 22 Mar 2022 06:14:09 -0700 (PDT) X-Gm-Message-State: AOAM532fjWDRjwUPCEzivzr2PS0LLwsp0RvwGtjvZTBaR22C8hjpwbeN MbRuZUNON9zH1JnabgZurFXUzVNmKc7i4pwPt4k= X-Google-Smtp-Source: ABdhPJyHDWv/mDC8WwrAR2wIUWCLvYl74QCY8Bcoo4lV+G5Q4bMm4glI5YH/lSfsRyVIlxl4wVhx+pOkDz4xDrytxAc= X-Received: by 2002:a05:600c:4e11:b0:38c:bd19:e72c with SMTP id b17-20020a05600c4e1100b0038cbd19e72cmr1599974wmq.174.1647954849655; Tue, 22 Mar 2022 06:14:09 -0700 (PDT) MIME-Version: 1.0 References: <20220321165049.35985-1-sven@svenpeter.dev> <20220321165049.35985-6-sven@svenpeter.dev> In-Reply-To: <20220321165049.35985-6-sven@svenpeter.dev> From: Arnd Bergmann Date: Tue, 22 Mar 2022 14:13:53 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 5/9] soc: apple: Add RTKit IPC library To: Sven Peter Cc: Hector Martin , Alyssa Rosenzweig , Rob Herring , Arnd Bergmann , Keith Busch , Jens Axboe , Christoph Hellwig , Sagi Grimberg , Marc Zyngier , DTML , Linux ARM , Linux Kernel Mailing List , linux-nvme@lists.infradead.org X-Provags-ID: V03:K1:pod3bt9j5g3uWnj8UMfhyvaQiCycy5Eibik5to3g4wbfD62Ibdc O2a7NBxBmyQXjuhnXzkyqObStpVnj6TTh/oFJzo51j0GfYgXtyuCP6dQ0/BLEz94IrVgLS1 umW2QGPuwiuBjYsn0cx/M4mlUdRQJR0pdsY/V5EOt9Lw9tsW/5dDx1h+hNqzDE3NcZW1yXv VkwXi7RegZy4pQ6B/J6dg== X-UI-Out-Filterresults: notjunk:1;V03:K0:Zgd6k2pC7Kg=:XMg7x4qvZTC7zHvdy+8u7y us/xbk2+9EtaC7I4zwKgeaSe1oTNe0wx63aPYV9G9TxWdqhzVPVMeqyIbnfUjSxndSl2wc4e2 k8S0MqDipDotCT2C1aZTK43xdHLzuHyIeeH1XiQGyCKgmW/iZaGRP+9RrTTrpexCXsfyyAw8d B/rQzRUtL2NQrZ0AJyJelJJ92r255d5SEjPjuCVM5vmYvjZoAOn6WmPW2Bbt1GIDxX0h1/kqg Vuri+6OUoW1oA20+N7MARJ/O4DsKssGC3KaBEtcZ2NbOOoKlQTutbIE81p5goK36KZjXZaCfG UbtGKTIOBzlRioFsz5FeHfWTFodWoTorUlJfo/UpHrwbrBFc8KYItXA6NICEIrEMmVbZRsH5A 2AL/ncWpGH40Kdip+HLM3ZW3LfnmMNScuJyGZBG0XqPaheoZOpnSVq/GjV9wNDlQWBrabwH6w 17bSgNuoAFvb+PjWmmyplqVUMbDFEMptW1clEjN66Ez9IISZVkEi07ny7NRUZvyhThFOHGQsQ hVGQU19S6xgHUZX52xcA9DUc/s6QUuYCilOUtZ/fcD2rK/Ew/YWG5GRKVgiluaGJIzcfsWBO9 xxr3wogWGxqX2mV+qJMQpqlawsFSl7HuEjVNs6yPlpc5h1lvdThKYkEA7yX5fK6d1wyeR+/nL ubjpOxteyTt3HmP0Py5s1DfyESclmdkrV+CGAWQFhHfBtJrVJ+Ug7M2oBNvDEp7UeDa8ho05y AMRbW8hrkxEUGPmbSyu9hQwWKx+qwwWqPdUr4biGnvbU6Br0jWpxOs7iXK/YwWqh4tFDo1wTU uAS0e9b/w3u2IMf60QYTA+ub5hRYU8dkz0ZM9y6WMZEfNqGaiM= X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220322_061416_260798_405287ED X-CRM114-Status: GOOD ( 27.20 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Mar 21, 2022 at 5:50 PM Sven Peter 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 > + > +#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. > +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 {' > +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()? > +#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. > +/* > + * 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? Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel