Re: [PATCH] tty: mips_ejtag_fdc: Call cpu_relax() in registers polling busy loops

[Zhongqiu Han <quic_zhonhan@quicinc.com>]

On 12/20/2024 11:06 PM, Greg KH wrote:
> On Fri, Dec 20, 2024 at 09:46:27PM +0800, Zhongqiu Han wrote:
>> On 12/19/2024 9:28 PM, Greg KH wrote:
>>> On Thu, Dec 19, 2024 at 08:42:54PM +0800, Zhongqiu Han wrote:
>>>> It is considered good practice to call cpu_relax() in busy loops, see
>>>> Documentation/process/volatile-considered-harmful.rst. This can lower CPU
>>>> power consumption or yield to a hyperthreaded twin processor, or serve as
>>>> a compiler barrier. In addition, if something goes wrong in the busy loop
>>>> at least it can prevent things from getting worse.
>>>>
>>>> Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
>>>> ---
>>>>    drivers/tty/mips_ejtag_fdc.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
>>>> index afbf7738c7c4..b17ead1e9698 100644
>>>> --- a/drivers/tty/mips_ejtag_fdc.c
>>>> +++ b/drivers/tty/mips_ejtag_fdc.c
>>>> @@ -346,7 +346,7 @@ static void mips_ejtag_fdc_console_write(struct console *c, const char *s,
>>>>    		/* Busy wait until there's space in fifo */
>>>>    		while (__raw_readl(regs + REG_FDSTAT) & REG_FDSTAT_TXF)
>>>> -			;
>>>> +			cpu_relax();
>>>>    		__raw_writel(word.word, regs + REG_FDTX(c->index));
>>>>    	}
>>>>    out:
>>>> @@ -1233,7 +1233,7 @@ static void kgdbfdc_push_one(void)
>>>>    	/* Busy wait until there's space in fifo */
>>>>    	while (__raw_readl(regs + REG_FDSTAT) & REG_FDSTAT_TXF)
>>>> -		;
>>>> +		cpu_relax();
>>>
>>> How did you test this?  Are you _sure_ it is needed at all?  I think you
>>> just made these loops take a lot longer than before :(
>>>
>>> Have you had problems with these tight loops doing anything bad to your
>>> system?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Hi Greg,
>> Thanks a lot for the review~
>>
>> Perhaps I should submit an RFC patch and explain the situation, as I
>> don't have a MIPS device for testing. Indeed, the cpu_relax()
>> implementation for MIPS is a memory barrier, which, compared to busy
>> waiting, doesn't save power and can make loops slower than before.
>> However, according to its definition file, for certain MIPS-based
>> architectures like Loongarch-3, it can help force the Loongson-3 SFB
>> (Store-Fill-Buffer) flush to avoid pending writes. Below is the
>> implementation of cpu_relax() for the MIPS architecture and its
>> comments.
>>
>> -----------------------------------------------------------------
>> arch/mips/include/asm/vdso/processor.h
>>
>> #ifdef CONFIG_CPU_LOONGSON64
>> /*
>>   * Loongson-3's SFB (Store-Fill-Buffer) may buffer writes indefinitely
>>   * when a tight read loop is executed, because reads take priority over
>>   * writes & the hardware (incorrectly) doesn't ensure that writes will
>>   * eventually occur.
>>   *
>>   * Since spin loops of any kind should have a cpu_relax() in them, force
>>   * an SFB flush from cpu_relax() such that any pending writes will
>>   * become visible as expected.
>>   */
>> #define cpu_relax()	smp_mb()
>> #else
>> #define cpu_relax()	barrier()
>> #endif
>> ----------------------------------------------------------------
>>
>> Based on this, cpu_relax() should be needed here? :)
> 
> I don't know, please test and let us know!
> 
> Without testing of this on real hardware, we can't take this change for
> obvious reasons.

Hi Greg,
Sorry for the delay reply.

Sure, I will conduct comparative testing if I have the mips device in
the future, or other developers are also welcome to participate in the
testing.

> 
> thanks,
> 
> greg k-h


-- 
Thx and BRs,
Zhongqiu Han