On Thu, May 14, 2015 at 09:58:44AM +0000, kernel@xxxxxxxxxxxxxxxx wrote:
> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
>
> Rewrite of spi_map_msg and spi_map_buf so that for SPI_MASTER_MUST_*:
This isn't just rewriting the internals, this is adding a completely new
API with separate code, fixing a bug, optimising the existing API and
adding a user of the new API. I'd have expected all of that to be
called out in the changelog and for it to be split out into several
patches - especially the bug fix since that should go to Linus' tree and
stable.
Like I say I'm not entirely convinced we need the extra flags over just
using can_dma().
> It also fixes the insufficient cleanup in case __spi_map_msg returns
> an error.
This should be a separate patch.
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index ac1760e..ac74456 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -463,7 +463,7 @@ void bcm2835_dma_init(struct spi_master *master, struct device *dev)
> master->can_dma = bcm2835_spi_can_dma;
> master->max_dma_len = 65535; /* limitation by BCM2835_SPI_DLEN */
> /* need to do TX AND RX DMA, so we need dummy buffers */
> - master->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
> + master->flags = SPI_MASTER_MUST_RX_SG | SPI_MASTER_MUST_TX_SG;
>
> return;
>
This is updating a specific driver to use the new API you're adding,
this should be in a separate patch.
> #ifdef CONFIG_HAS_DMA
> +static int spi_map_null(struct spi_master *master, struct device *dev,
> + struct sg_table *sgt,
> + struct sg_table *sgt_template,
> + void *buf,
> + enum dma_data_direction dir)
> +{
It's not obvious to me what this is supposed to do and it looks awfully
like it's duplicating the existing DMA mapping code (but if that's the
case the existing code should be using this not duplicating it so I
guess not). I think it's supposed to be for creating a scatterlist that
repeatedly reused the same dummy page but there's this template thing,
it's getting a buffer passed in and...
> + if (is_vmalloc_addr(buf)) {
> + vm_page = vmalloc_to_page(buf);
> + if (!vm_page) {
> + sg_free_table(sgt);
> + return -ENOMEM;
> + }
> + page_offset = offset_in_page(buf);
> + } else {
> + vm_page = NULL;
> + page_offset = 0;
> + }
...this is really weird for that case.
I'd have expected to be allocating a scatterlist that transfers a
specified length to/from a normal page sized kmalloc() buffer in which
case we don't need to worry about vmalloc() or this template stuff.
> + /*
> + * handle the SPI_MASTER_MUST_*_SG
> + * note that the situation with tx_buf and rx_buf both NULL
> + * is checked and handled inside spi_transfer_one_message
> + */
> + if ((!xfer->tx_buf) && (xfer->rx_buf) &&
> + (master->flags & SPI_MASTER_MUST_TX_SG)) {
> + ret = spi_map_null(master, tx_dev,
> + &xfer->tx_sg, &xfer->rx_sg,
> + master->page_tx,
> + DMA_TO_DEVICE);
> + if (ret != 0) {
> + spi_unmap_buf(master, rx_dev, &xfer->rx_sg,
> + DMA_FROM_DEVICE);
> + return ret;
> + }
> + }
Why does the transmit handling use a scatterlist allocated for receive
(and vice versa)? This goes back to the lack of clarity about what
spi_map_null() is doing.
> if (max_tx) {
> - tmp = krealloc(master->dummy_tx, max_tx,
> - GFP_KERNEL | GFP_DMA);
> - if (!tmp)
> - return -ENOMEM;
> - master->dummy_tx = tmp;
> - memset(tmp, 0, max_tx);
> + if (max_tx > PAGE_SIZE) {
> + tmp_tx = krealloc(master->dummy_tx, max_tx,
> + GFP_KERNEL | GFP_DMA);
> + if (!tmp_tx)
> + return -ENOMEM;
> + master->dummy_tx = tmp_tx;
> + memset(tmp_tx, 0, max_tx);
> + } else {
> + tmp_tx = master->page_tx;
> + }
This idea is fine but should be a separate patch.
I'd expect it should be about as cheap to change the realloc to be the
max of max_tx and PAGE_SIZE (the code was written on the basis that we'd
basically get that behaviour from the allocator anyway most of the
time though IIRC it will work in chunks smaller than a page) but this
does save us the memset() on the transmit path which is handy.
Attachment:
signature.asc
Description: Digital signature
