| Message ID | 20250421211100.687250-4-xi.pardee@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Create Intel PMC SSRAM Telemetry driver | expand |
On Mon, 21 Apr 2025, Xi Pardee wrote: > Move error handling code to generic_core_init() function. The previous > implementation is that init function called for "full cleanup" function > when error occurs which is error prone. The init function should handle > the error path itself to improve code maintainability. > > Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com> > --- > drivers/platform/x86/intel/pmc/core.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c > index 042b60c1185f..e09a97564398 100644 > --- a/drivers/platform/x86/intel/pmc/core.c > +++ b/drivers/platform/x86/intel/pmc/core.c > @@ -1583,10 +1583,26 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info) > if (pmc_dev_info->dmu_guid) > pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid); > > - if (ssram) > - return pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func); > + if (ssram) { > + ret = pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func); > + if (ret) > + goto unmap_regbase; > + } > > return 0; > + > +unmap_regbase: > + for (unsigned int i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) { > + struct pmc *pmc = pmcdev->pmcs[i]; > + > + if (pmc && pmc->regbase) > + iounmap(pmc->regbase); > + } > + > + if (pmcdev->punit_ep) > + pmt_telem_unregister_endpoint(pmcdev->punit_ep); > + > + return ret; > } > > static const struct x86_cpu_id intel_pmc_core_ids[] = { > @@ -1734,7 +1750,8 @@ static int pmc_core_probe(struct platform_device *pdev) > ret = generic_core_init(pmcdev, pmc_dev_info); > > if (ret) { > - pmc_core_clean_structure(pdev); > + platform_set_drvdata(pdev, NULL); > + mutex_destroy(&pmcdev->lock); Please change also the mutex init to use devm_mutex_init(), don't forget to include error handling for it as it devm_mutex_init() can fail. > return ret; > } > >
On 4/24/2025 7:02 AM, Ilpo Järvinen wrote: > On Mon, 21 Apr 2025, Xi Pardee wrote: > >> Move error handling code to generic_core_init() function. The previous >> implementation is that init function called for "full cleanup" function >> when error occurs which is error prone. The init function should handle >> the error path itself to improve code maintainability. >> >> Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com> >> --- >> drivers/platform/x86/intel/pmc/core.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c >> index 042b60c1185f..e09a97564398 100644 >> --- a/drivers/platform/x86/intel/pmc/core.c >> +++ b/drivers/platform/x86/intel/pmc/core.c >> @@ -1583,10 +1583,26 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info) >> if (pmc_dev_info->dmu_guid) >> pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid); >> >> - if (ssram) >> - return pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func); >> + if (ssram) { >> + ret = pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func); >> + if (ret) >> + goto unmap_regbase; >> + } >> >> return 0; >> + >> +unmap_regbase: >> + for (unsigned int i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) { >> + struct pmc *pmc = pmcdev->pmcs[i]; >> + >> + if (pmc && pmc->regbase) >> + iounmap(pmc->regbase); >> + } >> + >> + if (pmcdev->punit_ep) >> + pmt_telem_unregister_endpoint(pmcdev->punit_ep); >> + >> + return ret; >> } >> >> static const struct x86_cpu_id intel_pmc_core_ids[] = { >> @@ -1734,7 +1750,8 @@ static int pmc_core_probe(struct platform_device *pdev) >> ret = generic_core_init(pmcdev, pmc_dev_info); >> >> if (ret) { >> - pmc_core_clean_structure(pdev); >> + platform_set_drvdata(pdev, NULL); >> + mutex_destroy(&pmcdev->lock); > Please change also the mutex init to use devm_mutex_init(), don't forget > to include error handling for it as it devm_mutex_init() can fail. Will change to devm_mutex_init() in next version. Thanks! Xi >> return ret; >> } >> >>
diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c index 042b60c1185f..e09a97564398 100644 --- a/drivers/platform/x86/intel/pmc/core.c +++ b/drivers/platform/x86/intel/pmc/core.c @@ -1583,10 +1583,26 @@ int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info) if (pmc_dev_info->dmu_guid) pmc_core_punit_pmt_init(pmcdev, pmc_dev_info->dmu_guid); - if (ssram) - return pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func); + if (ssram) { + ret = pmc_core_ssram_get_lpm_reqs(pmcdev, pmc_dev_info->pci_func); + if (ret) + goto unmap_regbase; + } return 0; + +unmap_regbase: + for (unsigned int i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) { + struct pmc *pmc = pmcdev->pmcs[i]; + + if (pmc && pmc->regbase) + iounmap(pmc->regbase); + } + + if (pmcdev->punit_ep) + pmt_telem_unregister_endpoint(pmcdev->punit_ep); + + return ret; } static const struct x86_cpu_id intel_pmc_core_ids[] = { @@ -1734,7 +1750,8 @@ static int pmc_core_probe(struct platform_device *pdev) ret = generic_core_init(pmcdev, pmc_dev_info); if (ret) { - pmc_core_clean_structure(pdev); + platform_set_drvdata(pdev, NULL); + mutex_destroy(&pmcdev->lock); return ret; }
Move error handling code to generic_core_init() function. The previous implementation is that init function called for "full cleanup" function when error occurs which is error prone. The init function should handle the error path itself to improve code maintainability. Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com> --- drivers/platform/x86/intel/pmc/core.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)