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

[David Jander <david@xxxxxxxxxxx>]

On Wed, 8 Jun 2022 15:43:22 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Thu, May 26, 2022 at 2:46 AM David Jander <david@xxxxxxxxxxx> wrote:
> >
> > The interaction with the controller message queue and its corresponding
> > auxiliary flags and variables requires the use of the queue_lock which is
> > costly. Since spi_sync will transfer the complete message anyway, and not
> > return until it is finished, there is no need to put the message into the
> > queue if the queue is empty. This can save a lot of overhead.
> >
> > As an example of how significant this is, when using the MCP2518FD SPI CAN
> > controller on a i.MX8MM SoC, the time during which the interrupt line
> > stays active (during 3 relatively short spi_sync messages), is reduced
> > from 98us to 72us by this patch.  
> 
> ...
> 
> > +       /* If another context is idling the device then wait */
> > +       while (ctlr->idling) {
> > +               printk(KERN_INFO "spi sync message processing: controller is idling!\n");  
> 
> printk() when we have a device pointer, why (despite of pr_info() existence)?

Sorry for that. I often use printk() explicitly to remind me of things that
need to get removed, but in this case I left this broken piece of code on
purpose for the discussion and immediately addressed it in a reply to this
patch (hence the RFC tag in the subject).
Thanks for being vigilant, and sorry for the noise.

> > +               usleep_range(10000, 11000);
> > +       }
> > +
> > +       was_busy = READ_ONCE(ctlr->busy);
> > +
> > +       ret = __spi_pump_transfer_message(ctlr, msg, was_busy);
> > +       if (ret)
> > +               goto out;
> > +
> > +       if (!was_busy) {
> > +               kfree(ctlr->dummy_rx);
> > +               ctlr->dummy_rx = NULL;
> > +               kfree(ctlr->dummy_tx);
> > +               ctlr->dummy_tx = NULL;
> > +               if (ctlr->unprepare_transfer_hardware &&
> > +                   ctlr->unprepare_transfer_hardware(ctlr))
> > +                       dev_err(&ctlr->dev,
> > +                               "failed to unprepare transfer hardware\n");
> > +               spi_idle_runtime_pm(ctlr);
> > +       }  
> 

Best regards,

-- 
David Jander