Hi Alan, On Thu, Jul 6, 2017 at 11:47 AM, Alan Tull <at...@kernel.org> 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 <at...@kernel.org> > --- > 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 <at...@opensource.altera.com>"); > +MODULE_AUTHOR("Alan Tull <at...@kernel.org>"); > 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 = ®ion->dev; > struct fpga_image_info *info; > + const char *firmware_name; > int ret; > > - info = devm_kzalloc(®ion->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(®ion->bridge_list); > fpga_bridges_put(®ion->bridge_list); > - devm_kfree(®ion->dev, region->info); > + fpga_image_info_free(®ion->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