[bug report] ASoC: Intel: mrfld - create separate module for pci part
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?
regards, dan carpenter
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?
On Fri, Dec 03, 2021 at 07:46:04AM -0600, Pierre-Louis Bossart wrote:
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?
I'm fine with removing it.
On Fri, Dec 03, 2021 at 07:46:04AM -0600, 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?
The Edison platform and actually some more based on Intel Merrifield are still alive and on the (second hand) market. But yes, I would rather focus on making SOF working there, but via PCI bus (or with ACPI, ASL code for which one should actually write down, currently it's a device with PCI interface only).
On Fri, Dec 03, 2021 at 10:37:07PM +0200, Andy Shevchenko wrote:
On Fri, Dec 03, 2021 at 07:46:04AM -0600, 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:
...
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?
The Edison platform and actually some more based on Intel Merrifield are still alive and on the (second hand) market. But yes, I would rather focus on making SOF working there, but via PCI bus (or with ACPI, ASL code for which one should actually write down, currently it's a device with PCI interface only).
That said, Pierre, have you been able to setup your Edison to see anything on I2S with SOF?
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?
The Edison platform and actually some more based on Intel Merrifield are still alive and on the (second hand) market. But yes, I would rather focus on making SOF working there, but via PCI bus (or with ACPI, ASL code for which one should actually write down, currently it's a device with PCI interface only).
That said, Pierre, have you been able to setup your Edison to see anything on I2S with SOF?
No, I haven't touched my Edison boards since the initial integration with you and Ferry in 2020. I see that the firmware was updated to 1.8 and the kernel is 5.10+ now, so that should be easier than last time. We don't really need any change for the driver probe, PCI is just fine, what's missing is an ACPI recipe to enable audio functionality over the SSP pins.
Hi
Op 03-12-2021 om 23:58 schreef Pierre-Louis Bossart:
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?
The Edison platform and actually some more based on Intel Merrifield are still alive and on the (second hand) market. But yes, I would rather focus on making SOF working there, but via PCI bus (or with ACPI, ASL code for which one should actually write down, currently it's a device with PCI interface only).
That said, Pierre, have you been able to setup your Edison to see anything on I2S with SOF?
I checked with oscilloscope signal was present but did not connect any actual codec.
No, I haven't touched my Edison boards since the initial integration with you and Ferry in 2020. I see that the firmware was updated to 1.8 and the kernel is 5.10+ now, so that should be easier than last time. We don't really need any change for the driver probe, PCI is just fine, what's missing is an ACPI recipe to enable audio functionality over the SSP pins.
+Cc Ferry (the topic is non-SOF version of sound driver for Intel Merrifield)
On Fri, Dec 03, 2021 at 07:46:04AM -0600, 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?
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
participants (6)
-
Andy Shevchenko -
Dan Carpenter -
Ferry Toth -
Hans de Goede -
Mark Brown -
Pierre-Louis Bossart