[PATCH 0/6] Create ida helpers and use them

[PATCH 0/6] Create ida helpers and use them

[Lee Duncan]

As pointed out by Christoph Hellwig, many of the users
of the ida index routines call them with the same calling
pattern, so why not make some helper functions to help
clean up and simplify the code a bit.

After looking at all the uses of the ida routines, I
noticed that most of clients of this API used the
same sequence -- but not all of them. So I picked
the most common and seemingly most correct sequence,
and I made inline helper functions for that, creating:

 - ida_get_index() -- get an 'ida' index, with proper
   locking and retry
   
 - ida_put_index() -- release/return an index allocated
   by ida_get_index(), with proper locking

I converted the following drivers to use the new helper
routines, because they matched the calling sequence
used by the helper functions:

  scsi/sd.c                     SCSI disk
  base/soc.c                    sound?
  block/mtip32xxx/mtip32xx.c    PCI block?
  block/nvme-core.c             Intel SSD?
  block/rsxx/core.c             IBM Flash?

Obviously, only one of these is a SCSI driver, so
I hope this is still the right place to post this.

NOTE 1: the soc.c driver actually had normal locking
around it's 'get' call, but it had no locking around
it's 'put' (release) calls, so since the helper routines
have locking, that means the soc.c driver now has
locking when it releases an index.

There were other drivers that used ida in a way
that meant I could not easily modify them:

  drivers/iommu/iommu.c:
    - uses unlikely(...)
    - uses a mutex instead of spinlock

  drivers/misc/cb710/core.c:
    - calls return from within the 'get' while loop
    - uses spin_lock_irq*()

  drivers/scsi/osd/osd_uld.c:
    - uses no locking at all for indexes [See NOTE 2]

  drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
    - uses 'unlikely()' in 'get' while loop
    - uses 'goto's out of 'get' while loop

NOTE 2: Not sure if this is a bug in the osd code, since
I did not look at it very closely.

The only client driver changes I was able to functionally
test where the sd.c changes. I also tested the helper
functions using the hosts.c module, where they worked
fine, but those changes were later reverted in favor of
using idr there instead. (See previous patch series.)

Lee Duncan (6):
  Add ida helper routines
  SCSI: Update sd driver to use new ida helper functions.
  Update soc driver to use new ida helper functions.
  Update soc driver to use new ida helper functions.
  Update nvme-core driver to use new ida helper functions.
  Update rsxx core driver to use new ida helper functions.

 drivers/base/soc.c                | 17 +++-------------
 drivers/block/mtip32xx/mtip32xx.c | 22 ++++----------------
 drivers/block/nvme-core.c         | 14 ++-----------
 drivers/block/rsxx/core.c         | 16 ++-------------
 drivers/scsi/sd.c                 | 18 +++--------------
 include/linux/idr.h               | 42 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 56 insertions(+), 73 deletions(-)

-- 
2.1.4

[PATCH 1/6] Add ida helper routines

[Lee Duncan]

Clients of the ida API routinely follow the
same steps to allocate and ida index, as well
as to free said index. These helper routines
should make it a little easier to use these
APIs.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 include/linux/idr.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/include/linux/idr.h b/include/linux/idr.h
index 013fd9bc4cb6..5a9526dc6298 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -16,6 +16,8 @@
 #include <linux/bitops.h>
 #include <linux/init.h>
 #include <linux/rcupdate.h>
+#include <linux/spinlock.h>
+#include <linux/gfp.h>
 
 /*
  * We want shallower trees and thus more bits covered at each layer.  8
@@ -183,4 +185,44 @@ static inline int ida_get_new(struct ida *ida, int *p_id)
 
 void __init idr_init_cache(void);
 
+/**
+ * ida_get_index - allocate a ida index value
+ * @ida		idr handle
+ * @lock	spinlock handle protecting this index
+ * @p_id	pointer to allocated index value
+ *
+ * A helper function for safely allocating an index value (id),
+ * returning a negative errno value on failure, else 0.
+ */
+static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id)
+{
+	int error = -ENOMEM;
+
+	do {
+		if (!ida_pre_get(ida, GFP_KERNEL))
+			break;
+		spin_lock(lock);
+		error = ida_get_new(ida, p_id);
+		spin_unlock(lock);
+	} while (error == -EAGAIN);
+
+	return error;
+}
+
+/**
+ * ida_put_index - free an allocated ida index value
+ * @ida		idr handle
+ * @lock	spinlock handle protecting this index
+ * @id		the value of the allocated index
+ *
+ * A helper function that goes with @ida_get_index, which safely
+ * frees a previously-allocated index value.
+ */
+static inline void ida_put_index(struct ida *ida, spinlock_t *lock, int id)
+{
+	spin_lock(lock);
+	ida_remove(ida, id);
+	spin_unlock(lock);
+}
+
 #endif /* __IDR_H__ */
-- 
2.1.4

[PATCH 2/6] SCSI: Update sd driver to use new ida helper functions

[Lee Duncan]

This simplifies the calling sequence to
acquire and to release an ida-managed index.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/sd.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3b2fcb4fada0..0f855e48bc1d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2948,15 +2948,7 @@ static int sd_probe(struct device *dev)
 	if (!gd)
 		goto out_free;
 
-	do {
-		if (!ida_pre_get(&sd_index_ida, GFP_KERNEL))
-			goto out_put;
-
-		spin_lock(&sd_index_lock);
-		error = ida_get_new(&sd_index_ida, &index);
-		spin_unlock(&sd_index_lock);
-	} while (error == -EAGAIN);
-
+	error = ida_get_index(&sd_index_ida, &sd_index_lock, &index);
 	if (error) {
 		sdev_printk(KERN_WARNING, sdp, "sd_probe: memory exhausted.\n");
 		goto out_put;
@@ -3001,9 +2993,7 @@ static int sd_probe(struct device *dev)
 	return 0;
 
  out_free_index:
-	spin_lock(&sd_index_lock);
-	ida_remove(&sd_index_ida, index);
-	spin_unlock(&sd_index_lock);
+	ida_put_index(&sd_index_ida, &sd_index_lock, index);
  out_put:
 	put_disk(gd);
  out_free:
@@ -3064,9 +3054,7 @@ static void scsi_disk_release(struct device *dev)
 	struct scsi_disk *sdkp = to_scsi_disk(dev);
 	struct gendisk *disk = sdkp->disk;
 	
-	spin_lock(&sd_index_lock);
-	ida_remove(&sd_index_ida, sdkp->index);
-	spin_unlock(&sd_index_lock);
+	ida_put_index(&sd_index_ida, &sd_index_lock, sdkp->index);
 
 	blk_integrity_unregister(disk);
 	disk->private_data = NULL;
-- 
2.1.4

[PATCH 3/6] Update soc driver to use new ida helper functions

[Lee Duncan]

This simplifies the calling sequence to
acquire and to release an ida-managed index.

Note that the ida_destroy() calls here previously
had no locking, so this may close a race condition
when errors occur or when removing a soc.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/base/soc.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 39fca01c8fa1..7b632f086524 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -122,18 +122,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
 	}
 
 	/* Fetch a unique (reclaimable) SOC ID. */
-	do {
-		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
-			ret = -ENOMEM;
-			goto out2;
-		}
-
-		spin_lock(&soc_lock);
-		ret = ida_get_new(&soc_ida, &soc_dev->soc_dev_num);
-		spin_unlock(&soc_lock);
-
-	} while (ret == -EAGAIN);
-
+	ret = ida_get_index(&soc_ida, &soc_lock, &soc_dev->soc_dev_num);
 	if (ret)
 		goto out2;
 
@@ -151,7 +140,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
 	return soc_dev;
 
 out3:
-	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+	ida_put_index(&soc_ida, &sock_lock, soc_dev->soc_dev_num);
 out2:
 	kfree(soc_dev);
 out1:
@@ -161,7 +150,7 @@ out1:
 /* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
 void soc_device_unregister(struct soc_device *soc_dev)
 {
-	ida_remove(&soc_ida, soc_dev->soc_dev_num);
+	ida_put_index(&soc_ida, &sock_lock, soc_dev->soc_dev_num);
 
 	device_unregister(&soc_dev->dev);
 }
-- 
2.1.4

[PATCH 4/6] Update soc driver to use new ida helper functions

[Lee Duncan]

This simplifies the calling sequence to
acquire and to release an ida-managed index.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 4a2ef09e6704..ccff4119b554 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -3821,15 +3821,7 @@ static int mtip_block_initialize(struct driver_data *dd)
 	}
 
 	/* Generate the disk name, implemented same as in sd.c */
-	do {
-		if (!ida_pre_get(&rssd_index_ida, GFP_KERNEL))
-			goto ida_get_error;
-
-		spin_lock(&rssd_index_lock);
-		rv = ida_get_new(&rssd_index_ida, &index);
-		spin_unlock(&rssd_index_lock);
-	} while (rv == -EAGAIN);
-
+	rv = ida_get_index(&rssd_index_ida, &rssd_index_lock, &index);
 	if (rv)
 		goto ida_get_error;
 
@@ -3981,9 +3973,7 @@ init_hw_cmds_error:
 block_queue_alloc_init_error:
 	mtip_hw_debugfs_exit(dd);
 disk_index_error:
-	spin_lock(&rssd_index_lock);
-	ida_remove(&rssd_index_ida, index);
-	spin_unlock(&rssd_index_lock);
+	ida_put_index(&rssd_index_ida, &rssd_index_lock, index);
 
 ida_get_error:
 	put_disk(dd->disk);
@@ -4051,9 +4041,7 @@ static int mtip_block_remove(struct driver_data *dd)
 	}
 	dd->disk  = NULL;
 
-	spin_lock(&rssd_index_lock);
-	ida_remove(&rssd_index_ida, dd->index);
-	spin_unlock(&rssd_index_lock);
+	ida_put_index(&rssd_index_ida, &rssd_index_lock, dd->index);
 
 	/* De-initialize the protocol layer. */
 	mtip_hw_exit(dd);
@@ -4092,9 +4080,7 @@ static int mtip_block_shutdown(struct driver_data *dd)
 		dd->queue = NULL;
 	}
 
-	spin_lock(&rssd_index_lock);
-	ida_remove(&rssd_index_ida, dd->index);
-	spin_unlock(&rssd_index_lock);
+	ida_put_index(&rssd_index_ida, &rssd_index_lock, dd->index);
 	return 0;
 }
 
-- 
2.1.4

[PATCH 5/6] Update nvme-core driver to use new ida helper functions

[Lee Duncan]

