| 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: <f3b6a690f73e8f5a5370a587d0b1671e96e8b5b2.camel@mediatek.com>
Date: Tue, 18 Mar 2025 08:32:15 +0000
From: Jjian Zhou (周建) <Jjian.Zhou@...iatek.com>
To: "jassisinghbrar@...il.com" <jassisinghbrar@...il.com>, "robh@...nel.org"
<robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>, "krzk@...nel.org"
<krzk@...nel.org>, AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, "matthias.bgg@...il.com"
<matthias.bgg@...il.com>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>, "wenst@...omium.org"
<wenst@...omium.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol
interface
On Mon, 2025-03-17 at 18:16 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 17/03/2025 12:03, Jjian Zhou wrote:
> > Some of mediatek processors contain the Risc-V coprocessor.
> >
> > The communication between Host CPU and vcp firmware is
> > taking place using a shared memory area for message passing.
> >
> > VCP IPC protocol offers (send/recv) interfaces using
> > mediatek-mailbox APIs.
> >
> > Signed-off-by: Jjian Zhou <jjian.zhou@...iatek.com>
> > ---
> > drivers/firmware/Kconfig | 9 +
> > drivers/firmware/Makefile | 1 +
> > drivers/firmware/mtk-vcp-ipc.c | 481
> > ++++++++++++++++++
> > include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
> > 4 files changed, 642 insertions(+)
> > create mode 100644 drivers/firmware/mtk-vcp-ipc.c
> > create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h
>
> Do not send new versions while previous discussion is still going.
>
> You still did not respond to previous feedback and you already sent
> two
> versions repeating the same mistake.
>
I'm sorry for my mistake.
After we have discussed it, I will fix it in the next version.
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 37e43f287e78..98c4ff667836 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -179,6 +179,15 @@ config MTK_ADSP_IPC
> > ADSP exists on some mtk processors.
> > Client might use shared memory to exchange information with
> > ADSP.
> >
>
> ..
>
> > +
> > +/*
> > + * mtk_vcp_ipc_send - send ipc command to MTK VCP
> > + *
> > + * @ipidev: VCP struct mtk_ipi_device handle
> > + * @id: id of the feature IPI
> > + * @data: message address
> > + * @len: message length
> > + *
> > + * Return: Zero for success from mbox_send_message
> > + * negative value for error
> > + */
> > +int mtk_vcp_ipc_send(struct mtk_ipi_device *ipidev, u32 id, void
> > *data, u32 len)
> > +{
> > + struct device *dev;
> > + struct mtk_mbox_info *minfo;
> > + struct mtk_ipi_chan_table *table;
> > + struct mtk_vcp_ipc *vcp_ipc;
> > + int ret;
> > +
> > + if (!ipidev || !ipidev->ipi_inited || !data)
> > + return IPI_UNAVAILABLE;
> > + vcp_ipc = ipidev->vcp_ipc;
> > + if (!vcp_ipc)
> > + return IPI_UNAVAILABLE;
> > +
> > + table = ipidev->table;
> > + dev = ipidev->vcp_ipc->dev;
> > + minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> > + if (!minfo) {
> > + dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev-
> > >name, id);
> > + return IPI_UNAVAILABLE;
> > + }
> > +
> > + if (len > table[id].msg_size)
> > + return IPI_MSG_TOO_BIG;
> > + else if (!len)
> > + len = table[id].msg_size;
> > +
> > + mutex_lock(&table[id].mutex_send);
> > +
> > + minfo->ipi_info.msg = data;
> > + minfo->ipi_info.len = len;
> > + minfo->ipi_info.id = id;
> > + minfo->ipi_info.index = table[id].send_index;
> > + minfo->ipi_info.slot_ofs = table[id].send_ofs *
> > MBOX_SLOT_SIZE;
> > +
> > + ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> > + mutex_unlock(&table[id].mutex_send);
> > + if (ret < 0) {
> > + dev_err(dev, "%s IPI%d send failed.\n", ipidev->name,
> > id);
> > + return IPI_MBOX_ERR;
> > + }
> > +
> > + return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_ipc_send);
>
> Drop export - no users
>
> Anyway, every export must be GPL.
The Video Companion Processor (VCP) driver (currently being prepared
for submission to the community) will call it.
>
> > +
> > +/*
> > + * mtk_vcp_ipc_send_compl - send ipc command to MTK VCP
> > + *
> > + * @ipidev: VCP struct mtk_ipi_device handle
> > + * @id: id of the feature IPI
> > + * @data: message address
> > + * @len: message length
> > + * @timeout_ms:
> > + *
> > + * Return: Zero for success from mbox_send_message
> > + * negative value for error
> > + */
> > +int mtk_vcp_ipc_send_compl(struct mtk_ipi_device *ipidev, u32 id,
> > + void *data, u32 len, u32 timeout_ms)
> > +{
> > + struct device *dev;
> > + struct mtk_mbox_info *minfo;
> > + struct mtk_ipi_chan_table *table;
> > + struct mtk_vcp_ipc *vcp_ipc;
> > + int ret;
> > +
> > + if (!ipidev || !ipidev->ipi_inited || !data)
> > + return IPI_UNAVAILABLE;
> > + vcp_ipc = ipidev->vcp_ipc;
> > + if (!vcp_ipc)
> > + return IPI_UNAVAILABLE;
> > +
> > + table = ipidev->table;
> > + dev = ipidev->vcp_ipc->dev;
> > + minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> > + if (!minfo) {
> > + dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev-
> > >name, id);
> > + return IPI_UNAVAILABLE;
> > + }
> > +
> > + if (len > table[id].msg_size)
> > + return IPI_MSG_TOO_BIG;
> > + else if (!len)
> > + len = table[id].msg_size;
> > +
> > + mutex_lock(&table[id].mutex_send);
> > +
> > + minfo->ipi_info.msg = data;
> > + minfo->ipi_info.len = len;
> > + minfo->ipi_info.id = id;
> > + minfo->ipi_info.index = table[id].send_index;
> > + minfo->ipi_info.slot_ofs = table[id].send_ofs *
> > MBOX_SLOT_SIZE;
> > +
> > + atomic_inc(&table[id].holder);
> > +
> > + ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> > + if (ret < 0) {
> > + atomic_set(&table[id].holder, 0);
> > + mutex_unlock(&table[id].mutex_send);
> > + dev_err(dev, "%s IPI%d send failed.\n", ipidev->name,
> > id);
> > + return IPI_MBOX_ERR;
> > + }
> > +
> > + /* wait for completion */
> > + ret = wait_for_completion_timeout(&table[id].notify,
> > + msecs_to_jiffies(timeout_ms
> > ));
> > + atomic_set(&table[id].holder, 0);
> > + if (ret > 0)
> > + ret = IPI_ACTION_DONE;
> > +
> > + mutex_unlock(&table[id].mutex_send);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_ipc_send_compl);
>
> NAK, no users.
>
The VCP driver will call it.
> > +
> > +int mtk_vcp_mbox_ipc_register(struct mtk_ipi_device *ipidev, int
> > id,
> > + mbox_pin_cb_t cb, void *prdata, void
> > *msg)
> > +{
> > + if (!ipidev || !ipidev->ipi_inited)
> > + return IPI_DEV_ILLEGAL;
> > + if (!msg)
> > + return IPI_NO_MSGBUF;
> > +
> > + if (ipidev->table[id].pin_buf)
> > + return IPI_ALREADY_USED;
> > + ipidev->table[id].mbox_pin_cb = cb;
> > + ipidev->table[id].pin_buf = msg;
> > + ipidev->table[id].prdata = prdata;
> > +
> > + return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_register);
>
> NAK, no users.
>
The VCP driver will call it.
>
> > +
> > +int mtk_vcp_mbox_ipc_unregister(struct mtk_ipi_device *ipidev, int
> > id)
> > +{
> > + if (!ipidev || !ipidev->ipi_inited)
> > + return IPI_DEV_ILLEGAL;
> > +
> > + /* Drop the ipi and reset the record */
> > + complete(&ipidev->table[id].notify);
> > +
> > + ipidev->table[id].mbox_pin_cb = NULL;
> > + ipidev->table[id].pin_buf = NULL;
> > + ipidev->table[id].prdata = NULL;
> > +
> > + return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_unregister);
>
> NAK, no users.
>
The VCP driver will call it.
>
> > +
> > +static void mtk_fill_in_entry(struct mtk_ipi_chan_table *entry,
> > const u32 ipi_id,
> > + const struct mtk_mbox_table *mbdev)
> > +{
> > + const struct mtk_mbox_send_table *mbox_send = mbdev-
> > >send_table;
> > + u32 index;
> > +
> > + for (index = 0; index < mbdev->send_count; index++) {
> > + if (ipi_id != mbox_send[index].ipi_id)
> > + continue;
> > +
> > + entry->send_ofs = mbox_send[index].offset;
> > + entry->send_index = mbox_send[index].pin_index;
> > + entry->msg_size = mbox_send[index].msg_size;
> > + entry->mbox = mbox_send[index].mbox_id;
> > + return;
> > + }
> > +
> > + entry->mbox = -ENOENT;
> > +}
> > +
> > +int mtk_vcp_ipc_device_register(struct mtk_ipi_device *ipidev,
> > + u32 ipi_chan_count, struct
> > mtk_vcp_ipc *vcp_ipc)
> > +{
> > + struct mtk_ipi_chan_table *ipi_chan_table;
> > + struct mtk_mbox_table *mbdev;
> > + u32 index;
> > +
> > + if (!vcp_ipc || !ipidev)
> > + return -EINVAL;
> > +
> > + ipi_chan_table = kcalloc(ipi_chan_count,
> > + sizeof(struct mtk_ipi_chan_table),
> > GFP_KERNEL);
> > + if (!ipi_chan_table)
> > + return -ENOMEM;
> > +
> > + mbdev = vcp_ipc->mbdev;
> > + vcp_ipc->ipi_priv = (void *)ipidev;
> > + ipidev->table = ipi_chan_table;
> > + ipidev->vcp_ipc = vcp_ipc;
> > +
> > + for (index = 0; index < ipi_chan_count; index++) {
> > + atomic_set(&ipi_chan_table[index].holder, 0);
> > + mutex_init(&ipi_chan_table[index].mutex_send);
> > + init_completion(&ipi_chan_table[index].notify);
> > + mtk_fill_in_entry(&ipi_chan_table[index], index,
> > mbdev);
> > + }
> > +
> > + ipidev->ipi_inited = 1;
> > +
> > + dev_dbg(vcp_ipc->dev, "%s (with %d IPI) has registered.\n",
> > + ipidev->name, ipi_chan_count);
> > +
> > + return IPI_ACTION_DONE;
> > +}
> > +EXPORT_SYMBOL(mtk_vcp_ipc_device_register);
>
> NAK, no users.
>
The VCP driver will call it.
>
> > +
> > +static int setup_mbox_table(struct mtk_mbox_table *mbdev, u32
> > mbox)
> > +{
> > + struct mtk_mbox_send_table *mbox_send = &mbdev-
> > >send_table[0];
> > + struct mtk_mbox_recv_table *mbox_recv = &mbdev-
> > >recv_table[0];
> > + u32 i, last_ofs = 0, last_idx = 0, last_slot = 0, last_sz =
> > 0;
> > +
> > + for (i = 0; i < mbdev->send_count; i++) {
> > + if (mbox == mbox_send[i].mbox_id) {
> > + mbox_send[i].offset = last_ofs + last_slot;
> > + mbox_send[i].pin_index = last_idx + last_sz;
> > + last_idx = mbox_send[i].pin_index;
> > + last_sz = DIV_ROUND_UP(mbox_send[i].msg_size,
> > MBOX_SLOT_ALIGN);
> > + last_ofs = last_sz * MBOX_SLOT_ALIGN;
> > + last_slot = last_idx * MBOX_SLOT_ALIGN;
> > + } else if (mbox < mbox_send[i].mbox_id) {
> > + /* no need to search the rest id */
> > + break;
> > + }
> > + }
> > +
> > + for (i = 0; i < mbdev->recv_count; i++) {
> > + if (mbox == mbox_recv[i].mbox_id) {
> > + mbox_recv[i].offset = last_ofs + last_slot;
> > + mbox_recv[i].pin_index = last_idx + last_sz;
> > + last_idx = mbox_recv[i].pin_index;
> > + last_sz = DIV_ROUND_UP(mbox_recv[i].msg_size,
> > MBOX_SLOT_ALIGN);
> > + last_ofs = last_sz * MBOX_SLOT_ALIGN;
> > + last_slot = last_idx * MBOX_SLOT_ALIGN;
> > + } else if (mbox < mbox_recv[i].mbox_id) {
> > + /* no need to search the rest id */
> > + break;
> > + }
> > + }
> > +
> > + if (last_idx > MBOX_MAX_PIN || (last_ofs + last_slot) >
> > MAX_SLOT_NUM)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_vcp_ipc_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct mtk_vcp_ipc *vcp_ipc;
> > + struct mbox_client *cl;
> > + struct mtk_mbox_info *minfo;
> > + int ret;
> > + u32 mbox, i;
> > + struct mtk_mbox_table *mbox_data = dev_get_platdata(dev);
> > +
> > + device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
> > +
> > + vcp_ipc = devm_kzalloc(dev, sizeof(*vcp_ipc), GFP_KERNEL);
> > + if (!vcp_ipc)
> > + return -ENOMEM;
> > +
> > + if (!mbox_data) {
>
> Check goes immediately after declaration. I doubt it is useful in the
> first place as this cannot even happen...
>
>
> > + dev_err(dev, "No platform data available\n");
>
> No, drop. Cannot happen or fix your drivers. Who provides the
> platdata here?
The VCP driver will call platform_device_register_data to register the
structure data. mtk_vcp_ipc_probe will be triggered by vcp_probe. This
structure data is the structure we described in the cover letter.
>
> > + return -EINVAL;
> > + }
> > + vcp_ipc->mbdev = mbox_data;
> > +
> > + /* alloc and init mmup_mbox_info */
> > + vcp_ipc->info_table = vzalloc(sizeof(*vcp_ipc->info_table) *
> > VCP_MBOX_NUM);
> > + if (!vcp_ipc->info_table)
> > + return -ENOMEM;
> > +
> > + /* create mbox dev */
> > + for (mbox = 0; mbox < VCP_MBOX_NUM; mbox++) {
> > + minfo = &vcp_ipc->info_table[mbox];
> > + minfo->mbox_id = mbox;
> > + minfo->vcp_ipc = vcp_ipc;
> > + spin_lock_init(&minfo->mbox_lock);
> > +
> > + ret = setup_mbox_table(vcp_ipc->mbdev, mbox);
> > + if (ret)
> > + return ret;
> > +
> > + cl = &minfo->cl;
> > + cl->dev = &pdev->dev;
> > + cl->tx_block = false;
> > + cl->knows_txdone = false;
> > + cl->tx_prepare = NULL;
> > + cl->rx_callback = mtk_vcp_ipc_recv;
> > + minfo->ch = mbox_request_channel_byname(cl,
> > mbox_names[mbox]);
> > + if (IS_ERR(minfo->ch)) {
> > + ret = PTR_ERR(minfo->ch);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(dev, "Failed to request mbox
> > channel %s ret %d\n",
> > + mbox_names[mbox], ret);
>
> Do not open code dev_err_probe.
OK, I modify it as below:
dev_err_probe(dev, PTR_ERR(minfo->ch),
"Failed to request mbox channel %s \n",
mbox_names[mbox])
>
> > +
> > + for (i = 0; i < mbox; i++) {
> > + minfo = &vcp_ipc->info_table[i];
> > + mbox_free_channel(minfo->ch);
> > + }
> > +
> > + vfree(vcp_ipc->info_table);
> > + return ret;
> > + }
> > + }
> > +
> > + vcp_ipc->dev = dev;
> > + dev_set_drvdata(dev, vcp_ipc);
> > + dev_dbg(dev, "MTK VCP IPC initialized\n");
>
> No, drop
OK. I will remove this debug log.
>
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists