From 5cf0c7f6502f26332b46fa87914553a4d6ae75ac Mon Sep 17 00:00:00 2001 From: Alan Tull Date: Wed, 15 Nov 2017 14:20:12 -0600 Subject: fpga: mgr: API change to replace fpga load functions with single function 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. API documentation has been updated and a new document for FPGA region has been added. Signed-off-by: Alan Tull Acked-by: Moritz Fischer Signed-off-by: Greg Kroah-Hartman --- drivers/fpga/fpga-mgr.c | 68 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 11 deletions(-) (limited to 'drivers/fpga/fpga-mgr.c') diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index 188ffefa3cc3..a8dd54945470 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,40 @@ 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; + + get_device(dev); + + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); + if (!info) { + put_device(dev); + return NULL; + } + + info->dev = dev; + + return info; +} +EXPORT_SYMBOL_GPL(fpga_image_info_alloc); + +void fpga_image_info_free(struct fpga_image_info *info) +{ + struct device *dev; + + if (!info) + return; + + dev = info->dev; + if (info->firmware_name) + devm_kfree(dev, info->firmware_name); + + devm_kfree(dev, info); + put_device(dev); +} +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 +172,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 +206,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 +245,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 +302,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 +317,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 +342,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 +624,7 @@ static void __exit fpga_mgr_class_exit(void) ida_destroy(&fpga_mgr_ida); } -MODULE_AUTHOR("Alan Tull "); +MODULE_AUTHOR("Alan Tull "); MODULE_DESCRIPTION("FPGA manager framework"); MODULE_LICENSE("GPL v2"); -- cgit v1.2.3 From ebf877a51ad7b65e4ab024f021b60a4f7928864a Mon Sep 17 00:00:00 2001 From: Alan Tull Date: Wed, 15 Nov 2017 14:20:13 -0600 Subject: fpga: mgr: separate getting/locking FPGA manager Previously when the user gets a FPGA manager, it was locked and nobody else could use it for programming. This commit makes it straightforward to save a reference to an FPGA manager and only lock it when programming the FPGA. Add functions that get an FPGA manager's mutex for exclusive use: * fpga_mgr_lock * fpga_mgr_unlock The following functions no longer lock an FPGA manager's mutex: * of_fpga_mgr_get * fpga_mgr_get * fpga_mgr_put Signed-off-by: Alan Tull Acked-by: Moritz Fischer Signed-off-by: Greg Kroah-Hartman --- Documentation/fpga/fpga-mgr.txt | 35 ++++++++++++++++++++------- drivers/fpga/fpga-mgr.c | 52 ++++++++++++++++++++++++++++------------- drivers/fpga/fpga-region.c | 14 +++++++++-- include/linux/fpga/fpga-mgr.h | 3 +++ 4 files changed, 77 insertions(+), 27 deletions(-) (limited to 'drivers/fpga/fpga-mgr.c') diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt index 6ebc714f4b03..cc6413ed6fc9 100644 --- a/Documentation/fpga/fpga-mgr.txt +++ b/Documentation/fpga/fpga-mgr.txt @@ -48,8 +48,20 @@ To get/put a reference to a FPGA manager: struct fpga_manager *fpga_mgr_get(struct device *dev); void fpga_mgr_put(struct fpga_manager *mgr); -Given a DT node or device, get an exclusive reference to a FPGA manager. -fpga_mgr_put releases the reference. +Given a DT node or device, get a reference to a FPGA manager. This pointer +can be saved until you are ready to program the FPGA. fpga_mgr_put releases +the reference. + + +To get exclusive control of a FPGA manager: +------------------------------------------- + + int fpga_mgr_lock(struct fpga_manager *mgr); + void fpga_mgr_unlock(struct fpga_manager *mgr); + +The user should call fpga_mgr_lock and verify that it returns 0 before +attempting to program the FPGA. Likewise, the user should call +fpga_mgr_unlock when done programming the FPGA. To register or unregister the low level FPGA-specific driver: @@ -67,13 +79,21 @@ device." How to write an image buffer to a supported FPGA ================================================ -/* Include to get the API */ #include struct fpga_manager *mgr; struct fpga_image_info *info; int ret; +/* + * Get a reference to FPGA manager. The manager is not locked, so you can + * hold onto this reference without it preventing programming. + * + * This example uses the device node of the manager. Alternatively, use + * fpga_mgr_get(dev) instead if you have the device. + */ +mgr = of_fpga_mgr_get(mgr_node); + /* struct with information about the FPGA image to program. */ info = fpga_image_info_alloc(dev); @@ -99,17 +119,14 @@ if (image is in a scatter gather table) { } -/* - * Get a reference to FPGA manager. This example uses the device node of the - * manager. You could use fpga_mgr_get() instead if you have the device instead - * of the device node. - */ -mgr = of_fpga_mgr_get(mgr_node); +/* Get exclusive control of FPGA manager */ +ret = fpga_mgr_lock(mgr); /* Load the buffer to the FPGA */ ret = fpga_mgr_buf_load(mgr, &info, buf, count); /* Release the FPGA manager */ +fpga_mgr_unlock(mgr); fpga_mgr_put(mgr); /* Deallocate the image info if you're done with it */ diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index a8dd54945470..d27e8d2a149c 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -410,28 +410,19 @@ ATTRIBUTE_GROUPS(fpga_mgr); static struct fpga_manager *__fpga_mgr_get(struct device *dev) { struct fpga_manager *mgr; - int ret = -ENODEV; mgr = to_fpga_manager(dev); if (!mgr) goto err_dev; - /* Get exclusive use of fpga manager */ - if (!mutex_trylock(&mgr->ref_mutex)) { - ret = -EBUSY; - goto err_dev; - } - if (!try_module_get(dev->parent->driver->owner)) - goto err_ll_mod; + goto err_dev; return mgr; -err_ll_mod: - mutex_unlock(&mgr->ref_mutex); err_dev: put_device(dev); - return ERR_PTR(ret); + return ERR_PTR(-ENODEV); } static int fpga_mgr_dev_match(struct device *dev, const void *data) @@ -440,10 +431,10 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data) } /** - * fpga_mgr_get - get an exclusive reference to a fpga mgr + * fpga_mgr_get - get a reference to a fpga mgr * @dev: parent device that fpga mgr was registered with * - * Given a device, get an exclusive reference to a fpga mgr. + * Given a device, get a reference to a fpga mgr. * * Return: fpga manager struct or IS_ERR() condition containing error code. */ @@ -464,10 +455,10 @@ static int fpga_mgr_of_node_match(struct device *dev, const void *data) } /** - * of_fpga_mgr_get - get an exclusive reference to a fpga mgr + * of_fpga_mgr_get - get a reference to a fpga mgr * @node: device node * - * Given a device node, get an exclusive reference to a fpga mgr. + * Given a device node, get a reference to a fpga mgr. * * Return: fpga manager struct or IS_ERR() condition containing error code. */ @@ -491,11 +482,40 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get); void fpga_mgr_put(struct fpga_manager *mgr) { module_put(mgr->dev.parent->driver->owner); - mutex_unlock(&mgr->ref_mutex); put_device(&mgr->dev); } EXPORT_SYMBOL_GPL(fpga_mgr_put); +/** + * fpga_mgr_lock - Lock FPGA manager for exclusive use + * @mgr: fpga manager + * + * Given a pointer to FPGA Manager (from fpga_mgr_get() or + * of_fpga_mgr_put()) attempt to get the mutex. + * + * Return: 0 for success or -EBUSY + */ +int fpga_mgr_lock(struct fpga_manager *mgr) +{ + if (!mutex_trylock(&mgr->ref_mutex)) { + dev_err(&mgr->dev, "FPGA manager is in use.\n"); + return -EBUSY; + } + + return 0; +} +EXPORT_SYMBOL_GPL(fpga_mgr_lock); + +/** + * fpga_mgr_unlock - Unlock FPGA manager + * @mgr: fpga manager + */ +void fpga_mgr_unlock(struct fpga_manager *mgr) +{ + mutex_unlock(&mgr->ref_mutex); +} +EXPORT_SYMBOL_GPL(fpga_mgr_unlock); + /** * fpga_mgr_register - register a low level fpga manager driver * @dev: fpga manager device from pdev diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 120c496eb7bd..1e1640a29306 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -125,7 +125,7 @@ static void fpga_region_put(struct fpga_region *region) } /** - * fpga_region_get_manager - get exclusive reference for FPGA manager + * fpga_region_get_manager - get reference for FPGA manager * @region: FPGA region * * Get FPGA Manager from "fpga-mgr" property or from ancestor region. @@ -233,6 +233,7 @@ static int fpga_region_get_bridges(struct fpga_region *region, static int fpga_region_program_fpga(struct fpga_region *region, struct device_node *overlay) { + struct device *dev = ®ion->dev; struct fpga_manager *mgr; int ret; @@ -249,10 +250,16 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_region; } + ret = fpga_mgr_lock(mgr); + if (ret) { + dev_err(dev, "FPGA manager is busy\n"); + goto err_put_mgr; + } + ret = fpga_region_get_bridges(region, overlay); if (ret) { pr_err("failed to get fpga region bridges\n"); - goto err_put_mgr; + goto err_unlock_mgr; } ret = fpga_bridges_disable(®ion->bridge_list); @@ -273,6 +280,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } + fpga_mgr_unlock(mgr); fpga_mgr_put(mgr); fpga_region_put(region); @@ -280,6 +288,8 @@ static int fpga_region_program_fpga(struct fpga_region *region, err_put_br: fpga_bridges_put(®ion->bridge_list); +err_unlock_mgr: + fpga_mgr_unlock(mgr); err_put_mgr: fpga_mgr_put(mgr); err_put_region: diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index 6b791348023b..cb5615c87504 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -155,6 +155,9 @@ void fpga_image_info_free(struct fpga_image_info *info); int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info); +int fpga_mgr_lock(struct fpga_manager *mgr); +void fpga_mgr_unlock(struct fpga_manager *mgr); + struct fpga_manager *of_fpga_mgr_get(struct device_node *node); struct fpga_manager *fpga_mgr_get(struct device *dev); -- cgit v1.2.3 From 845089bbf589be75143d0c9fb326d5595c1b5787 Mon Sep 17 00:00:00 2001 From: Alan Tull Date: Wed, 15 Nov 2017 14:20:28 -0600 Subject: fpga: add attribute groups Make it easy to add attributes to low level FPGA drivers the right way. Add attribute groups pointers to structures that are used when registering a manager, bridge, or group. When the low level driver registers, set the device attribute group. The attributes are created in device_add. Signed-off-by: Alan Tull Acked-by: Moritz Fischer Signed-off-by: Greg Kroah-Hartman --- drivers/fpga/fpga-bridge.c | 1 + drivers/fpga/fpga-mgr.c | 1 + drivers/fpga/fpga-region.c | 1 + include/linux/fpga/fpga-bridge.h | 2 ++ include/linux/fpga/fpga-mgr.h | 2 ++ include/linux/fpga/fpga-region.h | 2 ++ 6 files changed, 9 insertions(+) (limited to 'drivers/fpga/fpga-mgr.c') diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c index 0dfe9d78cee2..e693c3607a14 100644 --- a/drivers/fpga/fpga-bridge.c +++ b/drivers/fpga/fpga-bridge.c @@ -367,6 +367,7 @@ int fpga_bridge_register(struct device *dev, const char *name, bridge->priv = priv; device_initialize(&bridge->dev); + bridge->dev.groups = br_ops->groups; bridge->dev.class = fpga_bridge_class; bridge->dev.parent = dev; bridge->dev.of_node = dev->of_node; diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index d27e8d2a149c..223f2401939b 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -569,6 +569,7 @@ int fpga_mgr_register(struct device *dev, const char *name, device_initialize(&mgr->dev); mgr->dev.class = fpga_mgr_class; + mgr->dev.groups = mops->groups; mgr->dev.parent = dev; mgr->dev.of_node = dev->of_node; mgr->dev.id = id; diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index afc61885a601..edab2a2e03ef 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -173,6 +173,7 @@ int fpga_region_register(struct device *dev, struct fpga_region *region) mutex_init(®ion->mutex); INIT_LIST_HEAD(®ion->bridge_list); device_initialize(®ion->dev); + region->dev.groups = region->groups; region->dev.class = fpga_region_class; region->dev.parent = dev; region->dev.of_node = dev->of_node; diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h index 6ca41f8f949f..3694821a6d2d 100644 --- a/include/linux/fpga/fpga-bridge.h +++ b/include/linux/fpga/fpga-bridge.h @@ -13,11 +13,13 @@ struct fpga_bridge; * @enable_show: returns the FPGA bridge's status * @enable_set: set a FPGA bridge as enabled or disabled * @fpga_bridge_remove: set FPGA into a specific state during driver remove + * @groups: optional attribute groups. */ struct fpga_bridge_ops { int (*enable_show)(struct fpga_bridge *bridge); int (*enable_set)(struct fpga_bridge *bridge, bool enable); void (*fpga_bridge_remove)(struct fpga_bridge *bridge); + const struct attribute_group **groups; }; /** diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h index 4fb706bd9aba..3c6de23aabdf 100644 --- a/include/linux/fpga/fpga-mgr.h +++ b/include/linux/fpga/fpga-mgr.h @@ -115,6 +115,7 @@ struct fpga_image_info { * @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 + * @groups: optional attribute groups. * * fpga_manager_ops are the low level functions implemented by a specific * fpga manager driver. The optional ones are tested for NULL before being @@ -131,6 +132,7 @@ struct fpga_manager_ops { int (*write_complete)(struct fpga_manager *mgr, struct fpga_image_info *info); void (*fpga_remove)(struct fpga_manager *mgr); + const struct attribute_group **groups; }; /** diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h index 704844944631..b6520318ab9c 100644 --- a/include/linux/fpga/fpga-region.h +++ b/include/linux/fpga/fpga-region.h @@ -14,6 +14,7 @@ * @info: FPGA image info * @priv: private data * @get_bridges: optional function to get bridges to a list + * @groups: optional attribute groups. */ struct fpga_region { struct device dev; @@ -23,6 +24,7 @@ struct fpga_region { struct fpga_image_info *info; void *priv; int (*get_bridges)(struct fpga_region *region); + const struct attribute_group **groups; }; #define to_fpga_region(d) container_of(d, struct fpga_region, dev) -- cgit v1.2.3 From 5f76c6f313bffbf372c0722582e7bd509f0d3510 Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Wed, 15 Nov 2017 16:33:10 -0600 Subject: fpga: fpga-mgr: remove unnecessary code in __fpga_mgr_get Notice that mgr = to_fpga_manager(dev); expands to: mgr = container_of(dev, struct fpga_manager, dev); and container_of is never null, so this null check is unnecessary. Addresses-Coverity-ID: 1397916 Signed-off-by: Gustavo A. R. Silva Signed-off-by: Alan Tull Signed-off-by: Greg Kroah-Hartman --- drivers/fpga/fpga-mgr.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'drivers/fpga/fpga-mgr.c') diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index 223f2401939b..9939d2cbc9a6 100644 --- a/drivers/fpga/fpga-mgr.c +++ b/drivers/fpga/fpga-mgr.c @@ -412,8 +412,6 @@ static struct fpga_manager *__fpga_mgr_get(struct device *dev) struct fpga_manager *mgr; mgr = to_fpga_manager(dev); - if (!mgr) - goto err_dev; if (!try_module_get(dev->parent->driver->owner)) goto err_dev; -- cgit v1.2.3