This simplifies the calling sequence to
acquire and to release an ida-managed index.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/block/nvme-core.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index d1d6141920d3..ab13833d4fde 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2715,15 +2715,7 @@ static int nvme_set_instance(struct nvme_dev *dev)
 {
 	int instance, error;
 
-	do {
-		if (!ida_pre_get(&nvme_instance_ida, GFP_KERNEL))
-			return -ENODEV;
-
-		spin_lock(&dev_list_lock);
-		error = ida_get_new(&nvme_instance_ida, &instance);
-		spin_unlock(&dev_list_lock);
-	} while (error == -EAGAIN);
-
+	error = ida_get_index(&nvme_instance_ida, &dev_list_lock, &instance);
 	if (error)
 		return -ENODEV;
 
@@ -2733,9 +2725,7 @@ static int nvme_set_instance(struct nvme_dev *dev)
 
 static void nvme_release_instance(struct nvme_dev *dev)
 {
-	spin_lock(&dev_list_lock);
-	ida_remove(&nvme_instance_ida, dev->instance);
-	spin_unlock(&dev_list_lock);
+	ida_put_index(&nvme_instance_ida, &dev_list_lock, dev->instance);
 }
 
 static void nvme_free_namespaces(struct nvme_dev *dev)
-- 
2.1.4

[PATCH 6/6] Update rsxx core driver to use new ida helper functions

[Lee Duncan]

This simplifies the calling sequence to
acquire and to release an ida-managed index.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/block/rsxx/core.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index d8b2488aaade..dd23a0e85040 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -774,17 +774,7 @@ static int rsxx_pci_probe(struct pci_dev *dev,
 	card->dev = dev;
 	pci_set_drvdata(dev, card);
 
-	do {
-		if (!ida_pre_get(&rsxx_disk_ida, GFP_KERNEL)) {
-			st = -ENOMEM;
-			goto failed_ida_get;
-		}
-
-		spin_lock(&rsxx_ida_lock);
-		st = ida_get_new(&rsxx_disk_ida, &card->disk_id);
-		spin_unlock(&rsxx_ida_lock);
-	} while (st == -EAGAIN);
-
+	st = ida_get_index(&rsxx_disk_ida, &rsxx_ida_lock, &card->disk_id);
 	if (st)
 		goto failed_ida_get;
 
@@ -987,9 +977,7 @@ failed_request_regions:
 failed_dma_mask:
 	pci_disable_device(dev);
 failed_enable:
-	spin_lock(&rsxx_ida_lock);
-	ida_remove(&rsxx_disk_ida, card->disk_id);
-	spin_unlock(&rsxx_ida_lock);
+	ida_put_index(&rsxx_disk_ida, &rsxx_ida_lock, card->disk_id);
 failed_ida_get:
 	kfree(card);
 
-- 
2.1.4

Re: [PATCH 3/6] Update soc driver to use new ida helper functions

[Lee Duncan]

Oops. I see the subject is wrong on this patch.

I will wait for comments (if any) then re-submit the sequence.

On 09/06/2015 10:59 AM, Lee Duncan wrote:
> This simplifies the calling sequence to
> acquire and to release an ida-managed index.
> 
> Note that the ida_destroy() calls here previously
> had no locking, so this may close a race condition
> when errors occur or when removing a soc.
> 
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>  drivers/base/soc.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 39fca01c8fa1..7b632f086524 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -122,18 +122,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  	}
>  
>  	/* Fetch a unique (reclaimable) SOC ID. */
> -	do {
> -		if (!ida_pre_get(&soc_ida, GFP_KERNEL)) {
> -			ret = -ENOMEM;
> -			goto out2;
> -		}
> -
> -		spin_lock(&soc_lock);
> -		ret = ida_get_new(&soc_ida, &soc_dev->soc_dev_num);
> -		spin_unlock(&soc_lock);
> -
> -	} while (ret == -EAGAIN);
> -
> +	ret = ida_get_index(&soc_ida, &soc_lock, &soc_dev->soc_dev_num);
>  	if (ret)
>  		goto out2;
>  
> @@ -151,7 +140,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  	return soc_dev;
>  
>  out3:
> -	ida_remove(&soc_ida, soc_dev->soc_dev_num);
> +	ida_put_index(&soc_ida, &sock_lock, soc_dev->soc_dev_num);
>  out2:
>  	kfree(soc_dev);
>  out1:
> @@ -161,7 +150,7 @@ out1:
>  /* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
>  void soc_device_unregister(struct soc_device *soc_dev)
>  {
> -	ida_remove(&soc_ida, soc_dev->soc_dev_num);
> +	ida_put_index(&soc_ida, &sock_lock, soc_dev->soc_dev_num);
>  
>  	device_unregister(&soc_dev->dev);
>  }
> 

-- 
Lee Duncan
SUSE Labs

Re: [PATCH 0/6] Create ida helpers and use them

[Johannes Thumshirn]

Lee Duncan <lduncan@suse.com> writes:

> As pointed out by Christoph Hellwig, many of the users
> of the ida index routines call them with the same calling
> pattern, so why not make some helper functions to help
> clean up and simplify the code a bit.
>
> After looking at all the uses of the ida routines, I
> noticed that most of clients of this API used the
> same sequence -- but not all of them. So I picked
> the most common and seemingly most correct sequence,
> and I made inline helper functions for that, creating:
>
>  - ida_get_index() -- get an 'ida' index, with proper
>    locking and retry
>    
>  - ida_put_index() -- release/return an index allocated
>    by ida_get_index(), with proper locking
>
> I converted the following drivers to use the new helper
> routines, because they matched the calling sequence
> used by the helper functions:
>
>   scsi/sd.c                     SCSI disk
>   base/soc.c                    sound?
>   block/mtip32xxx/mtip32xx.c    PCI block?
>   block/nvme-core.c             Intel SSD?
>   block/rsxx/core.c             IBM Flash?
>
> Obviously, only one of these is a SCSI driver, so
> I hope this is still the right place to post this.

I think drawing this to attention of a wider audience (i.e. add lkml to
Cc) would be a good idea.


Other than that, I'm OK with it.

>
> NOTE 1: the soc.c driver actually had normal locking
> around it's 'get' call, but it had no locking around
> it's 'put' (release) calls, so since the helper routines
> have locking, that means the soc.c driver now has
> locking when it releases an index.
>
> There were other drivers that used ida in a way
> that meant I could not easily modify them:
>
>   drivers/iommu/iommu.c:
>     - uses unlikely(...)
>     - uses a mutex instead of spinlock
>
>   drivers/misc/cb710/core.c:
>     - calls return from within the 'get' while loop
>     - uses spin_lock_irq*()
>
>   drivers/scsi/osd/osd_uld.c:
>     - uses no locking at all for indexes [See NOTE 2]
>
>   drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
>     - uses 'unlikely()' in 'get' while loop
>     - uses 'goto's out of 'get' while loop
>

Regarding the above, maybe Cc the maintainers of said drivers to the
cover letter, so they can comment on the IDA usage.


> NOTE 2: Not sure if this is a bug in the osd code, since
> I did not look at it very closely.
>
> The only client driver changes I was able to functionally
> test where the sd.c changes. I also tested the helper
> functions using the hosts.c module, where they worked
> fine, but those changes were later reverted in favor of
> using idr there instead. (See previous patch series.)
>
> Lee Duncan (6):
>   Add ida helper routines
>   SCSI: Update sd driver to use new ida helper functions.
>   Update soc driver to use new ida helper functions.
>   Update soc driver to use new ida helper functions.
>   Update nvme-core driver to use new ida helper functions.
>   Update rsxx core driver to use new ida helper functions.
>
>  drivers/base/soc.c                | 17 +++-------------
>  drivers/block/mtip32xx/mtip32xx.c | 22 ++++----------------
>  drivers/block/nvme-core.c         | 14 ++-----------
>  drivers/block/rsxx/core.c         | 16 ++-------------
>  drivers/scsi/sd.c                 | 18 +++--------------
>  include/linux/idr.h               | 42 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 56 insertions(+), 73 deletions(-)

-- 
Johannes Thumshirn                                           Storage
jthumshirn@suse.de                                 +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600  D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html