On 2019-05-17, at 13:50:13 +0200, Greg KH wrote: > 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.
Most of the IDR's I looked at were protected by locks, but none of the
IDA's ...
> 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.
... guessing this is why.
> So the changelog really isn't correct here :(
Will fix.
> > Updated TODO.
> >
> > Signed-off-by: Jeremy Sowden <jer...@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.
Will fix.
> > 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.
Nuts. Missed it. Will fix.
Thanks for the review.
J.
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
