Re: [PATCH] platform/x86: intel/pmc: Fix iounmap call for valid addresses

[Xi Pardee <xi.pardee@linux.intel.com>]

On 3/21/2025 7:49 AM, Ilpo Järvinen wrote:
> On Wed, 19 Mar 2025, Xi Pardee wrote:
>
>> 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.
> 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.

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

>> Fixes: 1b8c7b843c00 ("platform/x86:intel/pmc: Discover PMC devices")
>> Signed-off-by: Xi Pardee <xi.pardee@linux.intel.com>
>> ---
>>   drivers/platform/x86/intel/pmc/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
>> index 7a1d11f2914f..de5fc06232e5 100644
>> --- a/drivers/platform/x86/intel/pmc/core.c
>> +++ b/drivers/platform/x86/intel/pmc/core.c
>> @@ -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);