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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <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.

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.

Attachment: signature.asc
Description: PGP signature

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux