Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Received-SPF: Pass (protection.outlook.com: domain of cern.ch designates
 188.184.36.46 as permitted sender) receiver=protection.outlook.com;
 client-ip=188.184.36.46; helo=cernmxgwlb4.cern.ch;
From:   Federico Vaga <federico.vaga@cern.ch>
To:     Alan Tull <atull@kernel.org>
Reply-To: <federico.vaga@cern.ch>
CC:     Moritz Fischer <mdf@kernel.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        <linux-kernel@vger.kernel.org>, <linux-fpga@vger.kernel.org>
Subject: Re: [PATCH 1/2] fpga: Document when fpga_blah_free functions should be used
Date:   Thu, 26 Jul 2018 09:26:33 +0200
Message-ID: <3092191.aOBZP6MEga@pcbe13614>
In-Reply-To: <20180725181514.3501-1-atull@kernel.org>
References: <20180725181514.3501-1-atull@kernel.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jul 2018 07:26:47.4395
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 354ef03d-f6ab-4e22-9a2b-08d5f2c92698
X-MS-Exchange-CrossTenant-Id: c80d3499-4a40-4a8c-986e-abce017d6b19
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=c80d3499-4a40-4a8c-986e-abce017d6b19;Ip=[188.184.36.46];Helo=[cernmxgwlb4.cern.ch]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0601MB2470
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Alan,

have you considered the possibility of having something like devm_fpga_[mgr|
bridge|region]_[create|free]() ? Like this, it will be obvious that 'struct 
fpga_mgr' will be released automatically without reading any comment (but the 
comment is still good), and you use devm_*_free() only to handle error 
conditions. 

On Wednesday, July 25, 2018 8:15:13 PM CEST Alan Tull wrote:
> Clarify when fpga_(mgr|bridge|region)_free functions should be used.
> The class's dev_release will handle cleanup when the device is released
> so once the mgr/brige/region has been successfully registered, it
> would be a bug to call fpga_(mgr|bridge|region)_free.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> Suggested-by: Federico Vaga <federico.vaga@cern.ch>
> ---
>  drivers/fpga/fpga-bridge.c | 10 +++++++++-
>  drivers/fpga/fpga-mgr.c    | 10 +++++++++-
>  drivers/fpga/fpga-region.c | 10 +++++++++-
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-bridge.c b/drivers/fpga/fpga-bridge.c
> index 24b8f98..528d2149 100644
> --- a/drivers/fpga/fpga-bridge.c
> +++ b/drivers/fpga/fpga-bridge.c
> @@ -379,7 +379,11 @@ EXPORT_SYMBOL_GPL(fpga_bridge_create);
> 
>  /**
>   * fpga_bridge_free - free a fpga bridge and its id
> - * @bridge:	FPGA bridge struct created by fpga_bridge_create
> + * @bridge:	FPGA bridge struct created by fpga_bridge_create()
> + *
> + * Free a FPGA bridge.  This function should only be called for
> + * freeing a bridge that has not been registered yet (such as in error
> + * paths in a probe function).
>   */
>  void fpga_bridge_free(struct fpga_bridge *bridge)
>  {
> @@ -414,6 +418,10 @@ EXPORT_SYMBOL_GPL(fpga_bridge_register);
>  /**
>   * fpga_bridge_unregister - unregister and free a fpga bridge
>   * @bridge:	FPGA bridge struct created by fpga_bridge_create
> + *
> + * Unregister the bridge device.  The class's dev_release will handle
> + * freeing the bridge struct when the device is released so don't
> + * call fpga_bridge_free() after calling fpga_bridge_unregister().
>   */
>  void fpga_bridge_unregister(struct fpga_bridge *bridge)
>  {
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index a41b07e..9632cbd 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -619,7 +619,11 @@ EXPORT_SYMBOL_GPL(fpga_mgr_create);
> 
>  /**
>   * fpga_mgr_free - deallocate a FPGA manager
> - * @mgr:	fpga manager struct created by fpga_mgr_create
> + * @mgr:	fpga manager struct created by fpga_mgr_create()
> + *
> + * Free a FPGA manager struct.  This function should only be called
> + * for freeing a manager that has not been registered yet (such as in
> + * error paths in a probe function).
>   */
>  void fpga_mgr_free(struct fpga_manager *mgr)
>  {
> @@ -663,6 +667,10 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>  /**
>   * fpga_mgr_unregister - unregister and free a FPGA manager
>   * @mgr:	fpga manager struct
> + *
> + * Unregister the manager device.  The class's dev_release will handle
> + * freeing the manager struct when the device is released so don't
> + * call fpga_mgr_free() after calling fpga_mgr_unregister().
>   */
>  void fpga_mgr_unregister(struct fpga_manager *mgr)
>  {
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 0d65220..7335fa9 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -231,7 +231,11 @@ EXPORT_SYMBOL_GPL(fpga_region_create);
> 
>  /**
>   * fpga_region_free - free a struct fpga_region
> - * @region: FPGA region created by fpga_region_create
> + * @region:  FPGA region created by fpga_region_create()
> + *
> + * Free a FPGA region struct.  This function should only be called for
> + * freeing a region that has not been registered yet (such as in error
> + * paths in a probe function).
>   */
>  void fpga_region_free(struct fpga_region *region)
>  {
> @@ -255,6 +259,10 @@ EXPORT_SYMBOL_GPL(fpga_region_register);
>  /**
>   * fpga_region_unregister - unregister and free a FPGA region
>   * @region: FPGA region
> + *
> + * Unregister the region device.  The class's dev_release will handle
> + * freeing the region so don't call fpga_region_free() after calling
> + * fpga_region_unregister().
>   */
>  void fpga_region_unregister(struct fpga_region *region)
>  {