[PATCH v3 6/6] staging: kpc2000: use IDA to assign card numbers.

Greg KH gregkh at linuxfoundation.org
Fri May 17 11:50:13 UTC 2019


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 <jeremy at azazel.net>
> ---
>  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 <linux/idr.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> @@ -18,6 +19,7 @@
>  #include <linux/jiffies.h>
>  #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


More information about the devel mailing list