Re: [PATCH/RFC] SPI: add DMAUNSAFE analog

[David Brownell <david-b@pacbell.net>]

> static inline ssize_t spi_w8r8(struct spi_device *spi, u8 cmd)
> {
>         ssize_t                 status;
>         u8                      result;
> 
>         status = spi_write_then_read(spi, &cmd, 1, &result, 1);
> 
>         /* return negative errno or unsigned value */
>         return (status < 0) ? status : result;
> }
> 
> You're allocating u8 var on stack, then allocate a 1-byte-long buffer 
> and copy the data instead of letting the controller driver decide 
> whether this allocation/copy is necessary or not.

Yeah, like that matters in the face of the overhead to queue
the message, get to the head of the SPI transfer queue, go
through that queue, then finally wake up the task that was
synchronously blocking in write_then_read().  Oh, and since
that's inlined, GCC may be re-using existing state...

If folk want an "it looks simple" convenient/friendly API,
there is always a price to pay.  In this case, that cost is
dwarfed by the mere fact that they're using a synchronous
model to access shared resources (the SPI controller).


> >>Then he starts messing with allocate-or-use-preallocated stuff etc. etc.
> >>Why isn't he just kmalloc'ing/kfree'ing buffers each time these 
> >>functions are called 
> >
> >So that the typical case, with little SPI contention, doesn't
> >hit the heap?  That's sure what I thought ... though I can't speak
> >for what other people may think I thought.  You were the one that
> >wanted to optimize the atypical case to remove a blocking path!
>  
> I meant kmalloc'ing/kfree'ing buffers is spi_w8r8/spi_w8r16/etc.

As I said:  so the _typical_ case doesn't hit the heap.   There are
inherent overheads for such RPC-style calls.  But there's also no
point in gratuitously increasing them.

- Dave