From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8180FC48BDF for ; Tue, 15 Jun 2021 07:25:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 575626141E for ; Tue, 15 Jun 2021 07:25:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230144AbhFOH1o (ORCPT ); Tue, 15 Jun 2021 03:27:44 -0400 Received: from mail.kernel.org ([198.145.29.99]:53170 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229613AbhFOH1o (ORCPT ); Tue, 15 Jun 2021 03:27:44 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 0B99161407; Tue, 15 Jun 2021 07:25:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1623741940; bh=o1mteB7FeatiVAKG9ch+kb90GPaoGAoukIIR4+TsoIQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=AKcu6mIMMkdoGWwuovkL8Vdw+CCc0Z1exmEyQ5+HfAFHliRb1TVsSTraxx4T/NEcI 6dkwJt6DfQpWo7na6JH6Q7G3x+Mr85wqpvXY26T5JbRQXDzQNoip5vOCh/nDgUdbM7 qJlTeq/Db6PMTFYZsgNii+vKBN8wXa2aIKZP296U= Date: Tue, 15 Jun 2021 09:25:37 +0200 From: Greg KH To: Moritz Fischer Cc: linux-fpga@vger.kernel.org, Russ Weight , Xu Yilun Subject: Re: [PATCH 6/8] fpga: mgr: Use standard dev_release for class driver Message-ID: References: <20210614170909.232415-1-mdf@kernel.org> <20210614170909.232415-7-mdf@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210614170909.232415-7-mdf@kernel.org> Precedence: bulk List-ID: X-Mailing-List: linux-fpga@vger.kernel.org On Mon, Jun 14, 2021 at 10:09:07AM -0700, Moritz Fischer wrote: > From: Russ Weight > > The FPGA manager class driver data structure is being treated as a > managed resource instead of using the class.dev_release call-back > function to release the class data structure. This change populates > the class.dev_release function, changes the fpga_mgr_free() function > to call put_device() and changes the fpga_mgr_unregister() function > to call device_del() instead of device_unregister(). > > Signed-off-by: Russ Weight > Reviewed-by: Xu Yilun > Signed-off-by: Moritz Fischer > --- > drivers/fpga/fpga-mgr.c | 35 +++++++++++++++-------------------- > 1 file changed, 15 insertions(+), 20 deletions(-) > > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c > index 42ddc0844781..9f6c3760b6ff 100644 > --- a/drivers/fpga/fpga-mgr.c > +++ b/drivers/fpga/fpga-mgr.c > @@ -585,8 +585,10 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name, > return NULL; > > id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL); > - if (id < 0) > - goto error_kfree; > + if (id < 0) { > + kfree(mgr); > + return NULL; > + } > > mutex_init(&mgr->ref_mutex); > > @@ -602,17 +604,12 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name, > mgr->dev.id = id; > > ret = dev_set_name(&mgr->dev, "fpga%d", id); > - if (ret) > - goto error_device; > + if (ret) { > + put_device(&mgr->dev); > + return NULL; > + } > > return mgr; > - > -error_device: > - ida_simple_remove(&fpga_mgr_ida, id); > -error_kfree: > - kfree(mgr); > - > - return NULL; > } > EXPORT_SYMBOL_GPL(fpga_mgr_create); > > @@ -622,8 +619,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_create); > */ > void fpga_mgr_free(struct fpga_manager *mgr) > { > - ida_simple_remove(&fpga_mgr_ida, mgr->dev.id); > - kfree(mgr); > + put_device(&mgr->dev); > } > EXPORT_SYMBOL_GPL(fpga_mgr_free); > > @@ -692,16 +688,11 @@ int fpga_mgr_register(struct fpga_manager *mgr) > > ret = device_add(&mgr->dev); > if (ret) > - goto error_device; > + return ret; If this fails, are you sure you want to just return the error number? You can not call device_del() afterward if this fails, you have to call put_device(). See the documentation for device_add() for details. This is messy as you are doing a "two step" initialization of your fpga_manager device for some odd reason. Why do you need to do that? When you call this you seem to be forced to do: fpga_mgr_create() fpga_mgr_register() in each individual driver. Why force drivers to do this and not just do a simple: fpga_mgr_register() that internally does the create/add process? Why the two steps? That's normally reserved for when you need to do something complex in the "core" for the subsystem, and shouldn't be pushed out to each individual driver like it currently is for the fpga core as you will run into the problem you have here. Namely when you want to clean up from a failure, you don't know if you really did register that device properly or not. And no, don't add another flag, just make this simple and hard to get wrong. As it is it feels like each fpga driver has to do extra work to be sure to get this all correct each time. So I can not take this as-is, sorry. And sorry I never noticed these problems when the code when in originally. thanks, greg k-h