Re: [RFC] [PATCH 3/3] drivers: spi: spi.c: Don't use the message queue if possible in spi_sync

[David Jander <david@protonic.nl>]

On Thu, 9 Jun 2022 17:31:24 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jun 09, 2022 at 05:34:21PM +0200, David Jander wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> > > On Wed, Jun 08, 2022 at 09:54:09AM +0200, David Jander wrote:  
> > > > Mark Brown <broonie@kernel.org> wrote:    
> 
> > > I think the rest of it is fine or at least I'm finding it difficult to
> > > see anything beyond the concurrency issues.  I think we need to do an
> > > audit to find any users that are doing a spi_sync() to complete a
> > > sequence of spi_async() operations but I'm not aware of any and if it
> > > delivers the performance benefits it's probably worth changing that
> > > aspect of the driver API.  
> 
> > I just discovered a different issue (hit upon by Oleksij Rempel while
> > assisting with testing):  
> 
> > Apparently some drivers tend to rely on the fact that master->cur_msg is not
> > NULL and always points to the message being transferred.
> > This could be a show-stopper to this patch set, if it cannot be solved.
> > I am currently analyzing the different cases, to see if and how they could
> > eventually get fixed. The crux of the issue is the fact that there are two
> > different API's towards the driver:  
> 
> That seems resolvable?  If we have two things actually handling a
> message at once then we're in for a bad time so we should be able to
> arrange for cur_msg to be set in the sync path - the usage in the
> message pump between popping off the queue and getting to actually
> starting the transfer could be a local variable with the changes to the
> sync path I think?

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?
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.
The fact that spi_finalize_current_message() is a separate API function, and
not called directly from __spi_pump_messages() had me confused that it might
be called in a different context (from IRQ thread or something like that)
possibly after __spi_pump_messages() had already returned. But that doesn't
make much sense... right?

> >  1. transfer_one(): This call does not provide a reference to the message that
> >  contains the transfers. So all information stored only in the underlying
> >  spi_message are not accessible to the driver. Apparently some work around
> >  this by accessing master->cur_msg.
> 
> >  2. transfer_one_message(): I suspect this is a newer API. It takes the
> >  spi_message as argument, thus giving the driver access to all information it
> >  needs (like return status, and the complete list of transfers).
> 
> It's the other way around - transfer_one() is the result of providing a
> transfer_one_message() which factors out more of the code given that a
> huge proportion of drivers are for hardware which works at the transfer
> level and doesn't understand messages, just as transfer_one_message()
> and the message queue are factoring out code which was originally open
> coded in drivers.

Ah, thanks for the context. This makes sense or course.

Best regards,

-- 
David Jander