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@xxxxxxxxxx>
> ---
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
