Hi, Ralf, What do you think about? If you also prefer to totally remove the BUG_ON(), I will send a new patch. On Sat, Jul 12, 2014 at 5:30 PM, Paul Burton <paul.burton@xxxxxxxxxx> wrote: > On Sat, Jul 12, 2014 at 05:10:35PM +0800, Huacai Chen wrote: >> Hi, Paul, >> >> You means my patch (http://patchwork.linux-mips.org/patch/7297/) is >> the correct way? > > I believe you patch will fix the problem, but I think it would be better > to remove the check for !preemptible() & the BUG_ON entirely. > >> Another question: Your patch >> (http://patchwork.linux-mips.org/patch/7307/) remove >> preempt_disable()/preempt_enable() in init_fpu(). It will cause >> problems if there is another function call init_fpu() because it is >> previously preempt-safe. Maybe introduce a new function (e.g. >> __init_fpu()) is a better way? > > It may cause a problem if there were other callers, but there is only > one caller of init_fpu (enable_restore_fp_context) and it needs to > disable preemption for longer than the init_fpu function anyway. I see > no value in keeping init_fpu as a wrapper that disables preemption > when there would be nothing calling it. > > Thanks, > Paul > >> Huacai >> >> On Sat, Jul 12, 2014 at 7:28 AM, Chen Jie <chenj@xxxxxxxxxx> wrote: >> > 2014-07-11 23:56 GMT+08:00 Paul Burton <paul.burton@xxxxxxxxxx>: >> >> On Fri, Jul 11, 2014 at 11:14:13AM +0800, chenj wrote: >> >>> do_ade may be invoked with preempt enabled. do_cpu will be invoked with >> >>> preempt enabled. When it's preempted(in do_ade/do_cpu), TIF_USEDFPU will be >> >>> cleared, when it returns to do_ade/do_cpu, the fpu is actually disabled. >> >>> >> >>> e.g. >> >>> In do_ade() >> >>> emulate_load_store_insn(): >> >>> BUG_ON(!is_fpu_owner()); <-- This assertion may be breaked. >> >>> >> >>> In do_cpu() >> >>> enable_restore_fp_context(): >> >>> was_fpu_owner = is_fpu_owner(); >> >> >> >> Preemption should indeed be disabled around the assignment & use of the >> >> was_fpu_owner variable, but note that you can only hit the problem if >> >> using MSA. One of the MSA fixes I just submitted also fixes this along >> >> with another instance of the problem: >> >> >> >> http://patchwork.linux-mips.org/patch/7307/ >> >> >> >> I prefer my patch to this since it disables preemption for less time, >> >> in addition to fixing the !used_math() case. >> >> >> >> In emulate_load_store_insn I believe the correct fix is simply to remove >> >> that BUG_ON. The code is about to give up FPU ownership anyway, so it's >> >> not like there is any requirement being violated if it was already lost. >> > Yes, you're right. >> > >> > """ /* arch/mips/kernel/unaligned.c */ >> > lose_fpu(1); /* Save FPU state for the emulator. */ >> > res = fpu_emulator_cop1Handler(regs, ¤t->thread.fpu, 1, &fault_addr); >> > own_fpu(1); /* Restore FPU state. */ >> > """ >> > >> > Going deep into the code, I find lost_fpu(1) will save fpu context if >> > owns fpu (otherwise, if preempted, the fpu context will be saved in >> > process switch), then fpu_emulator_cop1Handler manipulates the saved >> > fpu context, own_fpu(1) restores it. >> > >> > So, remove "BUG_ON(!is_fpu_owner())" is OK. >> > >
