platform/x86: intel/pmc: Fix iounmap call for valid addresses
Subsystems (2)
Xi Pardee
maintainerPATCH 1/1 platform/x86: intel/pmc: Fix iounmap call for valid addresses
pmc_core_clean_structure() is called when generic_core_init() fails. generic_core_init() could fail before ioremap() is called to get a valid regbase for pmc structure. The current code does not check regbase before calling iounmap(). Add a check to fix it.
--- drivers/platform/x86/intel/pmc/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/x86/intel/pmc/core.c +1 -1
@@ -1471,7 +1471,7 @@ static void pmc_core_clean_structure(struct platform_device *pdev)for (i =0 ; i <ARRAY_SIZE (pmcdev->pmcs); ++i) {struct pmc *pmc = pmcdev->pmcs[i]; -if (pmc) +if (pmc && pmc->regbase)iounmap (pmc->regbase); }
Signature · 1 line
2.43.0
Ilpo Järvinen
maintainerQuoted 4 lines · Xi Pardee
pmc_core_clean_structure()is called whengeneric_core_init()fails.generic_core_init()could fail beforeioremap()is called to get a valid regbase for pmc structure. The current code does not check regbase before callingiounmap(). Add a check to fix it.
Hi,
The approach that calls the same "full cleanup" function as deinit uses when init function fails midway is very error prone as once again is demonstrated. Is this the only error handling problem? Are you 100% sure?
Think about it, init is x% (<100%) done when it fails, then it calls a function that tries to undo 100%. One needs to add lots of special logic to handle 0-100% rollback into that cleanup function. The init function, on the other hand, knows exactly where it was so it can rollback just what is needed and not even try to rollback for more.
It's also very inconsistent to rollback ssram_pcidev in this file as ssram code was moved into core_ssram so I think the ssram deinit should be moved there too.
I think these init functions should be converted to do proper rollback within the init function(s) to avoid very hard to track error handling. I tried to check the error handling now in the pmc driver and after I would have needed to jump between the files, I gave up.
Quoted 17 lines
Fixes 1b8c7b843c00 ("platform/x86:intel/pmc: Discover PMC devices")Signed-off-by Xi Pardee <[email protected]>--- drivers/platform/x86/intel/pmc/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/platform/x86/intel/pmc/core.c +1
-1@@ -1471,7 +1471,7 @@ static void pmc_core_clean_structure(struct platform_device *pdev)for (i =0 ; i <ARRAY_SIZE (pmcdev->pmcs); ++i) {struct pmc *pmc = pmcdev->pmcs[i]; -if (pmc) +if (pmc && pmc->regbase)iounmap (pmc->regbase);
Signature · 1 line
i.
Xi Pardee
maintainerQuoted 18 lines · Ilpo Järvinen
Quoted 4 lines · Xi Pardee
pmc_core_clean_structure()is called whengeneric_core_init()fails.generic_core_init()could fail beforeioremap()is called to get a valid regbase for pmc structure. The current code does not check regbase before callingiounmap(). Add a check to fix it.Hi,
The approach that calls the same "full cleanup" function as deinit uses when init function fails midway is very error prone as once again is demonstrated. Is this the only error handling problem? Are you 100% sure?
Think about it, init is x% (<100%) done when it fails, then it calls a function that tries to undo 100%. One needs to add lots of special logic to handle 0-100% rollback into that cleanup function. The init function, on the other hand, knows exactly where it was so it can rollback just what is needed and not even try to rollback for more.
Quoted 8 lines
It's also very inconsistent to rollback
ssram_pcidevin this file as ssram code was moved intocore_ssramso I think the ssram deinit should be moved there too.I think these init functions should be converted to do proper rollback within the init function(s) to avoid very hard to track error handling. I tried to check the error handling now in the pmc driver and after I would have needed to jump between the files, I gave up.
Hi,
Thanks for the comments! Will move the error handing code for init function to init function in next cycle.
I will send out a new version to separate SSRAM device handling code to a new PCI driver and remove ssram_pcidev field from pmcdev in next cycle.
Thanks!
Xi
Quoted 17 lines
Quoted 17 lines
Fixes 1b8c7b843c00 ("platform/x86:intel/pmc: Discover PMC devices")Signed-off-by Xi Pardee <[email protected]>--- drivers/platform/x86/intel/pmc/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/platform/x86/intel/pmc/core.c +1
-1@@ -1471,7 +1471,7 @@ static void pmc_core_clean_structure(struct platform_device *pdev)for (i =0 ; i <ARRAY_SIZE (pmcdev->pmcs); ++i) {struct pmc *pmc = pmcdev->pmcs[i]; -if (pmc) +if (pmc && pmc->regbase)iounmap (pmc->regbase);