| 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
| ||
|
Message-ID: <CAPaKu7RUx6KHyvdvrfX3u-7Lk=Wa3nmTh6-tD3CbReNAwNtgoQ@mail.gmail.com>
Date: Thu, 11 Sep 2025 21:48:17 -0700
From: Chia-I Wu <olvaffe@...il.com>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Boris Brezillon <boris.brezillon@...labora.com>, Steven Price <steven.price@....com>,
Liviu Dudau <liviu.dudau@....com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>, MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>, Chanwoo Choi <cw00.choi@...sung.com>,
Jassi Brar <jassisinghbrar@...il.com>, Kees Cook <kees@...nel.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>, Chen-Yu Tsai <wenst@...omium.org>, kernel@...labora.com,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-pm@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH RFC 05/10] mailbox: add MediaTek GPUEB IPI mailbox
On Fri, Sep 5, 2025 at 3:24 AM Nicolas Frattaroli
<nicolas.frattaroli@...labora.com> wrote:
>
> The MT8196 SoC uses an embedded MCU to control frequencies and power of
> the GPU. This controller is referred to as "GPUEB".
>
> It communicates to the application processor, among other ways, through
> a mailbox.
>
> The mailbox exposes one interrupt, which appears to only be fired when a
> response is received, rather than a transaction is completed. For us,
> this means we unfortunately need to poll for txdone.
>
> The mailbox also requires the EB clock to be on when touching any of the
> mailbox registers.
>
> Add a simple driver for it based on the common mailbox framework.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> ---
> drivers/mailbox/Kconfig | 10 ++
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/mtk-gpueb-mailbox.c | 330 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 342 insertions(+)
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 02432d4a5ccd46a16156a09c7f277fb03e4013f5..2016defda1fabb5c0fcc8078f84a52d4e4e00167 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -294,6 +294,16 @@ config MTK_CMDQ_MBOX
> critical time limitation, such as updating display configuration
> during the vblank.
>
> +config MTK_GPUEB_MBOX
> + tristate "MediaTek GPUEB Mailbox Support"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> + The MediaTek GPUEB mailbox is used to communicate with the embedded
> + controller in charge of GPU frequency and power management on some
> + MediaTek SoCs, such as the MT8196.
> + Say Y or m here if you want to support the MT8196 SoC in your kernel
> + build.
> +
> config ZYNQMP_IPI_MBOX
> tristate "Xilinx ZynqMP IPI Mailbox"
> depends on ARCH_ZYNQMP && OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 98a68f838486eed117d24296138bf59fda3f92e4..564d06e71313e6d1972e4a6036e1e78c8c7ec450 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -63,6 +63,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX) += mtk-adsp-mailbox.o
>
> obj-$(CONFIG_MTK_CMDQ_MBOX) += mtk-cmdq-mailbox.o
>
> +obj-$(CONFIG_MTK_GPUEB_MBOX) += mtk-gpueb-mailbox.o
> +
> obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
>
> obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
> diff --git a/drivers/mailbox/mtk-gpueb-mailbox.c b/drivers/mailbox/mtk-gpueb-mailbox.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..0236fb358136e434a09a21ef293fe949ced94123
> --- /dev/null
> +++ b/drivers/mailbox/mtk-gpueb-mailbox.c
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MediaTek GPUEB mailbox driver for SoCs such as the MT8196
> + *
> + * Copyright (C) 2025, Collabora Ltd.
> + *
> + * Developers harmed in the making of this driver:
> + * - Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#define MBOX_CTL_TX_STS 0x0000
> +#define MBOX_CTL_IRQ_SET 0x0004
> +#define MBOX_CTL_IRQ_CLR 0x0074
> +#define MBOX_CTL_RX_STS 0x0078
> +
> +#define MBOX_FULL BIT(0) /* i.e. we've received data */
> +#define MBOX_CLOGGED BIT(1) /* i.e. the channel is shutdown */
> +
> +struct mtk_gpueb_mbox {
> + struct device *dev;
> + struct clk *clk;
> + void __iomem *mbox_mmio;
> + void __iomem *mbox_ctl;
> + void **rx_buf;
> + atomic_t *rx_status;
> + struct mbox_controller mbox;
> + unsigned int *chn;
> + int irq;
> + const struct mtk_gpueb_mbox_variant *v;
> +};
> +
> +struct mtk_gpueb_mbox_ch {
> + const char *name;
> + const int num;
> + const unsigned int tx_offset;
> + const unsigned int tx_len;
> + const unsigned int rx_offset;
> + const unsigned int rx_len;
> + const bool no_response;
> +};
> +
> +struct mtk_gpueb_mbox_variant {
> + unsigned int num_channels;
> + const struct mtk_gpueb_mbox_ch channels[] __counted_by(num_channels);
> +};
> +
> +/**
> + * mtk_gpueb_mbox_read_rx - read RX buffer from MMIO into ebm's RX buffer
> + * @ebm: pointer to &struct mtk_gpueb_mbox instance
> + * @channel: number of channel to read
> + */
> +static void mtk_gpueb_mbox_read_rx(struct mtk_gpueb_mbox *ebm,
> + unsigned int channel)
> +{
> + const struct mtk_gpueb_mbox_ch *ch;
> +
> + ch = &ebm->v->channels[channel];
> +
> + memcpy_fromio(ebm->rx_buf[channel], ebm->mbox_mmio + ch->rx_offset,
> + ch->rx_len);
> +
> +}
> +
> +static irqreturn_t mtk_gpueb_mbox_isr(int irq, void *data)
> +{
> + struct mtk_gpueb_mbox *ebm = data;
> + u32 rx_handled = 0;
> + u32 rx_sts;
> + int i;
> +
> + rx_sts = readl(ebm->mbox_ctl + MBOX_CTL_RX_STS);
> +
> + for (i = 0; i < ebm->v->num_channels; i++) {
> + if (rx_sts & BIT(i)) {
> + if (!atomic_cmpxchg(&ebm->rx_status[i], 0,
> + MBOX_FULL | MBOX_CLOGGED))
> + rx_handled |= BIT(i);
> + }
> + }
We can loop over bits that are set in rx_sts, if we expect that only a
few bits are set most of the time.
> +
> + writel(rx_handled, ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> +
> + if (!(rx_sts ^ rx_handled))
"rx_sts == rx_handled" should be more direct.
> + return IRQ_WAKE_THREAD;
> +
> + dev_warn_ratelimited(ebm->dev, "spurious interrupts on 0x%04X\n",
> + rx_sts ^ rx_handled);
> + return IRQ_NONE;
It seems a bit too punishing when there are spurious interrupts. I
wonder if we should warn, but return IRQ_WAKE_THREAD as long as
rx_handled != 0.
Also, if another interrupt can fire before mtk_gpueb_mbox_thread runs,
that's data dropping rather than spurious interrupts.
> +}
> +
> +static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
> +{
> + struct mtk_gpueb_mbox *ebm = data;
> + irqreturn_t ret = IRQ_NONE;
> + int status;
> + int i;
> +
> + for (i = 0; i < ebm->v->num_channels; i++) {
> + status = atomic_cmpxchg(&ebm->rx_status[i],
> + MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
> + if (status == (MBOX_FULL | MBOX_CLOGGED)) {
We could also save rx_handled from mtk_gpueb_mbox_isr and loop over
bits that are set. If we do that, ebm->rx_status[i] is guaranteed to
be MBOX_FULL | MBOX_CLOGGED.
> + mtk_gpueb_mbox_read_rx(ebm, i);
> + mbox_chan_received_data(&ebm->mbox.chans[i],
> + ebm->rx_buf[i]);
It looks like we read the data and pass it on to the client
immediately. Does each channel need its own rx_buf?
> + /* FIXME: When does MBOX_FULL get cleared? Here? */
> + atomic_set(&ebm->rx_status[i], 0);
> + ret = IRQ_HANDLED;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> + unsigned int *num = chan->con_priv;
> + int i;
> + u32 *values = data;
> +
> + if (*num >= ebm->v->num_channels)
> + return -ECHRNG;
Can this ever happen? (I am not familiar with the mbox subsystem)
> +
> + if (!ebm->v->channels[*num].no_response &&
> + atomic_read(&ebm->rx_status[*num]))
> + return -EBUSY;
When no_response is true, rx_status is 0. We probably don't need to
check no_response.
> +
> + writel(BIT(*num), ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> +
> + /*
> + * We don't want any fancy nonsense, just write the 32-bit values in
> + * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> + */
> + for (i = 0; i < ebm->v->channels[*num].tx_len; i += 4) {
> + writel(values[i / 4],
> + ebm->mbox_mmio + ebm->v->channels[*num].tx_offset + i);
> + }
> +
> + writel(BIT(*num), ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
> +
> + return 0;
> +}
> +
> +static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
> +{
> + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> + unsigned int *num = chan->con_priv;
> +
> + if (*num >= ebm->v->num_channels)
> + return -ECHRNG;
> +
> + atomic_set(&ebm->rx_status[*num], 0);
> +
> + return 0;
> +}
> +
> +static void mtk_gpueb_mbox_shutdown(struct mbox_chan *chan)
> +{
> + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> + unsigned int *num = chan->con_priv;
> +
> + atomic_set(&ebm->rx_status[*num], MBOX_CLOGGED);
> +}
> +
> +static bool mtk_gpueb_mbox_last_tx_done(struct mbox_chan *chan)
> +{
> + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(chan->mbox->dev);
> + unsigned int *num = chan->con_priv;
> +
> + return !(readl(ebm->mbox_ctl + MBOX_CTL_TX_STS) & BIT(*num));
> +}
> +
> +const struct mbox_chan_ops mtk_gpueb_mbox_ops = {
> + .send_data = mtk_gpueb_mbox_send_data,
> + .startup = mtk_gpueb_mbox_startup,
> + .shutdown = mtk_gpueb_mbox_shutdown,
> + .last_tx_done = mtk_gpueb_mbox_last_tx_done,
> +};
> +
> +static struct mbox_chan *
> +mtk_gpueb_mbox_of_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *sp)
> +{
> + struct mtk_gpueb_mbox *ebm = dev_get_drvdata(mbox->dev);
> +
> + if (!sp->args_count)
> + return ERR_PTR(-EINVAL);
> +
> + if (sp->args[0] >= ebm->v->num_channels)
> + return ERR_PTR(-ECHRNG);
> +
> + return &mbox->chans[sp->args[0]];
> +}
> +
> +static int mtk_gpueb_mbox_probe(struct platform_device *pdev)
> +{
> + struct mtk_gpueb_mbox *ebm;
> + unsigned int rx_buf_sz;
> + void *buf;
> + unsigned int i;
> + int ret;
> +
> + ebm = devm_kzalloc(&pdev->dev, sizeof(*ebm), GFP_KERNEL);
> + if (!ebm)
> + return -ENOMEM;
> +
> + ebm->dev = &pdev->dev;
> + ebm->v = of_device_get_match_data(ebm->dev);
> +
> + dev_set_drvdata(ebm->dev, ebm);
> +
> + ebm->clk = devm_clk_get_enabled(ebm->dev, NULL);
> + if (IS_ERR(ebm->clk))
> + return dev_err_probe(ebm->dev, PTR_ERR(ebm->clk),
> + "Failed to get 'eb' clock\n");
> +
> + ebm->mbox_mmio = devm_platform_ioremap_resource_byname(pdev, "mbox");
> + if (IS_ERR(ebm->mbox_mmio))
> + return dev_err_probe(ebm->dev, PTR_ERR(ebm->mbox_mmio),
> + "Couldn't map mailbox registers\n");
> +
> + ebm->mbox_ctl = devm_platform_ioremap_resource_byname(pdev, "mbox_ctl");
> + if (IS_ERR(ebm->mbox_ctl))
> + return dev_err_probe(
> + ebm->dev, PTR_ERR(ebm->mbox_ctl),
> + "Couldn't map mailbox control registers\n");
> +
> + rx_buf_sz = (ebm->v->channels[ebm->v->num_channels - 1].rx_offset +
> + ebm->v->channels[ebm->v->num_channels - 1].rx_len);
rx is after tx in mmio. The first half of the space is wasted.
We follow mtk_gpueb_mbox_read_rx by mbox_chan_received_data. It seems
we only need max of rx_len's.
> +
> + buf = devm_kzalloc(ebm->dev, rx_buf_sz, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ebm->rx_buf = devm_kmalloc_array(ebm->dev, ebm->v->num_channels,
> + sizeof(*ebm->rx_buf), GFP_KERNEL);
> + if (!ebm->rx_buf)
> + return -ENOMEM;
> +
> + ebm->mbox.chans = devm_kcalloc(ebm->dev, ebm->v->num_channels,
> + sizeof(struct mbox_chan), GFP_KERNEL);
> + if (!ebm->mbox.chans)
> + return -ENOMEM;
> +
> + ebm->rx_status = devm_kcalloc(ebm->dev, ebm->v->num_channels,
> + sizeof(atomic_t), GFP_KERNEL);
> + if (!ebm->rx_status)
> + return -ENOMEM;
> +
> + ebm->chn = devm_kcalloc(ebm->dev, ebm->v->num_channels,
> + sizeof(*ebm->chn), GFP_KERNEL);
> +
> + for (i = 0; i < ebm->v->num_channels; i++) {
> + ebm->rx_buf[i] = buf + ebm->v->channels[i].rx_offset;
> + spin_lock_init(&ebm->mbox.chans[i].lock);
> + /* the things you do to avoid explicit casting void* */
I actually prefer an inline helper that casts chan->con_priv to the
channel number. Another option is "chan - ebm->mox.chans".
> + ebm->chn[i] = i;
> + ebm->mbox.chans[i].con_priv = &ebm->chn[i];
> + atomic_set(&ebm->rx_status[i], MBOX_CLOGGED);
> + }
> +
> + ebm->mbox.dev = ebm->dev;
> + ebm->mbox.num_chans = ebm->v->num_channels;
> + ebm->mbox.txdone_poll = true;
> + ebm->mbox.txpoll_period = 0; /* minimum hrtimer interval */
> + ebm->mbox.of_xlate = mtk_gpueb_mbox_of_xlate;
> + ebm->mbox.ops = &mtk_gpueb_mbox_ops;
> +
> + ebm->irq = platform_get_irq(pdev, 0);
> + if (ebm->irq < 0)
> + return ebm->irq;
> +
> + ret = devm_request_threaded_irq(ebm->dev, ebm->irq, mtk_gpueb_mbox_isr,
> + mtk_gpueb_mbox_thread, 0, NULL, ebm);
> + if (ret)
> + return dev_err_probe(ebm->dev, ret, "failed to request IRQ\n");
> +
> + ret = devm_mbox_controller_register(ebm->dev, &ebm->mbox);
> +
> + return 0;
> +}
> +
> +static const struct mtk_gpueb_mbox_variant mtk_gpueb_mbox_mt8196 = {
> + .num_channels = 12,
> + .channels = {
> + { "fast_dvfs_event", 0, 0x0000, 16, 0x00e0, 16, false },
> + { "gpufreq", 1, 0x0010, 32, 0x00f0, 32, false },
> + { "sleep", 2, 0x0030, 12, 0x0110, 4, true },
> + { "timer", 3, 0x003c, 24, 0x0114, 4, false },
> + { "fhctl", 4, 0x0054, 36, 0x0118, 4, false },
> + { "ccf", 5, 0x0078, 16, 0x011c, 16, false },
> + { "gpumpu", 6, 0x0088, 24, 0x012c, 4, false },
> + { "fast_dvfs", 7, 0x00a0, 24, 0x0130, 24, false },
> + { "ipir_c_met", 8, 0x00b8, 4, 0x0148, 16, false },
> + { "ipis_c_met", 9, 0x00bc, 16, 0x0158, 4, false },
> + { "brisket", 10, 0x00cc, 16, 0x015c, 16, false },
> + { "ppb", 11, 0x00dc, 4, 0x016c, 4, false },
> + },
> +};
> +
> +static const struct of_device_id mtk_gpueb_mbox_of_ids[] = {
> + { .compatible = "mediatek,mt8196-gpueb-mbox",
> + .data = &mtk_gpueb_mbox_mt8196 },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_gpueb_mbox_of_ids);
> +
> +static struct platform_driver mtk_gpueb_mbox_drv = {
> + .probe = mtk_gpueb_mbox_probe,
> + .driver = {
> + .name = "mtk-gpueb-mbox",
> + .of_match_table = mtk_gpueb_mbox_of_ids,
> + }
> +};
> +module_platform_driver(mtk_gpueb_mbox_drv);
> +
> +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@...labora.com>");
> +MODULE_DESCRIPTION("MediaTek GPUEB mailbox driver for SoCs such as the MT8196");
> +MODULE_LICENSE("GPL");
>
> --
> 2.51.0
>
Powered by blists - more mailing lists