From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932713AbdBPRfP (ORCPT ); Thu, 16 Feb 2017 12:35:15 -0500 Received: from mail-qt0-f196.google.com ([209.85.216.196]:34336 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932193AbdBPRfN (ORCPT ); Thu, 16 Feb 2017 12:35:13 -0500 MIME-Version: 1.0 In-Reply-To: References: <1487175261-7051-1-git-send-email-atull@kernel.org> <1487175261-7051-3-git-send-email-atull@kernel.org> From: Alan Tull Date: Thu, 16 Feb 2017 11:35:11 -0600 Message-ID: Subject: Re: [RFC 2/8] fpga-region: support more than one overlay per FPGA region To: matthew.gerlach@linux.intel.com Cc: Moritz Fischer , Jason Gunthorpe , linux-kernel , linux-fpga@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 16, 2017 at 10:50 AM, wrote: > > > Hi Alan, > > On Wed, 15 Feb 2017, Alan Tull wrote: > >> 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; > > > Each region has its own list of overlays. Shouldn't each region have its > own lock? Otherwise only one region can get overlays applied at one time > even though there may be more than one region and one fpga-mgr per region. Hi Matthew, I think the entire kernel is only taking one overlay at a time. But it's probably better for me to make that change anyway. Alan > > >> + >> #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; >> -- >> 2.7.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >