| 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
| ||
|
Date: Wed, 17 Apr 2024 17:21:21 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
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>,
<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 2/5] mailbox: Add support for QTI CPUCP mailbox controller
On 4/16/24 21:51, Dmitry Baryshkov wrote:
> On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@...cinc.com> wrote:
>>
>> Add support for CPUSS Control Processor (CPUCP) mailbox controller,
>> this driver enables communication between AP and CPUCP by acting as
>> a doorbell between them.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
>> ---
>>
>> rfc:
>> * Use chan->lock and chan->cl to detect if the channel is no longer
>> Available. [Dmitry]
>> * Use BIT() instead of using manual shifts. [Dmitry]
>> * Don't use integer as a pointer value. [Dmitry]
>> * Allow it to default to of_mbox_index_xlate. [Dmitry]
>> * Use devm_of_iomap. [Dmitry]
>> * Use module_platform_driver instead of module init/exit. [Dmitry]
>> * Get channel number using mailbox core (like other drivers) and
>> further simplify the driver by dropping setup_mbox func.
Hey Dmitry,
Thanks for taking time to review the series.
>>
>> drivers/mailbox/Kconfig | 8 ++
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
>> 3 files changed, 215 insertions(+)
>> create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
>>
[snip]
..
>> +
>> + status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> +
>> + for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
>> + val = 0;
>> + if (status & ((u64)1 << i)) {
>
> BIT() or test_bit()
I'll use BIT()
>
>> + val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
>
> #define APSS_CPUCP_MBOX_CMD_OFF(i)
ack
>
>> + chan = &cpucp->chans[i];
>> + spin_lock_irqsave(&chan->lock, flags);
>> + if (chan->cl)
>> + mbox_chan_received_data(chan, &val);
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> + writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>
> Why is status written from inside the loop? If the bits are cleared by
> writing 1, then you should be writing BIT(i) to that register. Also
> make sure that it is written at the correct time, so that if there is
> an event before notifying the driver, it doesn't get lost.
Thanks for catching this. I probably didn't run into this scenario
because of using just one channel at point any time. I'll move it
outside the loop.
>
>> + }
>> + }
>> +
>> + 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 * 8) + 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) {
>> + dev_err(&pdev->dev, "Failed to get the IRQ\n");
>> + return cpucp->irq;
>
> It already prints the error message.
ack
>
>> + }
>> +
>> + ret = devm_request_irq(&pdev->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
>> + IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
>> + return ret;
>
> return dev_err_probe();
ack
>
>> + }
>> +
>> + 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 = mbox_controller_register(mbox);
>
> Use devm_mbox_controller_register()
ack
> >> + if (ret) {
>> + dev_err(&pdev->dev, "Failed to create mailbox\n");
>> + return ret;
>
> return dev_err_probe();
I guess ^^ is a typo? Since devm_mbox_controller_register wouldn't
return -EPROBE_DEFER.
>
>> + }
>> +
>> + platform_set_drvdata(pdev, cpucp);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_cpucp_mbox_remove(struct platform_device *pdev)
>> +{
>> + struct qcom_cpucp_mbox *cpucp = platform_get_drvdata(pdev);
>> +
>> + mbox_controller_unregister(&cpucp->mbox);
> > This will be replaced by devm_mbox_controller_register().
ack
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> + { .compatible = "qcom,x1e80100-cpucp-mbox"},
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> +
>> +static struct platform_driver qcom_cpucp_mbox_driver = {
>> + .probe = qcom_cpucp_mbox_probe,
>> + .remove = qcom_cpucp_mbox_remove,
>> + .driver = {
>> + .name = "qcom_cpucp_mbox",
>> + .of_match_table = qcom_cpucp_mbox_of_match,
>> + .suppress_bind_attrs = true,
>
> No need to. Please drop.
ack
-Sibi
>
>> + },
>> +};
>> +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