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

[Hans de Goede]

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