Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <41af9d13-fff5-a416-87cc-4f69e77d6409@rock-chips.com>
Date:   Fri, 17 Jun 2022 20:46:24 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH] spi: rockchip: Disable local irq when pio write out of
 interrupt service
To:     Mark Brown <broonie@kernel.org>
Cc:     heiko@sntech.de, linux-spi@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20220613092744.9726-1-jon.lin@rock-chips.com>
 <YqcveggUU7yaXuk1@sirena.org.uk>
 <b97ee70c-70ee-6b15-65d6-a176125dcfd8@rock-chips.com>
 <YqxpTqvaQb4cohTh@sirena.org.uk>
From:   Jon Lin <jon.lin@rock-chips.com>
In-Reply-To: <YqxpTqvaQb4cohTh@sirena.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2022/6/17 19:45, Mark Brown wrote:
> On Fri, Jun 17, 2022 at 02:24:10PM +0800, Jon Lin wrote:
>> On 2022/6/13 20:37, Mark Brown wrote:
>>> On Mon, Jun 13, 2022 at 05:27:44PM +0800, Jon Lin wrote:
> 
>>>> +	spin_unlock_irqrestore(&rs->lock, flags);
> 
>>> So this is effectively just disabling interrupts during PIO, there's no
>>> other users of the lock which is rather heavyweight.  What's the actual
>>> issue here?  We should also have something saying what's going on in the
>>> code since right now the lock just looks redundant.
> 
>> For lock: In order to avoid special situations, such as when the CPU
>> frequency drops to close to the IO rate, the water line interrupt is
>> triggered during FIFO filling (triggered by other CPUs), resulting in
>> critical resources still not being protected in place. For local IRQ
> 
> So essentially we're so slow in filling the FIFO when starting a
> transfer that the interrupt triggers in the middle of the initial FIFO
> fill?  Something that tricky *really* needs a comment adding.
> 
> Ideally we'd just leave the interrupt masked until the FIFO is filled
> though, looking at the driver I see that there is an interrupt mask
> register which seems to have some level of masking available and I do
> note that in rockchip_spi_prepare_irq() we unmask interrupts before we
> start filling the FIFO rather than afterwards.  Would reversing the
> unmask order there address the issue more cleanly?

This idea is workable, and it's more efficient than previous code, So I 
send a new commit:
https://patchwork.kernel.org/project/spi-devel-general/patch/20220617124251.5051-1-jon.lin@rock-chips.com/
> 
>> disable: Turning off the local interrupt is mainly to prevent the CPU
>> schedule from being interrupted when filling FIFO.
> 
> If it were just this then there's preempt_disable(), but what's the
> problem with being preempted during the FIFO fill?

I think