| 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: <4506669.X9hSmTKtgW@workhorse>
Date: Fri, 12 Sep 2025 12:59:03 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Chia-I Wu <olvaffe@...il.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 Friday, 12 September 2025 06:48:17 Central European Summer Time Chia-I Wu wrote:
> 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.
Could you elaborate on your preferred approach? num_channels will be
smaller than the bit width of rx_sts. I could loop from __fls down to
0 checking for (!rx_sts ^ rx_handled) for an early exit, but I'm not
sure if this microoptimisation is what you meant or if that even makes
much sense instruction cost wise.
Alternatively, I could loop from __fss to num_channels.
Is there some commonly used hyper-optimised "loop over set bits" macro
that I'm unfamiliar with?
> > +
> > + writel(rx_handled, ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > +
> > + if (!(rx_sts ^ rx_handled))
> "rx_sts == rx_handled" should be more direct.
Good point, will change.
> > + 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.
>
Yeah, I agree that this is bad. As wens pointed out, my IRQ clearing
earlier would most certainly open us up to losing data. I'll
definitely look into changing this; iirc the current code resulted
from me being unsure whether partially handled IRQs should still
be counted as a handled IRQ or not.
> > +}
> > +
> > +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.
>
Wouldn't storing rx_handled open us up to data races with unpleasant
code to try and prevent them? As far as I understand, the ISR is allowed
to fire again before the thread has completed. Having the rx_status bits
as individual atomics gives us finer granularity than needing to protect
the entire set of bits.
If there is some way to pass data from the ISR to the awoken thread
directly as a per-invocation parameter rather than a squirreled away
member of the driver private struct, then I agree doing it your way
seems better.
> > + 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?
>
Yeah, common mailbox framework does not do a copy here, it just passes
the pointer. Which is good, because nobody likes copies, and it
prevents the mailbox core from needing to know the size of received
data.
So the individual rx_bufs are needed to prevent channel cross-talk.
> > + /* 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)
Only if someone with a pointer to chan modifies con_priv outside of
what the mailbox driver set. It's an unlikely scenario, so I do think
I can remove this check, but the downside is that if someone ever does
manage to do this then the next line is a convenient read primitive.
> > +
> > + 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.
Now that I'm thinking about it, I can probably get rid of no_response
altogether. It was originally added because I conflated txdone with
response received, and one particular command on the sleep channel,
namely powering off the MFG, is unable to ever cause a response
interrupt to fire.
Since we now poll for txdone anyway using the TX status register, I
don't think this is needed at all anymore, so I'll try and get
rid of it.
> > +
> > + 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.
We do need the sum of all rx_len's but you're correct that the current code
allocates too much memory. What I think it should do is
last rx_offset + last rx_len - first rx_offset
to get the total amount of MMIO occupied by RX.
> > +
> > + 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".
Hmmm, yeah. Though, I don't enjoy chan - ebm->mbox.chans because now we're
in the woods regarding pointer arithmetics.
I think the inline helper makes more sense, even if explicitly using a
pointer as an integer gives me the heebie-jeebies.
> > + 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
> >
>
Thank you for your review!
Kind regards,
Nicolas Frattaroli
Powered by blists - more mailing lists