Re: [PATCH v1 3/5] mailbox: add Microchip IPC support

[Valentina.FernandezAlanis]

On 12/09/2024 22:30, Samuel Holland wrote:
> [You don't often get email from samuel.holland@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Valentina,
> 
> On 2024-09-12 12:00 PM, Valentina Fernandez wrote:
>> +static int mchp_ipc_probe(struct platform_device *pdev)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct mchp_ipc_probe ipc_info;
>> +     struct microchip_ipc *ipc;
>> +     struct ipc_chan_info *priv;
>> +     bool irq_avail = false;
>> +     int ret;
>> +     u32 chan_id;
>> +
>> +     ret = sbi_probe_extension(SBI_EXT_MICROCHIP_TECHNOLOGY);
>> +     if (ret <= 0)
>> +             return dev_err_probe(dev, ret, "Microchip SBI extension not detected\n");
>> +
>> +     ipc = devm_kzalloc(dev, sizeof(*ipc), GFP_KERNEL);
>> +     if (!ipc)
>> +             return -ENOMEM;
>> +
>> +     platform_set_drvdata(pdev, ipc);
>> +
>> +     ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(IPC_DMA_BIT_MASK));
>> +     if (ret)
>> +             return dev_err_probe(dev, ret, "dma_set_mask_and_coherent failed\n");
>> +
>> +     ipc->buf_base = dmam_alloc_coherent(dev, sizeof(u32), &ipc->dma_addr, GFP_KERNEL);
>> +
>> +     if (!ipc->buf_base)
>> +             return -ENOMEM;
> 
> One drive-by comment here: you don't need to use the DMA API to get a physical
> address for passing to the SBI interface. You can use __pa() on a kmalloc'd
> buffer, since kmalloc() returns memory from the linear map. This has the
> advantage of 1) using cacheable memory and 2) not rounding up the allocation
> size to a whole page.
Hi Samuel,

Thanks for the suggestion, I will take a look into that approach
for the next iterarion of the series.

Thanks,
Valentina
> 
>> +
>> +     ret = mchp_ipc_sbi_send(SBI_EXT_IPC_PROBE, ipc->dma_addr);
>> +     if (ret < 0)
>> +             return dev_err_probe(dev, ret, "could not probe IPC SBI service\n");
>> +
>> +     memcpy(&ipc_info, ipc->buf_base, sizeof(struct mchp_ipc_probe));
> 
> Here sizeof(struct mchp_ipc_probe) > sizeof(u32), so if the DMA API wasn't
> rounding up the allocation size, this would be a buffer overflow.
> 
> Regards,
> Samuel
> 
>> +     ipc->num_channels = ipc_info.num_channels;
>> +     ipc->hw_type = ipc_info.hw_type;
>> +
>> +     ipc->chans = devm_kcalloc(dev, ipc->num_channels, sizeof(*ipc->chans), GFP_KERNEL);
>> +     if (!ipc->chans)
>> +             return -ENOMEM;
>> +
>> +     ipc->dev = dev;
>> +     ipc->controller.txdone_irq = true;
>> +     ipc->controller.dev = ipc->dev;
>> +     ipc->controller.ops = &mchp_ipc_ops;
>> +     ipc->controller.chans = ipc->chans;
>> +     ipc->controller.num_chans = ipc->num_channels;
>> +     ipc->controller.of_xlate = mchp_ipc_mbox_xlate;
>> +
>> +     for (chan_id = 0; chan_id < ipc->num_channels; chan_id++) {
>> +             priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +             if (!priv)
>> +                     return -ENOMEM;
>> +
>> +             ipc->chans[chan_id].con_priv = priv;
>> +             priv->id = chan_id;
>> +     }
>> +
>> +     if (ipc->hw_type == MIV_IHC) {
>> +             ipc->cluster_cfg = devm_kcalloc(dev, num_online_cpus(),
>> +                                             sizeof(struct mchp_ipc_cluster_cfg),
>> +                                             GFP_KERNEL);
>> +             if (!ipc->cluster_cfg)
>> +                     return -ENOMEM;
>> +
>> +             if (mchp_ipc_get_cluster_aggr_irq(ipc))
>> +                     irq_avail = true;
>> +     }
>> +
>> +     if (!irq_avail)
>> +             return dev_err_probe(dev, -ENODEV, "missing interrupt property\n");
>> +
>> +     ret = devm_mbox_controller_register(dev, &ipc->controller);
>> +     if (ret)
>> +             return dev_err_probe(dev, ret,
>> +                                      "Inter-Processor communication (IPC) registration failed\n");
>> +
>> +     return 0;
>> +}
>