Messages in this thread | | | Date | Fri, 17 Jun 2022 20:46:24 +0800 | Subject | Re: [PATCH] spi: rockchip: Disable local irq when pio write out of interrupt service | From | Jon Lin <> |
| |
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
| |