Re: [PATCH v3 03/16] fpga: mgr: API change to replace fpga load functions with single function

From: Moritz Fischer
Date: Mon Jul 10 2017 - 13:37:04 EST

Hi Alan,

On Thu, Jul 6, 2017 at 11:47 AM, Alan Tull <atull@xxxxxxxxxx> wrote:
> fpga-mgr has three methods for programming FPGAs, depending on
> whether the image is in a scatter gather list, a contiguous
> buffer, or a firmware file. This makes it difficult to write
> upper layers as the caller has to assume whether the FPGA image
> is in a sg table, as a single buffer, or a firmware file.
> This commit moves these parameters to struct fpga_image_info
> and adds a single function for programming fpgas.
>
> New functions:
> * fpga_mgr_load - given fpga manager and struct fpga_image_info,
> program the fpga.
>
> * fpga_image_info_alloc - alloc a struct fpga_image_info.
>
> * fpga_image_info_free - free a struct fpga_image_info.
>
> These three functions are unexported:
> * fpga_mgr_buf_load_sg
> * fpga_mgr_buf_load
> * fpga_mgr_firmware_load
>
> Also use devm_kstrdup to copy firmware_name so we aren't making
> assumptions about where it comes from when allocing/freeing the
> struct fpga_image_info.
>
> Signed-off-by: Alan Tull <atull@xxxxxxxxxx>
> ---
> drivers/fpga/fpga-mgr.c | 59 +++++++++++++++++++++++++++++++++++--------
> drivers/fpga/fpga-region.c | 43 ++++++++++++++++++++-----------
> include/linux/fpga/fpga-mgr.h | 22 ++++++++++------
> 3 files changed, 90 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 188ffef..3478aab 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -2,6 +2,7 @@
> * FPGA Manager Core
> *
> * Copyright (C) 2013-2015 Altera Corporation
> + * Copyright (C) 2017 Intel Corporation
> *
> * With code from the mailing list:
> * Copyright (C) 2013 Xilinx, Inc.
> @@ -31,6 +32,31 @@
> static DEFINE_IDA(fpga_mgr_ida);
> static struct class *fpga_mgr_class;
>
> +struct fpga_image_info *fpga_image_info_alloc(struct device *dev)
> +{
> + struct fpga_image_info *info;
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return ERR_PTR(-ENOMEM);

Doesn't this make it more complex? If in the end you'll anyway have to check
if IS_ERR_OR_NULL()? As opposed to just checking if (!info) on the returned
value.

Just a thought.

> +
> + return info;
> +}
> +EXPORT_SYMBOL_GPL(fpga_image_info_alloc);
> +
> +void fpga_image_info_free(struct device *dev,
> + struct fpga_image_info *info)
> +{
> + if (!info)
> + return;
> +
> + if (info->firmware_name)
> + devm_kfree(dev, info->firmware_name);
> +
> + devm_kfree(dev, info);
> +}
> +EXPORT_SYMBOL_GPL(fpga_image_info_free);
> +
> /*
> * 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
> @@ -137,8 +163,9 @@ static int fpga_mgr_write_complete(struct fpga_manager *mgr,
> *
> * Return: 0 on success, negative error code otherwise.
> */
> -int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info *info,
> - struct sg_table *sgt)
> +static int fpga_mgr_buf_load_sg(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + struct sg_table *sgt)
> {
> int ret;
>
> @@ -170,7 +197,6 @@ int fpga_mgr_buf_load_sg(struct fpga_manager *mgr, struct fpga_image_info *info,
>
> 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,
> @@ -210,8 +236,9 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
> *
> * 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)
> +static 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;
> @@ -266,7 +293,6 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, struct fpga_image_info *info,
>
> return rc;
> }
> -EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
>
> /**
> * fpga_mgr_firmware_load - request firmware and load to fpga
> @@ -282,9 +308,9 @@ EXPORT_SYMBOL_GPL(fpga_mgr_buf_load);
> *
> * Return: 0 on success, negative error code otherwise.
> */
> -int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> - struct fpga_image_info *info,
> - const char *image_name)
> +static int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> + struct fpga_image_info *info,
> + const char *image_name)
> {
> struct device *dev = &mgr->dev;
> const struct firmware *fw;
> @@ -307,7 +333,18 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> +
> +int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
> +{
> + if (info->sgt)
> + return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
> + if (info->buf && info->count)
> + return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
> + if (info->firmware_name)
> + return fpga_mgr_firmware_load(mgr, info, info->firmware_name);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_load);
>
> static const char * const state_str[] = {
> [FPGA_MGR_STATE_UNKNOWN] = "unknown",
> @@ -578,7 +615,7 @@ static void __exit fpga_mgr_class_exit(void)
> ida_destroy(&fpga_mgr_ida);
> }
>
> -MODULE_AUTHOR("Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Alan Tull <atull@xxxxxxxxxx>");
> MODULE_DESCRIPTION("FPGA manager framework");
> MODULE_LICENSE("GPL v2");
>
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 655940f..c71c9cb 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -226,14 +226,11 @@ static int fpga_region_get_bridges(struct fpga_region *region,
> /**
> * fpga_region_program_fpga - program FPGA
> * @region: FPGA region
> - * @firmware_name: name of FPGA image firmware file
> * @overlay: device node of the overlay
> - * Program an FPGA using information in the device tree.
> - * Function assumes that there is a firmware-name property.
> + * Program an FPGA using information in the region's fpga image info.
> * Return 0 for success or negative error code.
> */
> static int fpga_region_program_fpga(struct fpga_region *region,
> - const char *firmware_name,
> struct device_node *overlay)
> {
> struct fpga_manager *mgr;
> @@ -264,7 +261,7 @@ static int fpga_region_program_fpga(struct fpga_region *region,
> goto err_put_br;
> }
>
> - ret = fpga_mgr_firmware_load(mgr, region->info, firmware_name);
> + ret = fpga_mgr_load(mgr, region->info);
> if (ret) {
> pr_err("failed to load fpga image\n");
> goto err_put_br;
> @@ -357,16 +354,15 @@ static int child_regions_with_firmware(struct device_node *overlay)
> static int fpga_region_notify_pre_apply(struct fpga_region *region,
> struct of_overlay_notify_data *nd)
> {
> - const char *firmware_name = NULL;
> + struct device *dev = &region->dev;
> struct fpga_image_info *info;
> + const char *firmware_name;
> int ret;
>
> - info = devm_kzalloc(&region->dev, sizeof(*info), GFP_KERNEL);
> + info = fpga_image_info_alloc(dev);
> if (!info)
> return -ENOMEM;

Can't you return PTR_ERR(info) here (If you don't follow my suggestion above),
or keep it the same here and follow my suggestion ;-)
>
> - region->info = info;
> -
> /* Reject overlay if child FPGA Regions have firmware-name property */
> ret = child_regions_with_firmware(nd->overlay);
> if (ret)
> @@ -382,7 +378,13 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
> if (of_property_read_bool(nd->overlay, "encrypted-fpga-config"))
> info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
>
> - of_property_read_string(nd->overlay, "firmware-name", &firmware_name);
> + if (!of_property_read_string(nd->overlay, "firmware-name",
> + &firmware_name)) {
> + info->firmware_name = devm_kstrdup(dev, firmware_name,
> + GFP_KERNEL);
> + if (!info->firmware_name)
> + return -ENOMEM;
> + }
>
> of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us",
> &info->enable_timeout_us);
> @@ -394,22 +396,33 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region,
> &info->config_complete_timeout_us);
>
> /* If FPGA was externally programmed, don't specify firmware */
> - if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && firmware_name) {
> + if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) {
> pr_err("error: specified firmware and external-fpga-config");
> + fpga_image_info_free(dev, info);
> return -EINVAL;
> }
>
> /* FPGA is already configured externally. We're done. */
> - if (info->flags & FPGA_MGR_EXTERNAL_CONFIG)
> + if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
> + fpga_image_info_free(dev, info);
> return 0;
> + }
>
> /* If we got this far, we should be programming the FPGA */
> - if (!firmware_name) {
> + if (!info->firmware_name) {
> pr_err("should specify firmware-name or external-fpga-config\n");
> + fpga_image_info_free(dev, info);
> return -EINVAL;
> }
>
> - return fpga_region_program_fpga(region, firmware_name, nd->overlay);
> + region->info = info;
> + ret = fpga_region_program_fpga(region, nd->overlay);
> + if (ret) {
> + fpga_image_info_free(dev, info);
> + region->info = NULL;
> + }
> +
> + return ret;
> }
>
> /**
> @@ -426,7 +439,7 @@ static void fpga_region_notify_post_remove(struct fpga_region *region,
> {
> fpga_bridges_disable(&region->bridge_list);
> fpga_bridges_put(&region->bridge_list);
> - devm_kfree(&region->dev, region->info);
> + fpga_image_info_free(&region->dev, region->info);
> region->info = NULL;
> }
>
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index b4ac24c..1c53aa3 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -1,7 +1,8 @@
> /*
> * FPGA Framework
> *
> - * Copyright (C) 2013-2015 Altera Corporation
> + * Copyright (C) 2013-2016 Altera Corporation
> + * Copyright (C) 2017 Intel Corporation
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms and conditions of the GNU General Public License,
> @@ -79,12 +80,20 @@ enum fpga_mgr_states {
> * @disable_timeout_us: maximum time to disable traffic through bridge (uSec)
> * @config_complete_timeout_us: maximum time for FPGA to switch to operating
> * status in the write_complete op.
> + * @firmware_name: name of FPGA image firmware file
> + * @sgt: scatter/gather table containing FPGA image
> + * @buf: contiguous buffer containing FPGA image
> + * @count: size of buf
> */
> struct fpga_image_info {
> u32 flags;
> u32 enable_timeout_us;
> u32 disable_timeout_us;
> u32 config_complete_timeout_us;
> + char *firmware_name;
> + struct sg_table *sgt;
> + const char *buf;
> + size_t count;
> };
>
> /**
> @@ -134,14 +143,11 @@ struct fpga_manager {
>
> #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
>
> -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 fpga_image_info *fpga_image_info_alloc(struct device *dev);
>
> -int fpga_mgr_firmware_load(struct fpga_manager *mgr,
> - struct fpga_image_info *info,
> - const char *image_name);
> +void fpga_image_info_free(struct device *dev, struct fpga_image_info *info);
> +
> +int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>
> struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>
> --
> 2.7.4
>

Cheers,
Moritz