[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.21.2504021933160.53907@angie.orcam.me.uk>
Date: Thu, 3 Apr 2025 13:00:15 +0100 (BST)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: Marco Crivellari <marco.crivellari@...e.com>
cc: linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Frederic Weisbecker <frederic@...nel.org>,
Anna-Maria Behnsen <anna-maria@...utronix.de>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>, Huacai Chen <chenhuacai@...nel.org>
Subject: Re: [PATCH v6 1/1] MIPS: Fix idle VS timer enqueue
On Wed, 2 Apr 2025, Marco Crivellari wrote:
> > Well, you should be able to set a breakpoint at `rollback_handle_int' and
> > fiddle with $epc by hand to see if the code sequence correctly skips over
> > WAIT. Though I reckon QEMU used to have an issue with presenting the MIPS
> > privileged context over its debug stub. Has the issue been fixed? Either
> > way you should be able to just operate on the copy in $k0 retrieved with
> > (D)MFC0.
>
> Nope, seems not fixed so far. But yes, changing $k0 is working fine.
> With the cpu in idle code (executing "wait"), ctrl+c, then placed a bp
> in rollback_handle_init+4. Then "c" to hit the bp.
> When the bp is hit, I can see $k0 = r4k_wait_exit.
>
> I changed $k0 with an address inside the region, and setting a bp on "bne",
> the value is equal to $k1. I'm assuming the value is also saved
> correctly in $epc,
> considering it points correctly to r4k_wait_exit.
I think it's enough evidence to prove the code works as expected.
> > Hmm, "skipover" maybe?
>
> Now that I'm looking at the code, shouldn't it be better to address this in a
> separate patch or another time?
>
> I can see the rollback_handler* is exported also in arch/mips/kernel/traps.c
> And there are a few parts that makes use of the "rollback" name; I'm
> wondering if also
> the code should be refactored a bit then,
> eg arch/mips /include/asm/idle.h:using_rollback_handler()
Yeah, I agree cluttering the semantics change with the rename is not the
best idea, so a follow-up patch approach will be better.
Also I wonder if we could come up with yet another name that does not
have the exact semantics implied, so that if in 20 years' time we change
our minds again and decide: "Oh, let's do the rollback thing after all,"
we don't have to do the rename again. Though it's like I'm diverging into
the bikeshedding area here, so perhaps please just use whatever name you
have found most reasonable at the time you're about to wrap up with this
effort.
Maciej
Powered by blists - more mailing lists