Re: [PATCH] spi: SPI_MASTER_MUST_* with scatter-gather only option and avoiding realloc

[Martin Sperl <kernel@xxxxxxxxxxxxxxxx>]

> On 19.05.2015, at 14:46, Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
> Like I say I'm not entirely convinced we need the extra flags over just
> using can_dma().
If you can tell me that:
	(master->flags & (SPI_MASTER_MUST_TX | SPI_MASTER_MUST_RX)) &&
	master->can_dma && master->can_dma(...) 
means that we do NOT need real memory allocated in the first place but
we only expect a scatter list to be created 
and memory allocated in all other cases then that be it.

I am just not convinced that this is necessarily true for all
existing (or future) drivers.

During my discovery questions you mentioned that for some drivers
it may not be as simple as
  tx = (xfer->tx_buf) ? xfer->tx_buf[i] : 0;

So this “extension” of the API is to avoid the potential risk of
breaking other drivers by making the above assumption (with
additional calls to can_dma, which then we should start to cache)

> 
>> It also fixes the insufficient cleanup in case __spi_map_msg returns
>> an error.
> 
> This should be a separate patch.

The problem is that this came up while developing the patch, so
I left it in the patch, assuming there would be some feedback
that requires another version of the patch...

> This is updating a specific driver to use the new API you're adding,
> this should be in a separate patch.

At one occasion I got scolded for separating things out into different
patches (in that case api from implemetation), now the other way around,
but we can do that...

> 
>> #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…

OK, so the idea is that this situation ONLY happens when we have either
tx_buf or rx_buf for which we need to map the other transfer from a
“dummy/null” page.

So maybe instead of spi_map_null it should get called spi_map_dummy.

The template is there to create an (almost) identical scatter lists for
both rx and tx (modulo PAGE_SIZE). So if we have a tx-scatter list of:
4, 8, 4103 bytes, and we need to do a dummy transfer for rx, then the
(basic) identical pattern of 4, 8, 4096, 7 would get created.

This kind avoids some restrictions that I have seen with the spi-bcm2835
hardware.

As for replication - the code is similar in some aspects,
but still serves a different purpose (especially with the templating).

Adding all that into one would mean actually merging tx and rx cases
into one piece of code, and I did not want to go that far.
Keep what is working.

Also if we would do that then we should also add additional features
like creating a list of scatter lists for some cases, when:
* an individual dma transfer to the HW can only be < 65536 bytes in
  length (the spi HW got a counter)
* or when individual entries in the scatter list are not a multiple
  of 4
we would need to add multiple sg lists and handle each one separately.
(again examples from spi-bcm2835)

This would be a much bigger change.

> 
>> +	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.

vm_page actually serves as a flag what call should get used a little
further down - sg_set_page (in case of not null) or sg_set_buf.

This is there as I am not 100% sure that the allocation used as
buffer is ALWAYS of the same identical type on _all_ platforms
in all .config variants (and I want to avoid breaking things...

If you can tell me that it is always of one type (even when used
early during kernel initialization), then we can remove that.

> 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.

See above about “templating"

> 
>> 		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.

Well - this comes mostly from the fact that we now have a PAGE that
is cleaned already (and we need it for mapping anyway), so 
just make use of it as long as the size is small enough, which
probably happens in most cases.

Martin

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html