On Fri, Jun 10, 2022 at 02:41:34PM +0100, Mark Brown wrote: > On Fri, Jun 10, 2022 at 09:27:53AM +0200, David Jander wrote: > > Mark Brown <broonie@xxxxxxxxxx> wrote: > > Ok, I first thought that this wouldn't be possible without taking the > > necessary spinlock, but looking a little closer, I think I understand now. > > One question to confirm I understand the code correctly: > > An SPI driver that implements its own transfer_one_message() is required to > > _always_ call spi_finalize_current_message() _before_ returning, right? > Yes, it should. Sorry, my mistake - I misremembered how that worked. They might return without having completed the message since the message pump is a workqueue so it'll just go idle, spi_sync() will work because the caller will block on the completion in the message. It's cur_msg that's stopping any new work being scheduled. I was confusing this with the handling of individual transfers, it's been a while since I got deep into this. The bit about allowing us to finalise in any context still applies - the goal with that interface is to avoid the need to context switch back to the message pump to report that the message completed, and ideally immediately start work on the next message if the controller can cope with that. > > If this is a guarantee and we take the io_mutex at the beginning of > > __spi_pump_messages(), then ctlr->cur_msg is only manipulated with the > > io_mutex held, and that would make it safe to be used in the sync path, which > > is also behind the io_mutex. > > Would appreciate if you could confirm this, just to be sure I understand the > > code correctly. > I think that should work. If there's something missed it should be > possible to make things work that way. Can we move the cleanup of cur_msg out of spi_finalize_current_message() and into the context holding the io_mutex with that blocking on a completion? Don't know what the overhead of that is like, I think it should be fine for the case where we don't actually block on the completion and it shouldn't be any worse in the case where we're completing in the interrupt. Also the kthread_queue_work() that's in there could be moved out to only the queued case since with your new approach for spi_sync() we'll idle the hardware in the calling context and don't need to schedule the thread at all, that should save some overhead. Sorry about misremembering this bit.
Attachment:
signature.asc
Description: PGP signature
