On Fri, 6 Jan 2017, Jason Gunthorpe wrote:
> Requiring contiguous kernel memory is not a good idea, this is a limited
> resource and allocation can fail under normal work loads.
>
> This introduces a .write_sg op that supporting drivers can provide
> to DMA directly from dis-contiguous memory and a new entry point
> fpga_mgr_buf_load_sg that users can call to directly provide page
> lists.
>
> The full matrix of compatibility is provided, either the linear or sg
> interface can be used by the user with a driver supporting either
> interface.
>
> A notable change for drivers is that the .write op can now be called
> multiple times.
>
> Signed-off-by: Jason Gunthorpe <jguntho...@obsidianresearch.com>
> ---
> Documentation/fpga/fpga-mgr.txt | 19 +++-
> drivers/fpga/fpga-mgr.c | 238
> ++++++++++++++++++++++++++++++++++------
> include/linux/fpga/fpga-mgr.h | 5 +
> 3 files changed, 228 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index 86ee5078fd034f..78f197fadfd1b6 100644
> --- a/Documentation/fpga/fpga-mgr.txt
> +++ b/Documentation/fpga/fpga-mgr.txt
> @@ -22,7 +22,16 @@ To program the FPGA from a file or from a buffer:
> struct fpga_image_info *info,
> const char *buf, size_t count);
>
> -Load the FPGA from an image which exists as a buffer in memory.
> +Load the FPGA from an image which exists as a contiguous buffer in
> +memory. Allocating contiguous kernel memory for the buffer should be avoided,
> +users are encouraged to use the _sg interface instead of this.
> +
> + int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + struct sg_table *sgt);
> +
> +Load the FPGA from an image from non-contiguous in memory. Callers can
> +construct a sg_table using alloc_page backed memory.
>
> int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> struct fpga_image_info *info,
> @@ -166,7 +175,7 @@ success or negative error codes otherwise.
>
> The programming sequence is:
> 1. .write_init
> - 2. .write (may be called once or multiple times)
> + 2. .write or .write_sg (may be called once or multiple times)
> 3. .write_complete
>
> The .write_init function will prepare the FPGA to receive the image data.
> The
> @@ -176,7 +185,11 @@ buffer up at least this much before starting.
>
> The .write function writes a buffer to the FPGA. The buffer may be contain
> the
> whole FPGA image or may be a smaller chunk of an FPGA image. In the latter
> -case, this function is called multiple times for successive chunks.
> +case, this function is called multiple times for successive chunks. This
> interface
> +is suitable for drivers which use PIO.
> +
> +The .write_sg version behaves the same as .write except the input is a
> sg_table
> +scatter list. This interface is suitable for drivers which use DMA.
>
> The .write_complete function is called after all the image has been written
> to put the FPGA into operating mode.
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index f0a69d3e60a584..30f9778d0632d2 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -1,4 +1,4 @@
> -/*
> + /*
Hi Jason,
Need to take these added 2 spaces out. Otherwise,
Acked-by: Alan Tull <at...@opensource.altera.com>
Thank you for adding this functionality.
Alan
> * FPGA Manager Core
> *
> * Copyright (C) 2013-2015 Altera Corporation
> @@ -25,16 +25,106 @@
> #include <linux/of.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> +#include <linux/scatterlist.h>
> +#include <linux/highmem.h>
>
> static DEFINE_IDA(fpga_mgr_ida);
> static struct class *fpga_mgr_class;
>
> +/*
> + * Call the low level driver's write_init function. This will do the
> + * device-specific things to get the FPGA into the state where it is ready to
> + * receive an FPGA image. The low level driver only gets to see the first
> + * initial_header_size bytes in the buffer.
> + */
> +static int fpga_mgr_write_init_buf(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + int ret;
> +
> + mgr->state = FPGA_MGR_STATE_WRITE_INIT;
> + if (!mgr->mops->initial_header_size)
> + ret = mgr->mops->write_init(mgr, info, NULL, 0);
> + else
> + ret = mgr->mops->write_init(
> + mgr, info, buf, min(mgr->mops->initial_header_size, count));
> +
> + if (ret) {
> + dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
> + mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int fpga_mgr_write_init_sg(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + struct sg_table *sgt)
> +{
> + struct sg_mapping_iter miter;
> + size_t len;
> + char *buf;
> + int ret;
> +
> + if (!mgr->mops->initial_header_size)
> + return fpga_mgr_write_init_buf(mgr, info, NULL, 0);
> +
> + /*
> + * First try to use miter to map the first fragment to access the
> + * header, this is the typical path.
> + */
> + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> + if (sg_miter_next(&miter) &&
> + miter.length >= mgr->mops->initial_header_size) {
> + ret = fpga_mgr_write_init_buf(mgr, info, miter.addr,
> + miter.length);
> + sg_miter_stop(&miter);
> + return ret;
> + }
> + sg_miter_stop(&miter);
> +
> + /* Otherwise copy the fragments into temporary memory. */
> + buf = kmalloc(mgr->mops->initial_header_size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + len = sg_copy_to_buffer(sgt->sgl, sgt->nents, buf,
> + mgr->mops->initial_header_size);
> + ret = fpga_mgr_write_init_buf(mgr, info, buf, len);
> +
> + kfree(buf);
> +
> + return ret;
> +}
> +
> +/*
> + * After all the FPGA image has been written, do the device specific steps to
> + * finish and set the FPGA into operating mode.
> + */
> +static int fpga_mgr_write_complete(struct fpga_manager *mgr,
> + struct fpga_image_info *info)
> +{
> + int ret;
> +
> + mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
> + ret = mgr->mops->write_complete(mgr, info);
> + if (ret) {
> + dev_err(&mgr->dev, "Error after writing image data to FPGA\n");
> + mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
> + return ret;
> + }
> + mgr->state = FPGA_MGR_STATE_OPERATING;
> +
> + return 0;
> +}
> +
> /**
> - * fpga_mgr_buf_load - load fpga from image in buffer
> + * fpga_mgr_buf_load_sg - load fpga from image in buffer from a scatter list
> * @mgr: fpga manager
> * @info: fpga image specific information
> - * @buf: buffer contain fpga image
> - * @count: byte count of buf
> + * @sgt: scatterlist table
> *
> * Step the low level fpga manager through the device-specific steps of
> getting
> * an FPGA ready to be configured, writing the image to it, then doing
> whatever
> @@ -42,54 +132,139 @@ static struct class *fpga_mgr_class;
> * mgr pointer from of_fpga_mgr_get() or fpga_mgr_get() and checked that it
> is
> * not an error code.
> *
> + * This is the preferred entry point for FPGA programming, it does not
> require
> + * any contiguous kernel memory.
> + *
> * Return: 0 on success, negative error code otherwise.
> */
> -int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
> - const char *buf, size_t count)
> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info
> *info,
> + struct sg_table *sgt)
> {
> - struct device *dev = &mgr->dev;
> int ret;
>
> - /*
> - * Call the low level driver's write_init function. This will do the
> - * device-specific things to get the FPGA into the state where it is
> - * ready to receive an FPGA image. The low level driver only gets to
> - * see the first initial_header_size bytes in the buffer.
> - */
> - mgr->state = FPGA_MGR_STATE_WRITE_INIT;
> - ret = mgr->mops->write_init(mgr, info, buf,
> - min(mgr->mops->initial_header_size, count));
> + ret = fpga_mgr_write_init_sg(mgr, info, sgt);
> + if (ret)
> + return ret;
> +
> + /* Write the FPGA image to the FPGA. */
> + mgr->state = FPGA_MGR_STATE_WRITE;
> + if (mgr->mops->write_sg) {
> + ret = mgr->mops->write_sg(mgr, sgt);
> + } else {
> + struct sg_mapping_iter miter;
> +
> + sg_miter_start(&miter, sgt->sgl, sgt->nents, SG_MITER_FROM_SG);
> + while (sg_miter_next(&miter)) {
> + ret = mgr->mops->write(mgr, miter.addr, miter.length);
> + if (ret)
> + break;
> + }
> + sg_miter_stop(&miter);
> + }
> +
> if (ret) {
> - dev_err(dev, "Error preparing FPGA for writing\n");
> - mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
> + dev_err(&mgr->dev, "Error while writing image data to FPGA\n");
> + mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> return ret;
> }
>
> + return fpga_mgr_write_complete(mgr, info);
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_buf_load_sg);
> +
> +static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + int ret;
> +
> + ret = fpga_mgr_write_init_buf(mgr, info, buf, count);
> + if (ret)
> + return ret;
> +
> /*
> * Write the FPGA image to the FPGA.
> */
> mgr->state = FPGA_MGR_STATE_WRITE;
> ret = mgr->mops->write(mgr, buf, count);
> if (ret) {
> - dev_err(dev, "Error while writing image data to FPGA\n");
> + dev_err(&mgr->dev, "Error while writing image data to FPGA\n");
> mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> return ret;
> }
>
> + return fpga_mgr_write_complete(mgr, info);
> +}
> +
> +/**
> + * fpga_mgr_buf_load - load fpga from image in buffer
> + * @mgr: fpga manager
> + * @flags: flags setting fpga confuration modes
> + * @buf: buffer contain fpga image
> + * @count: byte count of buf
> + *
> + * Step the low level fpga manager through the device-specific steps of
> getting
> + * an FPGA ready to be configured, writing the image to it, then doing
> whatever
> + * post-configuration steps necessary. This code assumes the caller got the
> + * mgr pointer from of_fpga_mgr_get() and checked that it is not an error
> code.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
> + const char *buf, size_t count)
> +{
> + struct page **pages;
> + struct sg_table sgt;
> + const void *p;
> + int nr_pages;
> + int index;
> + int rc;
> +
> /*
> - * After all the FPGA image has been written, do the device specific
> - * steps to finish and set the FPGA into operating mode.
> + * This is just a fast path if the caller has already created a
> + * contiguous kernel buffer and the driver doesn't require SG, non-SG
> + * drivers will still work on the slow path.
> */
> - mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
> - ret = mgr->mops->write_complete(mgr, info);
> - if (ret) {
> - dev_err(dev, "Error after writing image data to FPGA\n");
> - mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
> - return ret;
> + if (mgr->mops->write)
> + return fpga_mgr_buf_load_mapped(mgr, info, buf, count);
> +
> + /*
> + * Convert the linear kernel pointer into a sg_table of pages for use
> + * by the driver.
> + */
> + nr_pages = DIV_ROUND_UP((unsigned long)buf + count, PAGE_SIZE) -
> + (unsigned long)buf / PAGE_SIZE;
> + pages = kmalloc_array(nr_pages, sizeof(struct page *), GFP_KERNEL);
> + if (!pages)
> + return -ENOMEM;
> +
> + p = buf - offset_in_page(buf);
> + for (index = 0; index < nr_pages; index++) {
> + if (is_vmalloc_addr(p))
> + pages[index] = vmalloc_to_page(p);
> + else
> + pages[index] = kmap_to_page((void *)p);
> + if (!pages[index]) {
> + kfree(pages);
> + return -EFAULT;
> + }
> + p += PAGE_SIZE;
> }
> - mgr->state = FPGA_MGR_STATE_OPERATING;
>
> - return 0;
> + /*
> + * The temporary pages list is used to code share the merging algorithm
> + * in sg_alloc_table_from_pages
> + */
> + rc = sg_alloc_table_from_pages(&sgt, pages, index, offset_in_page(buf),
> + count, GFP_KERNEL);
> + kfree(pages);
> + if (rc)
> + return rc;
> +
> + rc = fpga_mgr_buf_load_sg(mgr, info, &sgt);
> + sg_free_table(&sgt);
> +
> + return rc;
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
>
> @@ -291,8 +466,9 @@ int fpga_mgr_register(struct device *dev, const char
> *name,
> struct fpga_manager *mgr;
> int id, ret;
>
> - if (!mops || !mops->write_init || !mops->write ||
> - !mops->write_complete || !mops->state) {
> + if (!mops || !mops->write_complete || !mops->state ||
> + !mops->write_init || (!mops->write && !mops->write_sg) ||
> + (mops->write && mops->write_sg)) {
> dev_err(dev, "Attempt to register without fpga_manager_ops\n");
> return -EINVAL;
> }
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 16551d5eac36a7..57beb5d09bfcb2 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -22,6 +22,7 @@
> #define _LINUX_FPGA_MGR_H
>
> struct fpga_manager;
> +struct sg_table;
>
> /**
> * enum fpga_mgr_states - fpga framework states
> @@ -88,6 +89,7 @@ struct fpga_image_info {
> * @state: returns an enum value of the FPGA's state
> * @write_init: prepare the FPGA to receive confuration data
> * @write: write count bytes of configuration data to the FPGA
> + * @write_sg: write the scatter list of configuration data to the FPGA
> * @write_complete: set FPGA to operating state after writing is done
> * @fpga_remove: optional: Set FPGA into a specific state during driver
> remove
> *
> @@ -102,6 +104,7 @@ struct fpga_manager_ops {
> struct fpga_image_info *info,
> const char *buf, size_t count);
> int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> + int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
> int (*write_complete)(struct fpga_manager *mgr,
> struct fpga_image_info *info);
> void (*fpga_remove)(struct fpga_manager *mgr);
> @@ -129,6 +132,8 @@ struct fpga_manager {
>
> int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
> const char *buf, size_t count);
> +int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info
> *info,
> + struct sg_table *sgt);
>
> int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> struct fpga_image_info *info,
> --
> 2.7.4
>
>
