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? > 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?
Attachment:
signature.asc
Description: PGP signature
