From patchwork Wed Feb 15 16:14:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Tull X-Patchwork-Id: 9574397 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 0696C60493 for ; Wed, 15 Feb 2017 16:15:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E93B22808F for ; Wed, 15 Feb 2017 16:15:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DDECA2846B; Wed, 15 Feb 2017 16:15:39 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 296B52847F for ; Wed, 15 Feb 2017 16:15:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752105AbdBOQPW (ORCPT ); Wed, 15 Feb 2017 11:15:22 -0500 Received: from mail.kernel.org ([198.145.29.136]:51648 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751599AbdBOQPF (ORCPT ); Wed, 15 Feb 2017 11:15:05 -0500 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BE13C20263; Wed, 15 Feb 2017 16:14:34 +0000 (UTC) Received: from localhost.localdomain (user-0ccsrjt.cable.mindspring.com [24.206.110.125]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 77050202EB; Wed, 15 Feb 2017 16:14:32 +0000 (UTC) From: Alan Tull To: Moritz Fischer , Jason Gunthorpe Cc: Alan Tull , linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org Subject: [RFC 2/8] fpga-region: support more than one overlay per FPGA region Date: Wed, 15 Feb 2017 10:14:15 -0600 Message-Id: <1487175261-7051-3-git-send-email-atull@kernel.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1487175261-7051-1-git-send-email-atull@kernel.org> References: <1487175261-7051-1-git-send-email-atull@kernel.org> X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-fpga-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fpga@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Currently if a user applies > 1 overlays to a region and removes them, we get a slow warning from devm_kfree. because the pointer to the FPGA image info was overwritten. This commit adds a list to keep track of overlays applied to each FPGA region. Some rules are enforced: * Only allow the first overlay to a region to do FPGA programming. * Allow subsequent overlays to modify properties but not properties regarding FPGA programming. * To reprogram a region, first remove all previous overlays to the region. Signed-off-by: Alan Tull --- drivers/fpga/fpga-region.c | 222 +++++++++++++++++++++++++++++++-------------- 1 file changed, 155 insertions(+), 67 deletions(-) diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 24f4ed5..60d2947 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -31,15 +31,32 @@ * @dev: FPGA Region device * @mutex: enforces exclusive reference to region * @bridge_list: list of FPGA bridges specified in region - * @info: fpga image specific information + * @overlays: list of struct region_overlay_info */ struct fpga_region { struct device dev; struct mutex mutex; /* for exclusive reference to region */ struct list_head bridge_list; - struct fpga_image_info *info; + struct list_head overlays; }; +/** + * struct region_overlay: info regarding overlays applied to the region + * @node: list node + * @overlay: pointer to overlay + * @image_info: fpga image specific information parsed from overlay. Is NULL if + * overlay doesn't program FPGA. + */ + +struct region_overlay { + struct list_head node; + struct device_node *overlay; + struct fpga_image_info *image_info; +}; + +/* Lock for list of overlays */ +spinlock_t overlay_list_lock; + #define to_fpga_region(d) container_of(d, struct fpga_region, dev) static DEFINE_IDA(fpga_region_ida); @@ -161,7 +178,7 @@ static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region) /** * fpga_region_get_bridges - create a list of bridges * @region: FPGA region - * @overlay: device node of the overlay + * @reg_ovl: overlay applied to the region * * Create a list of bridges including the parent bridge and the bridges * specified by "fpga-bridges" property. Note that the @@ -175,7 +192,7 @@ static struct fpga_manager *fpga_region_get_manager(struct fpga_region *region) * or -EBUSY if any of the bridges are in use. */ static int fpga_region_get_bridges(struct fpga_region *region, - struct device_node *overlay) + struct region_overlay *reg_ovl) { struct device *dev = ®ion->dev; struct device_node *region_np = dev->of_node; @@ -183,7 +200,8 @@ static int fpga_region_get_bridges(struct fpga_region *region, int i, ret; /* If parent is a bridge, add to list */ - ret = fpga_bridge_get_to_list(region_np->parent, region->info, + ret = fpga_bridge_get_to_list(region_np->parent, + reg_ovl->image_info, ®ion->bridge_list); if (ret == -EBUSY) return ret; @@ -192,8 +210,8 @@ static int fpga_region_get_bridges(struct fpga_region *region, parent_br = region_np->parent; /* If overlay has a list of bridges, use it. */ - if (of_parse_phandle(overlay, "fpga-bridges", 0)) - np = overlay; + if (of_parse_phandle(reg_ovl->overlay, "fpga-bridges", 0)) + np = reg_ovl->overlay; else np = region_np; @@ -207,7 +225,7 @@ static int fpga_region_get_bridges(struct fpga_region *region, continue; /* If node is a bridge, get it and add to list */ - ret = fpga_bridge_get_to_list(br, region->info, + ret = fpga_bridge_get_to_list(br, reg_ovl->image_info, ®ion->bridge_list); /* If any of the bridges are in use, give up */ @@ -222,13 +240,15 @@ static int fpga_region_get_bridges(struct fpga_region *region, /** * fpga_region_program_fpga - program FPGA - * @region: FPGA region - * @overlay: device node of the overlay - * Program an FPGA using information in the region's fpga image info. - * Return 0 for success or negative error code. + * @region: FPGA region that is receiving an overlay + * @reg_ovl: region overlay with fpga_image_info parsed from overlay + * + * Program an FPGA using information in a device tree overlay. + * + * Return: 0 for success or negative error code. */ static int fpga_region_program_fpga(struct fpga_region *region, - struct device_node *overlay) + struct region_overlay *reg_ovl) { struct fpga_manager *mgr; int ret; @@ -245,7 +265,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, return PTR_ERR(mgr); } - ret = fpga_region_get_bridges(region, overlay); + ret = fpga_region_get_bridges(region, reg_ovl); if (ret) { pr_err("failed to get fpga region bridges\n"); goto err_put_mgr; @@ -257,7 +277,7 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } - ret = fpga_mgr_load(mgr, region->info); + ret = fpga_mgr_load(mgr, reg_ovl->image_info); if (ret) { pr_err("failed to load fpga image\n"); goto err_put_br; @@ -321,80 +341,135 @@ static int child_regions_with_firmware(struct device_node *overlay) } /** - * fpga_region_notify_pre_apply - pre-apply overlay notification - * - * @region: FPGA region that the overlay was applied to - * @nd: overlay notification data - * - * Called after when an overlay targeted to a FPGA Region is about to be - * applied. Function will check the properties that will be added to the FPGA - * region. If the checks pass, it will program the FPGA. - * - * The checks are: - * The overlay must add either firmware-name or external-fpga-config property - * to the FPGA Region. + * fpga_region_parse_ov - parse and check overlay applied to region * - * firmware-name : program the FPGA - * external-fpga-config : FPGA is already programmed - * - * The overlay can add other FPGA regions, but child FPGA regions cannot have a - * firmware-name property since those regions don't exist yet. + * @region: FPGA region + * @overlay: overlay applied to the FPGA region * - * If the overlay that breaks the rules, notifier returns an error and the - * overlay is rejected before it goes into the main tree. + * Given an overlay applied to a FPGA region, parse the FPGA image specific + * info in the overlay and do some checking. * - * Returns 0 for success or negative error code for failure. + * Returns: + * NULL if overlay doesn't direct us to program the FPGA. + * fpga_image_info struct if there is an image to program. + * error code for invalid overlay. */ -static int fpga_region_notify_pre_apply(struct fpga_region *region, - struct of_overlay_notify_data *nd) +static struct fpga_image_info *fpga_region_parse_ov(struct fpga_region *region, + struct device_node *overlay) { + struct device *dev = ®ion->dev; struct fpga_image_info *info; int ret; - info = devm_kzalloc(®ion->dev, sizeof(*info), GFP_KERNEL); - if (!info) - return -ENOMEM; - - region->info = info; - - /* Reject overlay if child FPGA Regions have firmware-name property */ - ret = child_regions_with_firmware(nd->overlay); + /* + * Reject overlay if child FPGA Regions added in the overlay have + * firmware-name property (would mean that an FPGA region that has + * not been added to the live tree yet is doing FPGA programming). + */ + ret = child_regions_with_firmware(overlay); if (ret) - return ret; + return ERR_PTR(ret); + + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); + if (!info) + return ERR_PTR(-ENOMEM); - /* Read FPGA region properties from the overlay */ - if (of_property_read_bool(nd->overlay, "partial-fpga-config")) + if (of_property_read_bool(overlay, "partial-fpga-config")) info->flags |= FPGA_MGR_PARTIAL_RECONFIG; - if (of_property_read_bool(nd->overlay, "external-fpga-config")) + if (of_property_read_bool(overlay, "external-fpga-config")) info->flags |= FPGA_MGR_EXTERNAL_CONFIG; - of_property_read_string(nd->overlay, "firmware-name", - &info->firmware_name); + of_property_read_string(overlay, "firmware-name", &info->firmware_name); - of_property_read_u32(nd->overlay, "region-unfreeze-timeout-us", + of_property_read_u32(overlay, "region-unfreeze-timeout-us", &info->enable_timeout_us); - of_property_read_u32(nd->overlay, "region-freeze-timeout-us", + of_property_read_u32(overlay, "region-freeze-timeout-us", &info->disable_timeout_us); - /* If FPGA was externally programmed, don't specify firmware */ - if ((info->flags & FPGA_MGR_EXTERNAL_CONFIG) && info->firmware_name) { - pr_err("error: specified firmware and external-fpga-config"); - return -EINVAL; + /* If overlay is not programming the FPGA, don't need FPGA image info */ + if (!info->firmware_name) { + devm_kfree(dev, info); + return NULL; } - /* FPGA is already configured externally. We're done. */ - if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) - return 0; + /* + * If overlay informs us FPGA was externally programmed, specifying + * firmware here would be ambiguous. + */ + if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) { + dev_err(dev, "error: specified firmware and external-fpga-config"); + devm_kfree(dev, info); + return ERR_PTR(-EINVAL); + } - /* If we got this far, we should be programming the FPGA */ - if (!info->firmware_name) { - pr_err("should specify firmware-name or external-fpga-config\n"); - return -EINVAL; + /* + * The first overlay to a region may reprogram the FPGA and specify how + * to program the fpga (fpga_image_info). Subsequent overlays can be + * can add/modify child node properties if that is useful. + */ + if (!list_empty(®ion->overlays)) { + dev_err(dev, "Only 1st DTO to a region may program a FPGA.\n"); + devm_kfree(dev, info); + return ERR_PTR(-EINVAL); } - return fpga_region_program_fpga(region, nd->overlay); + return info; +} + +/** + * fpga_region_notify_pre_apply - pre-apply overlay notification + * + * @region: FPGA region that the overlay will be applied to + * @nd: overlay notification data + * + * Called when an overlay targeted to a FPGA Region is about to be applied. + * Parses the overlay for properties that influence how the FPGA will be + * programmed and does some checking. If the checks pass, programs the FPGA. + * + * If the overlay that breaks the rules, notifier returns an error and the + * overlay is rejected, preventing it from being added to the main tree. + * + * Return: 0 for success or negative error code for failure. + */ +static int fpga_region_notify_pre_apply(struct fpga_region *region, + struct of_overlay_notify_data *nd) +{ + struct device *dev = ®ion->dev; + struct region_overlay *reg_ovl; + struct fpga_image_info *info; + unsigned long flags; + int ret; + + info = fpga_region_parse_ov(region, nd->overlay); + if (IS_ERR(info)) + return PTR_ERR(info); + + reg_ovl = devm_kzalloc(dev, sizeof(*reg_ovl), GFP_KERNEL); + if (!reg_ovl) + return -ENOMEM; + + reg_ovl->overlay = nd->overlay; + reg_ovl->image_info = info; + + if (info) { + ret = fpga_region_program_fpga(region, reg_ovl); + if (ret) + goto pre_a_err; + } + + spin_lock_irqsave(&overlay_list_lock, flags); + list_add_tail(®_ovl->node, ®ion->overlays); + spin_unlock_irqrestore(&overlay_list_lock, flags); + + return 0; + +pre_a_err: + if (info) + devm_kfree(dev, info); + devm_kfree(dev, reg_ovl); + return ret; } /** @@ -409,10 +484,22 @@ static int fpga_region_notify_pre_apply(struct fpga_region *region, static void fpga_region_notify_post_remove(struct fpga_region *region, struct of_overlay_notify_data *nd) { + struct region_overlay *reg_ovl; + unsigned long flags; + + reg_ovl = list_last_entry(®ion->overlays, typeof(*reg_ovl), node); + fpga_bridges_disable(®ion->bridge_list); fpga_bridges_put(®ion->bridge_list); - devm_kfree(®ion->dev, region->info); - region->info = NULL; + + spin_lock_irqsave(&overlay_list_lock, flags); + list_del(®_ovl->node); + spin_unlock_irqrestore(&overlay_list_lock, flags); + + if (reg_ovl->image_info) + devm_kfree(®ion->dev, reg_ovl->image_info); + + devm_kfree(®ion->dev, reg_ovl); } /** @@ -496,6 +583,7 @@ static int fpga_region_probe(struct platform_device *pdev) mutex_init(®ion->mutex); INIT_LIST_HEAD(®ion->bridge_list); + INIT_LIST_HEAD(®ion->overlays); device_initialize(®ion->dev); region->dev.class = fpga_region_class;