Re: Issue with spi_map_buf

[Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>]

Hello Mark,

Le Thu, 1 Mar 2018 20:28:02 +0000,
Mark Brown <broonie@xxxxxxxxxx> a écrit :

> On Thu, Jan 25, 2018 at 03:47:54PM +0100, Maxime Chevallier wrote:

I changed email address since I sent the first email, I'm responding
with this one.

> > The issue boils down to this (for the sake of clarity, I simplified
> > the function by taking the following assumptions :  
> 
> > - We work on a vmalloced buffer
> > - max_seg_size < PAGE_SIZE
> > - len > max_seg_size (len is size of the buffer we want to split)  
> 
> ...
> 
> > 	min = min_t(size_t,
> > 		    len, desc_len - offset_in_page(buf));  
> 
> > The 'min' variable computation seems incorrect when max_seg_size is
> > less than PAGE_SIZE, in particular the "desc_len -
> > offset_in_page(buf)", which should be something like "(len %
> > PAGE_SIZE) - offset_in_page(buf)"  
> 
> If I'm understanding you correctly the issue you're identifying here
> is that we ignore max_seg_size here and instead just use PAGE_SIZE
> (you don't explicitly say what you think is wrong so I might be
> missing something here).

Sorry for being unclear. The issue seems to be that this 'min' var,
which represent the size of the next scatterlist entry, is IMHO not
correctly computed.

As of today, it's the minimum between the remaining length of the
buffer to be split, and the max length of a scatterlist entry minus the
offset in page.

For example, lets assume that desc_len is 512 (I'll get back on wether
or not it's pertinent to have such a small value), a buffer
that is 2048 bytes long, and that we are working on a buffer that starts
with an offset in page of 512.

the first scatterlist entry will actually have a 0 length (desc_len -
offset_in_page == 0).

I think that the right way to compute that value would be the minimum
between the desc_len and the remaining part that would fit in the page :

        min(desc_len, min(len, (PAGE_SIZE - offset_in_page(buf)))

But again, I also could miss something here, hence why I bring that
topic on this list :)

> > Unfortunately, this start to be complex if we want to take all
> > possible cases into account : If we want to split a buffer in
> > segments, where some might cross page boundaries, how do we know
> > how much sg elements will there be ?  
> 
> > I thought about using something like  __sg_alloc_table_from_pages,
> > but the "max_segment" parameter is expected to be page aligned,
> > which is not the case here.  
> 
> Perhaps I'm missing something here but surely we just need to handle
> the case where desc_len is bigger than offset_in_page()?  The number
> of scatterlist entries we're going to need is at most
> DIV_ROUND_UP(PAGE_SIZE / max_seg_size) * DIV_ROUND_UP(len /
> PAGE_SIZE) (ie, the number of segments we need per page multiplied by
> the number of pages covered by the data) which it looks like we
> already have mostly handled.

What worried me at the time was "what if we can't fill a page with
exactly N max_seg_size", meaning that max_seg_size doesn't divide
PAGE_SIZE. After some thoughts I think this won't occur very often,
I think I overthought that.

So yes you're right about this.

> > I found this issue with a prototype implementation of SPI DMA xfers
> > on armada 3700, using fpga-pgr as a spi device. In my case,
> > max_seg_size is set to 512 bytes.  
> 
> Like I say I might be missing something here.
> 
> > This should occur with other DMA / SPI controllers, as long as the
> > max_seg_size is less than PAGE_SIZE.  
> 
> I'm not sure how likely that is TBH - 512 is an extremely low limit
> for a DMA controller.

Yes I agree with you on that, I don't think this happens often. I found
this issue because of a HW bug that forced me to use 512 bytes as a max
DMA transfer size, while waiting for a fix, this is clearly not a
common use-case.

Thanks for your answer,

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