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; >> +} >
