From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by ash.osuosl.org (Postfix) with ESMTP id 6948A1BF30A for ; Fri, 17 May 2019 11:50:17 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 663CC86C66 for ; Fri, 17 May 2019 11:50:17 +0000 (UTC) Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zMj5Nq4hqecz for ; Fri, 17 May 2019 11:50:16 +0000 (UTC) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by hemlock.osuosl.org (Postfix) with ESMTPS id BA62A865A1 for ; Fri, 17 May 2019 11:50:16 +0000 (UTC) Date: Fri, 17 May 2019 13:50:13 +0200 From: Greg KH Subject: Re: [PATCH v3 6/6] staging: kpc2000: use IDA to assign card numbers. Message-ID: <20190517115013.GA662@kroah.com> References: <20190517073057.GA2631@kroah.com> <20190517110315.10646-1-jeremy@azazel.net> <20190517110315.10646-7-jeremy@azazel.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190517110315.10646-7-jeremy@azazel.net> List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Jeremy Sowden Cc: Linux Driver Project Developer List On Fri, May 17, 2019 at 12:03:15PM +0100, Jeremy Sowden wrote: > Previously the next card number was assigned from a static int local > variable, which was read and later incremented. This was not thread- > safe, so now we use an IDA instead. An ida is not thread safe either. But, you are onlyu touching this from the pci probe/release functions, which are guaranteed to never race for a specific driver, so you could use a static int if you were just worried about the race. So the changelog really isn't correct here :( > > Updated TODO. > > Signed-off-by: Jeremy Sowden > --- > drivers/staging/kpc2000/TODO | 1 - > drivers/staging/kpc2000/kpc2000/core.c | 15 ++++++++++++--- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/kpc2000/TODO b/drivers/staging/kpc2000/TODO > index 669fe5bf9637..47530e23e940 100644 > --- a/drivers/staging/kpc2000/TODO > +++ b/drivers/staging/kpc2000/TODO > @@ -1,6 +1,5 @@ > - the kpc_spi driver doesn't seem to let multiple transactions (to different instances of the core) happen in parallel... > - The kpc_i2c driver is a hot mess, it should probably be cleaned up a ton. It functions against current hardware though. > -- pcard->card_num in kp2000_pcie_probe() is a global variable and needs atomic / locking / something better. > - would be nice if the AIO fileops in kpc_dma could be made to work > - probably want to add a CONFIG_ option to control compilation of the AIO functions > - if the AIO fileops in kpc_dma start working, next would be making iov_count > 1 work too > diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c > index 80141514f7d1..3a90cdad3eb4 100644 > --- a/drivers/staging/kpc2000/kpc2000/core.c > +++ b/drivers/staging/kpc2000/kpc2000/core.c > @@ -1,4 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0+ > +#include > #include > #include > #include > @@ -18,6 +19,7 @@ > #include > #include "pcie.h" > > +static DEFINE_IDA(card_num_ida); > > /******************************************************* > * SysFS Attributes > @@ -230,7 +232,6 @@ int kp2000_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > int err = 0; > struct kp2000_device *pcard; > - static int card_count = 1; > int rv; > unsigned long reg_bar_phys_addr; > unsigned long reg_bar_phys_len; > @@ -250,8 +251,13 @@ int kp2000_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) > //} > > //{ Step 2: Initialize trivial pcard elements > - pcard->card_num = card_count; > - card_count++; > + rv = ida_simple_get(&card_num_ida, 1, INT_MAX, GFP_KERNEL); > + if (rv < 0) { > + err = rv; > + dev_err(&pdev->dev, "probe: failed to get card number (%d)\n", err); > + goto out2; > + } > + pcard->card_num = rv; When writing new code, you could use the correct coding style please. Why is 'rv' even needed here? Just use err. > scnprintf(pcard->name, 16, "kpcard%d", pcard->card_num); > > mutex_init(&pcard->sem); > @@ -428,6 +434,8 @@ int kp2000_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) > pci_disable_device(pcard->pdev); > out3: > unlock_card(pcard); > + ida_simple_remove(&card_num_ida, pcard->card_num); > + out2: > kfree(pcard); > return err; > } > @@ -461,5 +469,6 @@ void kp2000_pcie_remove(struct pci_dev *pdev) > pci_disable_device(pcard->pdev); > pci_set_drvdata(pdev, NULL); > unlock_card(pcard); > + ida_simple_remove(&card_num_ida, pcard->card_num); > kfree(pcard); > } You forgot to call ida_destroy() when the module is unloaded :( Yeah, it's not obvious, and is supposed to be fixed up soon, but for now you still need to do that. thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel