Re: [PATCH] x86: fix error paths in microcode_init()

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 12/02/2011 08:23 PM, Jan Beulich wrote:

>>>> On 02.12.11 at 15:35, Borislav Petkov <bp@amd64.org> wrote:
>> Hi,
>>
>> On Fri, Dec 02, 2011 at 01:35:19PM +0000, Jan Beulich wrote:
>>> After failure of platform_device_register_simple(), microcode_dev_exit()
>>> got called without the call to microcode_dev_init() already having taken
>>> place.
>>>
>>> After failure of microcode_dev_init(), no cleanup of previously carried
>>> out setup was done at all.
>>>
>>> As a result, microcode_dev_exit() can now get __exit tagged on it.
>>>
>>> (Noticed while looking at the code, not because of having experienced
>>> an actual problem.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> ---
>>>  arch/x86/kernel/microcode_core.c |   18 +++++++++++++-----
>>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> --- 3.2-rc4/arch/x86/kernel/microcode_core.c
>>> +++ 3.2-rc4-x86-ucode-init-eh/arch/x86/kernel/microcode_core.c
>>> @@ -256,7 +256,7 @@ static int __init microcode_dev_init(voi
>>>  	return 0;
>>>  }
>>>  
>>> -static void microcode_dev_exit(void)
>>> +static void __exit microcode_dev_exit(void)
>>>  {
>>>  	misc_deregister(&microcode_dev);
>>>  }
>>> @@ -519,10 +519,8 @@ static int __init microcode_init(void)
>>>  
>>>  	microcode_pdev = platform_device_register_simple("microcode", -1,
>>>  							 NULL, 0);
>>> -	if (IS_ERR(microcode_pdev)) {
>>> -		microcode_dev_exit();
>>> +	if (IS_ERR(microcode_pdev))
>>>  		return PTR_ERR(microcode_pdev);
>>> -	}
>>>  
>>>  	get_online_cpus();
>>>  	mutex_lock(&microcode_mutex);
>>> @@ -538,8 +536,18 @@ static int __init microcode_init(void)
>>>  	}
>>>  
>>>  	error = microcode_dev_init();
>>> -	if (error)
>>> +	if (error) {
>>> +		get_online_cpus();
>>> +		mutex_lock(&microcode_mutex);
>>> +
>>> +		sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
>>> +
>>> +		mutex_unlock(&microcode_mutex);
>>> +		put_online_cpus();
>>> +
>>> +		platform_device_unregister(microcode_pdev);
>>>  		return error;
>>> +	}
>>
>> Actually, Srivatsa made a similar patch already which I sent to x86
>> guys (I don't think they've pulled yet) but yours is additionally more
>> careful to do proper locking before doing sysdev_driver_unregister().
>>
>> Would you like to add that part ontop of Srivatsa's patch at the
>> out_sysdev_driver label and resend?
> 
> Why shouldn't this one be taken in its entirety instead?
> 


Link to my patch: https://lkml.org/lkml/2011/11/7/136

Your patch fixes the issue more properly than mine, but adding your part
on top of my patch makes the code look better. For example,
platform_device_unregister() wouldn't need to be called twice; and we 
can use the quite popular way of handling error path via goto statements,
which makes the code flow much more comprehensible and intuitive.

[Btw, in addition to that, since Boris has already asked Ingo to pull my
patch, perhaps it also makes it easier that way. But I definitely see a
plus point in putting your part on top of mine, for code clarity reasons.]

Regards,
Srivatsa S. Bhat
IBM Linux Technology Center