Re: [PATCH v4 2/4] spi: Split spi message into max_dma_len size chunks

[Lukas Wunner <lukas@xxxxxxxxx>]

On Thu, Apr 11, 2019 at 11:02:26PM +0200, Noralf Trønnes wrote:
> Den 11.04.2019 20.18, skrev Lukas Wunner:
> > On Thu, Apr 11, 2019 at 06:42:33PM +0200, Noralf Trønnes wrote:
> >> @@ -1299,6 +1299,11 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
> >>  
> >>  	trace_spi_message_start(ctlr->cur_msg);
> >>  
> >> +	ret = spi_split_transfers_maxsize(ctlr, ctlr->cur_msg, ctlr->max_dma_len,
> >> +					  GFP_KERNEL | GFP_DMA);
> > 
> > Um, shouldn't this be ifdef'ed to CONFIG_HAS_DMA?
> 
> Maybe, I don't know. Mark didn't mentioned it when he commented on a
> previous version of this. Some hate ifdef's and want to avoid them, some
> don't.

The above call is clearly only necessary if DMA is present and
all the DMA-specific functionality in spi.c is ifdef'ed to
CONFIG_HAS_DMA, so the above should likewise only be compiled
in if DMA is present.

Regardless whether this is achieved with an #ifdef or an empty
inline stub for the non-DMA case.


> > > Some drivers like spi_bcm2835 have a max size on DMA transfers. Work
> > > around this by splitting up the transfer if necessary.
> > 
> > This feels like a very expensive solution to the problem:  Large transfers
> > are split into multiple smaller transfers (requiring lots of overhead to
> > allocate and populate the structures) and the split transfers seem to be
> > cached for later reuse.
> > 
> 
> Only drivers that set ->max_dma_len will be split, and only when the
> length is bigger.

The issue on the BCM2385 is that the (non-Lite) DMA channels have a
max length of 1 GByte but the SPI controller has a 65535 byte limit.

There are three other drivers which set max_dma_len, I'm not sure if
they suffer from the same issue as the BCM2835, but this patch causes
additional overhead for them so the driver maintainers / main authors
should at least be cc'ed.


> I can't see any caching with a quick glance, where do you see it?

Misunderstanding of the code on my part, sorry.  I thought
spi_res_release() wouldn't be called until the spi_message is freed,
in which case the split transfers would be kept around (cached) for
retransmissions of the same message.  But I notice now that
spi_res_release() is already called upon finishing the first
transmission of the spi_message, hence the split transfers are gone
and need to be reconstructed on retransmission.


> > Note that spi_map_buf() already splits every transfer's sglist into
> > segments that are smaller than ctlr->max_dma_len.  Now all that needs
> > to be done is to amend spi-bcm2835.c to iterate over the sglist
> > and transmit it in portions which do not exceed 65535.  Addressing the
> > problem at this lower level would drastically reduce the overhead
> > compared to the approach in the present patch and hence appears to be
> > more recommendable.
> 
> In a previous version of this I suggested to Meghana to put this in the
> driver, but Mark wanted it in the core.

Do you have a link to these comments of Mark?  The first version of
this patchset that I have here is v2 of March 2018 and it already
uses spi_split_transfers_maxsize().  I've never seen a version which
splits the sglist in spi-bcm2835.c (instead of splitting the transfers
in spi.c, which, again, is significantly more expensive).

Thanks,

Lukas