From patchwork Mon Sep 18 17:59:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Moritz Fischer X-Patchwork-Id: 9957245 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 D239E60385 for ; Mon, 18 Sep 2017 17:58:12 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BF957204BA for ; Mon, 18 Sep 2017 17:58:12 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B442D28A85; Mon, 18 Sep 2017 17:58:12 +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, T_TVD_MIME_EPI autolearn=ham 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 81039204BA for ; Mon, 18 Sep 2017 17:58:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756515AbdIRR6L (ORCPT ); Mon, 18 Sep 2017 13:58:11 -0400 Received: from mail-pf0-f182.google.com ([209.85.192.182]:44130 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756490AbdIRR6K (ORCPT ); Mon, 18 Sep 2017 13:58:10 -0400 Received: by mail-pf0-f182.google.com with SMTP id e1so621907pfk.1 for ; Mon, 18 Sep 2017 10:58:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=UwjRitN1YlvPC3oNC7hIcjhhW8SH45+6yArJgHw+o2M=; b=gdB/oQL4bOkLMeGCCC/lmzIpn6i9kiODiGmttlAz+1mUU0vpInUWYFSIG3OrBYGZrk Bu0WN7489PkkR+z+mp3F0S3PPfhntsgy1KfN9QZhotQ3TJA9hBerk9rssVL4uZoDqY8u S6lSFcSpnQCxD3lIXAw6K28zozApuELSU92aBVlyZY2OaNEbI0Dvm//8HvYz8b/4G4oN R/S/ZPPdbCXwqRP1GVpCCbXeiS7TAuxutlM0Md91G2zfNqo+tmsn0nuUTGdeVnXLu7ub RGaKkzaWMZmjevRDRrLpfuKRpDzXhKyvEErqMydU7IBkad6qnjtZl2bI0uKQ+VVTHj09 QFRg== X-Gm-Message-State: AHPjjUhIfA+Kk21giyI3clkWSSwZA1Hmo/5jbQbsSc01FpFfYin/zvkM YuBq+nFgfejVusIq X-Google-Smtp-Source: AOwi7QC3Godeqynm9oKigYfbl0i8zkKx9P5jA/6t9l8mP4czmA7aGSX6laUJg6WeRCyX1uiRI61nfw== X-Received: by 10.84.248.66 with SMTP id e2mr15414012pln.384.1505757489455; Mon, 18 Sep 2017 10:58:09 -0700 (PDT) Received: from localhost (207-114-172-147.static.twtelecom.net. [207.114.172.147]) by smtp.gmail.com with ESMTPSA id g68sm19155pfc.64.2017.09.18.10.58.07 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 18 Sep 2017 10:58:07 -0700 (PDT) Date: Mon, 18 Sep 2017 10:59:27 -0700 From: Moritz Fischer To: Alan Tull Cc: Moritz Fischer , linux-kernel@vger.kernel.org, linux-fpga@vger.kernel.org Subject: Re: [PATCH v4 01/18] fpga: bridge: support getting bridge from device Message-ID: <20170918175927.GA3429@tyrael.ni.corp.natinst.com> References: <20170913204841.2730-1-atull@kernel.org> <20170913204841.2730-2-atull@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170913204841.2730-2-atull@kernel.org> User-Agent: Mutt/1.8.3 (2017-05-23) 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 On Wed, Sep 13, 2017 at 03:48:24PM -0500, Alan Tull wrote: > Add two functions for getting the FPGA bridge from the device > rather than device tree node. This is to enable writing code > that will support using FPGA bridges without device tree. > Rename one old function to make it clear that it is device > tree-ish. This leaves us with 3 functions for getting a bridge: > > * fpga_bridge_get > Get the bridge given the device. > > * fpga_bridges_get_to_list > Given the device, get the bridge and add it to a list. > > * of_fpga_bridges_get_to_list > Renamed from priviously existing fpga_bridges_get_to_list. > Given the device node, get the bridge and add it to a list. > > Signed-off-by: Alan Tull > --- > v2: use list_for_each_entry > static the bridge_list_lock > update copyright and author email > v3: no change to this patch in this version of patchset > v4: no change to this patch in this version of patchset > --- > drivers/fpga/fpga-bridge.c | 110 +++++++++++++++++++++++++++++++-------- > drivers/fpga/fpga-region.c | 11 ++-- > include/linux/fpga/fpga-bridge.h | 7 ++- > 3 files changed, 100 insertions(+), 28 deletions(-) > > diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c > index fcd2bd3..af6d97e 100644 > --- a/drivers/fpga/fpga-bridge.c > +++ b/drivers/fpga/fpga-bridge.c > @@ -2,6 +2,7 @@ > * FPGA Bridge Framework Driver > * > * Copyright (C) 2013-2016 Altera Corporation, All Rights Reserved. > + * 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, > @@ -70,29 +71,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge) > } > EXPORT_SYMBOL_GPL(fpga_bridge_disable); > > -/** > - * of_fpga_bridge_get - get an exclusive reference to a fpga bridge > - * > - * @np: node pointer of a FPGA bridge > - * @info: fpga image specific information > - * > - * Return fpga_bridge struct if successful. > - * Return -EBUSY if someone already has a reference to the bridge. > - * Return -ENODEV if @np is not a FPGA Bridge. > - */ > -struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, > - struct fpga_image_info *info) > - > +struct fpga_bridge *__fpga_bridge_get(struct device *dev, > + struct fpga_image_info *info) > { > - struct device *dev; > struct fpga_bridge *bridge; > int ret = -ENODEV; > > - dev = class_find_device(fpga_bridge_class, NULL, np, > - fpga_bridge_of_node_match); > - if (!dev) > - goto err_dev; > - > bridge = to_fpga_bridge(dev); > if (!bridge) > goto err_dev; > @@ -117,8 +101,58 @@ struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, > put_device(dev); > return ERR_PTR(ret); > } > + > +/** > + * of_fpga_bridge_get - get an exclusive reference to a fpga bridge > + * > + * @np: node pointer of a FPGA bridge > + * @info: fpga image specific information > + * > + * Return fpga_bridge struct if successful. > + * Return -EBUSY if someone already has a reference to the bridge. > + * Return -ENODEV if @np is not a FPGA Bridge. > + */ > +struct fpga_bridge *of_fpga_bridge_get(struct device_node *np, > + struct fpga_image_info *info) > +{ > + struct device *dev; > + > + dev = class_find_device(fpga_bridge_class, NULL, np, > + fpga_bridge_of_node_match); > + if (!dev) > + return ERR_PTR(-ENODEV); > + > + return __fpga_bridge_get(dev, info); > +} > EXPORT_SYMBOL_GPL(of_fpga_bridge_get); > > +static int fpga_bridge_dev_match(struct device *dev, const void *data) > +{ > + return dev->parent == data; > +} > + > +/** > + * fpga_bridge_get - get an exclusive reference to a fpga bridge > + * @dev: parent device that fpga bridge was registered with > + * > + * Given a device, get an exclusive reference to a fpga bridge. > + * > + * Return: fpga manager struct or IS_ERR() condition containing error code. > + */ > +struct fpga_bridge *fpga_bridge_get(struct device *dev, > + struct fpga_image_info *info) > +{ > + struct device *bridge_dev; > + > + bridge_dev = class_find_device(fpga_bridge_class, NULL, dev, > + fpga_bridge_dev_match); > + if (!bridge_dev) > + return ERR_PTR(-ENODEV); > + > + return __fpga_bridge_get(bridge_dev, info); > +} > +EXPORT_SYMBOL_GPL(fpga_bridge_get); > + > /** > * fpga_bridge_put - release a reference to a bridge > * > @@ -206,7 +240,7 @@ void fpga_bridges_put(struct list_head *bridge_list) > EXPORT_SYMBOL_GPL(fpga_bridges_put); > > /** > - * fpga_bridges_get_to_list - get a bridge, add it to a list > + * of_fpga_bridge_get_to_list - get a bridge, add it to a list > * > * @np: node pointer of a FPGA bridge > * @info: fpga image specific information > @@ -216,14 +250,44 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put); > * > * Return 0 for success, error code from of_fpga_bridge_get() othewise. > */ > -int fpga_bridge_get_to_list(struct device_node *np, > +int of_fpga_bridge_get_to_list(struct device_node *np, > + struct fpga_image_info *info, > + struct list_head *bridge_list) > +{ > + struct fpga_bridge *bridge; > + unsigned long flags; > + > + bridge = of_fpga_bridge_get(np, info); > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + > + spin_lock_irqsave(&bridge_list_lock, flags); > + list_add(&bridge->node, bridge_list); > + spin_unlock_irqrestore(&bridge_list_lock, flags); I know this is not new code, but I was wondering the other day: Why are we using a single spinlock to protect all lists that are being passed in as parameters here? I have a patch that moves the spinlock into the region containing the list that uses the bridges which (unless I misunderstand something here), makes more sense: Am I missing something here? If not I"ll send out my patch separately. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_fpga_bridge_get_to_list); > + > +/** > + * fpga_bridge_get_to_list - given device, get a bridge, add it to a list > + * > + * @dev: FPGA bridge device > + * @info: fpga image specific information > + * @bridge_list: list of FPGA bridges > + * > + * Get an exclusive reference to the bridge and and it to the list. > + * > + * Return 0 for success, error code from fpga_bridge_get() othewise. > + */ > +int fpga_bridge_get_to_list(struct device *dev, > struct fpga_image_info *info, > struct list_head *bridge_list) > { > struct fpga_bridge *bridge; > unsigned long flags; > > - bridge = of_fpga_bridge_get(np, info); > + bridge = fpga_bridge_get(dev, info); > if (IS_ERR(bridge)) > return PTR_ERR(bridge); > > @@ -381,7 +445,7 @@ static void __exit fpga_bridge_dev_exit(void) > } > > MODULE_DESCRIPTION("FPGA Bridge Driver"); > -MODULE_AUTHOR("Alan Tull "); > +MODULE_AUTHOR("Alan Tull "); > MODULE_LICENSE("GPL v2"); > > subsys_initcall(fpga_bridge_dev_init); > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index d9ab7c7..91755562 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -183,11 +183,14 @@ 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, > - ®ion->bridge_list); > + ret = of_fpga_bridge_get_to_list(region_np->parent, region->info, > + ®ion->bridge_list); > + > + /* -EBUSY means parent is a bridge that is under use. Give up. */ > if (ret == -EBUSY) > return ret; > > + /* Zero return code means parent was a bridge and was added to list. */ > if (!ret) > parent_br = region_np->parent; > > @@ -207,8 +210,8 @@ 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, > - ®ion->bridge_list); > + ret = of_fpga_bridge_get_to_list(br, region->info, > + ®ion->bridge_list); > > /* If any of the bridges are in use, give up */ > if (ret == -EBUSY) { > diff --git a/include/linux/fpga/fpga-bridge.h b/include/linux/fpga/fpga-bridge.h > index dba6e3c..9f6696b 100644 > --- a/include/linux/fpga/fpga-bridge.h > +++ b/include/linux/fpga/fpga-bridge.h > @@ -42,6 +42,8 @@ struct fpga_bridge { > > struct fpga_bridge *of_fpga_bridge_get(struct device_node *node, > struct fpga_image_info *info); > +struct fpga_bridge *fpga_bridge_get(struct device *dev, > + struct fpga_image_info *info); > void fpga_bridge_put(struct fpga_bridge *bridge); > int fpga_bridge_enable(struct fpga_bridge *bridge); > int fpga_bridge_disable(struct fpga_bridge *bridge); > @@ -49,9 +51,12 @@ int fpga_bridge_disable(struct fpga_bridge *bridge); > int fpga_bridges_enable(struct list_head *bridge_list); > int fpga_bridges_disable(struct list_head *bridge_list); > void fpga_bridges_put(struct list_head *bridge_list); > -int fpga_bridge_get_to_list(struct device_node *np, > +int fpga_bridge_get_to_list(struct device *dev, > struct fpga_image_info *info, > struct list_head *bridge_list); > +int of_fpga_bridge_get_to_list(struct device_node *np, > + struct fpga_image_info *info, > + struct list_head *bridge_list); > > int fpga_bridge_register(struct device *dev, const char *name, > const struct fpga_bridge_ops *br_ops, void *priv); > -- > 2.7.4 > Thanks, Moritz diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c index 9651aa56244a..b03ec59448e2 100644 --- a/drivers/fpga/fpga-bridge.c +++ b/drivers/fpga/fpga-bridge.c @@ -214,6 +214,8 @@ EXPORT_SYMBOL_GPL(fpga_bridges_put); * * Get an exclusive reference to the bridge and and it to the list. * + * Must be called with list lock held. + * * Return 0 for success, error code from of_fpga_bridge_get() othewise. */ int fpga_bridge_get_to_list(struct device_node *np, @@ -221,15 +223,12 @@ int fpga_bridge_get_to_list(struct device_node *np, struct list_head *bridge_list) { struct fpga_bridge *bridge; - unsigned long flags; bridge = of_fpga_bridge_get(np, info); if (IS_ERR(bridge)) return PTR_ERR(bridge); - spin_lock_irqsave(&bridge_list_lock, flags); list_add(&bridge->node, bridge_list); - spin_unlock_irqrestore(&bridge_list_lock, flags); return 0; } diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 3b6b2f4182a1..c5c958e0e601 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -37,6 +37,7 @@ struct fpga_region { struct device dev; struct mutex mutex; /* for exclusive reference to region */ struct list_head bridge_list; + spinlock_t bridge_list_lock; /* protects access to bridge list */ struct fpga_image_info *info; }; @@ -180,11 +181,15 @@ static int fpga_region_get_bridges(struct fpga_region *region, struct device *dev = ®ion->dev; struct device_node *region_np = dev->of_node; struct device_node *br, *np, *parent_br = NULL; + unsigned long flags; int i, ret; /* If parent is a bridge, add to list */ + spin_lock_irqsave(®ion->bridge_list_lock, flags); ret = fpga_bridge_get_to_list(region_np->parent, region->info, ®ion->bridge_list); + spin_unlock_irqrestore(®ion->bridge_list_lock, flags); + if (ret == -EBUSY) return ret; @@ -508,6 +513,7 @@ static int fpga_region_probe(struct platform_device *pdev) mutex_init(®ion->mutex); INIT_LIST_HEAD(®ion->bridge_list); + spin_lock_init(®ion->bridge_list_lock); device_initialize(®ion->dev); region->dev.class = fpga_region_class;