| 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: <ZiA+mE3QduLxc+a5@hu-bjorande-lv.qualcomm.com>
Date: Wed, 17 Apr 2024 14:26:48 -0700
From: Bjorn Andersson <quic_bjorande@...cinc.com>
To: Sibi Sankar <quic_sibis@...cinc.com>
CC: <sudeep.holla@....com>, <cristian.marussi@....com>, <andersson@...nel.org>,
<konrad.dybcio@...aro.org>, <jassisinghbrar@...il.com>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<dmitry.baryshkov@...aro.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<quic_rgottimu@...cinc.com>, <quic_kshivnan@...cinc.com>,
<conor+dt@...nel.org>, <quic_gkohli@...cinc.com>,
<quic_nkela@...cinc.com>, <quic_psodagud@...cinc.com>
Subject: Re: [PATCH V3 2/5] mailbox: Add support for QTI CPUCP mailbox
controller
On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> new file mode 100644
> index 000000000000..059eb25f217c
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
Nope.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +
> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
> +#define APSS_CPUCP_MBOX_CMD_OFF 0x4
> +
> +/* Tx Registers */
> +#define APSS_CPUCP_TX_MBOX_IDR 0
> +#define APSS_CPUCP_TX_MBOX_CMD(i) (0x100 + ((i) * 8))
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_IDR 0
> +#define APSS_CPUCP_RX_MBOX_CMD(i) (0x100 + ((i) * 8))
> +#define APSS_CPUCP_RX_MBOX_MAP 0x4000
> +#define APSS_CPUCP_RX_MBOX_STAT 0x4400
> +#define APSS_CPUCP_RX_MBOX_CLEAR 0x4800
> +#define APSS_CPUCP_RX_MBOX_EN 0x4C00
Can we have lower case hex digits, plz?
> +#define APSS_CPUCP_RX_MBOX_CMD_MASK 0xFFFFFFFFFFFFFFFF
> +
> +/**
> + * struct qcom_cpucp_mbox - Holder for the mailbox driver
> + * @chans: The mailbox channel
> + * @mbox: The mailbox controller
> + * @tx_base: Base address of the CPUCP tx registers
> + * @rx_base: Base address of the CPUCP rx registers
> + * @dev: Device associated with this instance
> + * @irq: CPUCP to AP irq
@dev and @irq can be a local variables in qcom_cpucp_mbox_probe().
> + */
> +struct qcom_cpucp_mbox {
> + struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> + struct mbox_controller mbox;
> + void __iomem *tx_base;
> + void __iomem *rx_base;
> + struct device *dev;
> + int irq;
> +};
> +
> +static inline int channel_number(struct mbox_chan *chan)
> +{
> + return chan - chan->mbox->chans;
> +}
> +
> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> +{
> + struct qcom_cpucp_mbox *cpucp = data;
> + struct mbox_chan *chan;
> + unsigned long flags;
> + u64 status;
> + u32 val;
> + int i;
> +
> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +
> + for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
> + val = 0;
This value is immediately overwritten (or unused).
> + if (status & BIT(i)) {
Can't you combine the for loop and this conditional into a
for_each_bit_set()?
> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> + chan = &cpucp->chans[i];
> + spin_lock_irqsave(&chan->lock, flags);
Can you please add a comment here to document that the lock is taken
here to deal with races against client registration? (It wasn't obvious
to me...)
> + if (chan->cl)
> + mbox_chan_received_data(chan, &val);
> + writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> + spin_unlock_irqrestore(&chan->lock, flags);
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
> +{
> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + unsigned long chan_id = channel_number(chan);
> + u64 val;
> +
> + val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> + val |= BIT(chan_id);
> + writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +
> + return 0;
> +}
> +
> +static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
> +{
> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + unsigned long chan_id = channel_number(chan);
> + u64 val;
> +
> + val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> + val &= ~BIT(chan_id);
> + writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> +}
> +
> +static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + unsigned long chan_id = channel_number(chan);
> + u32 *val = data;
> +
> + writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD(chan_id) + APSS_CPUCP_MBOX_CMD_OFF);
> +
> + return 0;
> +}
> +
> +static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
> + .startup = qcom_cpucp_mbox_startup,
> + .send_data = qcom_cpucp_mbox_send_data,
> + .shutdown = qcom_cpucp_mbox_shutdown
> +};
> +
> +static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> +{
> + struct qcom_cpucp_mbox *cpucp;
> + struct mbox_controller *mbox;
> + int ret;
> +
> + cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
> + if (!cpucp)
> + return -ENOMEM;
> +
> + cpucp->dev = &pdev->dev;
> +
> + cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
> + if (IS_ERR(cpucp->rx_base))
> + return PTR_ERR(cpucp->rx_base);
> +
> + cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 1, NULL);
> + if (IS_ERR(cpucp->tx_base))
> + return PTR_ERR(cpucp->tx_base);
> +
> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> + writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +
> + cpucp->irq = platform_get_irq(pdev, 0);
> + if (cpucp->irq < 0)
> + return cpucp->irq;
> +
> + ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
> + IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> + if (ret < 0)
> + return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
> +
> + writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +
> + mbox = &cpucp->mbox;
> + mbox->dev = cpucp->dev;
> + mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> + mbox->chans = cpucp->chans;
> + mbox->ops = &qcom_cpucp_mbox_chan_ops;
> + mbox->txdone_irq = false;
> + mbox->txdone_poll = false;
> +
> + ret = devm_mbox_controller_register(cpucp->dev, mbox);
> + if (ret)
> + return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
> +
> + platform_set_drvdata(pdev, cpucp);
I don't see you using the drvdata anywhere, can we drop this?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> + { .compatible = "qcom,x1e80100-cpucp-mbox"},
A space after the final '"' would be good for aesthetics.
Regards,
Bjorn
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> +
> +static struct platform_driver qcom_cpucp_mbox_driver = {
> + .probe = qcom_cpucp_mbox_probe,
> + .driver = {
> + .name = "qcom_cpucp_mbox",
> + .of_match_table = qcom_cpucp_mbox_of_match,
> + },
> +};
> +module_platform_driver(qcom_cpucp_mbox_driver);
> +
> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
Powered by blists - more mailing lists