Hi,
On 12/3/21 14:46, Pierre-Louis Bossart wrote:
>
>
> On 12/3/21 4:13 AM, Dan Carpenter wrote:
>> Hello Vinod Koul,
>>
>> The patch f533a035e4da: "ASoC: Intel: mrfld - create separate module
>> for pci part" from Nov 4, 2014, leads to the following Smatch static
>> checker warning:
>>
>> sound/soc/intel/atom/sst/sst_pci.c:102 sst_platform_get_resources()
>> warn: resource freed on success: 'ctx->pci'
>>
>> sound/soc/intel/atom/sst/sst_pci.c
>> 25 static int sst_platform_get_resources(struct intel_sst_drv *ctx)
>> 26 {
>> 27 int ddr_base, ret = 0;
>> 28 struct pci_dev *pci = ctx->pci;
>> 29
>> 30 ret = pci_request_regions(pci, SST_DRV_NAME);
>> 31 if (ret)
>> 32 return ret;
>> 33
>> 34 /* map registers */
>> 35 /* DDR base */
>> 36 if (ctx->dev_id == SST_MRFLD_PCI_ID) {
>> 37 ctx->ddr_base = pci_resource_start(pci, 0);
>> 38 /* check that the relocated IMR base matches with FW Binary */
>> 39 ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base);
>> 40 if (!ctx->pdata->lib_info) {
>> 41 dev_err(ctx->dev, "lib_info pointer NULL\n");
>> 42 ret = -EINVAL;
>> 43 goto do_release_regions;
>> 44 }
>> 45 if (ddr_base != ctx->pdata->lib_info->mod_base) {
>> 46 dev_err(ctx->dev,
>> 47 "FW LSP DDR BASE does not match with IFWI\n");
>> 48 ret = -EINVAL;
>> 49 goto do_release_regions;
>> 50 }
>> 51 ctx->ddr_end = pci_resource_end(pci, 0);
>> 52
>> 53 ctx->ddr = pcim_iomap(pci, 0,
>> 54 pci_resource_len(pci, 0));
>> 55 if (!ctx->ddr) {
>> 56 ret = -EINVAL;
>> 57 goto do_release_regions;
>> 58 }
>> 59 dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);
>> 60 } else {
>> 61 ctx->ddr = NULL;
>> 62 }
>> 63 /* SHIM */
>> 64 ctx->shim_phy_add = pci_resource_start(pci, 1);
>> 65 ctx->shim = pcim_iomap(pci, 1, pci_resource_len(pci, 1));
>> 66 if (!ctx->shim) {
>> 67 ret = -EINVAL;
>> 68 goto do_release_regions;
>> 69 }
>> 70 dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
>> 71
>> 72 /* Shared SRAM */
>> 73 ctx->mailbox_add = pci_resource_start(pci, 2);
>> 74 ctx->mailbox = pcim_iomap(pci, 2, pci_resource_len(pci, 2));
>> 75 if (!ctx->mailbox) {
>> 76 ret = -EINVAL;
>> 77 goto do_release_regions;
>> 78 }
>> 79 dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
>> 80
>> 81 /* IRAM */
>> 82 ctx->iram_end = pci_resource_end(pci, 3);
>> 83 ctx->iram_base = pci_resource_start(pci, 3);
>> 84 ctx->iram = pcim_iomap(pci, 3, pci_resource_len(pci, 3));
>> 85 if (!ctx->iram) {
>> 86 ret = -EINVAL;
>> 87 goto do_release_regions;
>> 88 }
>> 89 dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
>> 90
>> 91 /* DRAM */
>> 92 ctx->dram_end = pci_resource_end(pci, 4);
>> 93 ctx->dram_base = pci_resource_start(pci, 4);
>> 94 ctx->dram = pcim_iomap(pci, 4, pci_resource_len(pci, 4));
>> 95 if (!ctx->dram) {
>> 96 ret = -EINVAL;
>> 97 goto do_release_regions;
>> 98 }
>> 99 dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
>> 100 do_release_regions:
>> 101 pci_release_regions(pci);
>> --> 102 return ret;
>> 103 }
>>
>> Surely there should be a "return 0;" before the do_release_regions:
>> label? How does this code work?
>
> Thanks for reporting this Dan. Yes this doesn't look good at all.
>
> This PCI part is only used on Merrifield/Tangier, and I am not sure if
> anyone ever managed to make audio work with the upstream version of this
> driver. It's my understanding that this platform is no longer maintained
> by Intel, and the Edison Yocto code uses the SOF driver.
>
> The Kconfig updated in 2018 hints at those limitations:
>
> config SND_SST_ATOM_HIFI2_PLATFORM_PCI
> tristate "PCI HiFi2 (Merrifield) Platforms"
> depends on X86 && PCI
> select SND_SST_ATOM_HIFI2_PLATFORM
> help
> If you have a Intel Merrifield/Edison platform, then
> enable this option by saying Y or m. Distros will typically not
> enable this option: while Merrifield/Edison can run a mainline
> kernel with limited functionality it will require a firmware file
> which is not in the standard firmware tree
>
> I would guess that indeed a return 0; is missing, but maybe it's time to
> remove this PCI code completely. I can't think of any user of the PCI
> parts of this driver.
>
> Andy, Hans, Mark, Takashi, what do you think?
Merrifield/Edison is something Andy focuses on. I'm more focused on
Bay Trail and Cherry Trail, so this is best answered by Andy (which
he has already done).
Regards,
Hans
