[bug report] ASoC: Intel: mrfld - create separate module for pci part

[Pierre-Louis Bossart]

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?