Re: [PATCH v2] arm64: armv8_deprecated: Checking return value for memory allocation

[Yunfeng Ye]

On 2019/10/8 18:25, Will Deacon wrote:
> On Tue, Oct 08, 2019 at 10:33:17AM +0800, Yunfeng Ye wrote:
>> On 2019/10/7 23:37, Will Deacon wrote:
>>> On Mon, Oct 07, 2019 at 06:06:35PM +0800, Yunfeng Ye wrote:
>>>> @@ -617,25 +624,47 @@ static int t16_setend_handler(struct pt_regs *regs, u32 instr)
>>>>   */
>>>>  static int __init armv8_deprecated_init(void)
>>>>  {
>>>> -	if (IS_ENABLED(CONFIG_SWP_EMULATION))
>>>> -		register_insn_emulation(&swp_ops);
>>>> +	int ret = 0;
>>>> +	int err = 0;
>>>> +
>>>> +	if (IS_ENABLED(CONFIG_SWP_EMULATION)) {
>>>> +		ret = register_insn_emulation(&swp_ops);
>>>> +		if (ret) {
>>>> +			pr_err("register insn emulation swp: fail\n");
>>>> +			err = ret;
>>>> +		}
>>>> +	}
>>>
>>> Is there much point in continuing here? May as well just return ret, I
>>> think. I also don't think you need to print anything, since kmalloc
>>> should already have shouted.
>>>
>> The registration of each instruction simulation is independent. I think
>> that one failure does not affect the registration of other instructions.
> 
> Dunno, I think that if kmalloc() starts failing then it's time to give up!
> 
>> In addition, if return directly, is it need to unregister? Of course,
>> the first instruction registration can be directly returned, If the
>> following instruction registration fails, is it need unregister operation?
>> currently the unregistration of instruction simulation is not be implemented
>> yet.
> 
> That's an interesting one -- currently there isn't a way to unregister
> an emulation hook afaict. We could add unregister_insn_emulation() to
> remove the emulation hook from the insn_emulation list and free it, but
> I'm actually now starting to prefer your initial patch after all. The only
> way these failures will happen are either because the system is doomed
> or kmalloc fault injection is being used; so keeping things simple rather
> than add rarely executed complexity is probably best.
> 
>> The purpose of printing information is to replace the direct return, which
>> can distinguish which instruction failed to register. There is no need to print
>> information if it returns directly.
> 
> What do you expect people to do with that information?
> 
> Are you ok with me applying your original patch?
> 
I agree, it is simple. thanks.

> Will
> 
> .
>