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

[Zhongqiu Han <quic_zhonhan@quicinc.com>]

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? :)

Thank you~


-- 
Thx and BRs,
Zhongqiu Han