Re: [PATCH 1/3] mtd: spi-nor: add optional DMA-safe bounce buffer for data transfer

[Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>]

On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote:
> 
> Then the patch adds two hardware capabilities for SPI flash controllers,
> SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE.

Are there any drivers for which a bounce buffer is NOT needed when the
tx/rx buffer is not in DMA safe memory?  Maybe it would make more sense
to invert the sense of these flags, so that they indicate the driver
does not need DMA safe buffers, if that is the uncommon/non-existent
case, so that fewer drivers need to be modified to to be fixed?

> +static bool spi_nor_is_dma_safe(const void *buf)
> +{
> +	if (is_vmalloc_addr(buf))
> +		return false;
> +
> +#ifdef CONFIG_HIGHMEM
> +	if ((unsigned long)buf >= PKMAP_BASE &&
> +	    (unsigned long)buf < (PKMAP_BASE + (LAST_PKMAP * PAGE_SIZE)))
> +		return false;
> +#endif

It looks like:

(unsigned long)addr >= PKMAP_ADDR(0) &&
(unsigned long)addr < PKMAP_ADDR(LAST_PKMAP)

is the expression used in the highmem code.  But really, isn't this
begging for is_highmem_addr() in include/linux/mm.h that can always
return false when highmem is not enabled?

In order to be safe, this must be called when nor->lock is held, right?
 Otherwise it could race against two callers allocating the buffer at
the same time.  That should probably be noted in the kerneldoc comments
for this function, which should also be written.

> +static int spi_nor_get_bounce_buffer(struct spi_nor *nor,
> +				     u_char **buffer,
> +				     size_t *buffer_size)
> +{

> +
> +	*buffer = nor->bounce_buffer;
> +	*buffer_size = size;

So the buffer is returned via the parameter, and also via a field
inside nor.  Seems redundant.  Consider address could be returned via
the function return value coupled with PTR_ERR() for the error cases. 
Or not return address at all since it's available via nor-
>bounce_buffer.

>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	bool use_bounce = (nor->flags & SNOR_F_USE_RD_BOUNCE) &&
> +			  !spi_nor_is_dma_safe(buf);
> +	u_char *buffer = buf;
> +	size_t buffer_size = 0;
>  	int ret;
>  
>  	dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
> @@ -1268,13 +1324,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	if (ret)
>  		return ret;
>  
> +	if (use_bounce) {
> +		ret = spi_nor_get_bounce_buffer(nor, &buffer, &buffer_size);
> +		if (ret < 0)
> +			goto read_err;
> +	}

This pattern, check if bounce is enabled, check if address is dma-
unsafe, get bounce buffer, seems to be very common.  Could it be
refactored into one helper?

u_char *buffer = spi_nor_check_bounce(nor, buf, len, &buffer_size);
if (IS_ERR(buffer)) 
    return PTR_ERR(buffer);
// buffer = nor->bounce_buffer or buf, whichever is correct
// buffer_size = len or bounce buffer size, whichever is correct

Could spi_nor_read_sfdp_dma_unsafe() also use this buffer?