Re: [PATCH] Fix fnic driver to remove bogus ratelimit messages.

[Lee Duncan]

On 3/24/21 3:41 PM, Joe Perches wrote:
> On Tue, 2021-03-23 at 10:27 -0700, ldun...@suse.com wrote:
>> From: Lee Duncan 
>>
>> Commit b43abcbbd5b1 ("scsi: fnic: Ratelimit printks to avoid
>> looding when vlan is not set by the switch.i") added
>> printk_ratelimit() in front of a couple of debug-mode
>> messages, to reduce logging overrun when debugging the
>> driver. The code:
>>
>>>   if (printk_ratelimit())
>>>   FNIC_FCS_DBG(KERN_DEBUG, fnic->lport->host,
>>> "Start VLAN Discovery\n");
>>
>> ends up calling printk_ratelimit() quite often, triggering
>> many kernel messages about callbacks being surpressed.
>>
>> The fix is to decompose FNIC_FCS_DBG(), then change the order
>> of checks so that printk_ratelimit() is only called if
>> driver debugging is enabled.
> 
> Please make sure to cc the fnic maintainers when submitting patches
> to their drivers.
> 
> $ ./scripts/get_maintainer.pl drivers/scsi/fnic/
> Satish Kharat  (supporter:CISCO FCOE HBA DRIVER)
> Sesidhar Baddela  (supporter:CISCO FCOE HBA DRIVER)
> Karan Tilak Kumar  (supporter:CISCO FCOE HBA DRIVER)
> "James E.J. Bottomley"  (maintainer:SCSI SUBSYSTEM)
> "Martin K. Petersen"  (maintainer:SCSI SUBSYSTEM)
> linux-s...@vger.kernel.org (open list:CISCO FCOE HBA DRIVER)
> linux-kernel@vger.kernel.org (open list)
> 
> 
> My preference would be to rewrite the FNIC__DBG macros to use
> a single fnic_dbg macro and add a new fnic_dbg_ratelimited macro.
> 
> Something like the below:
> 
> This converts a few uses at KERN_INFO to KERN_DEBUG, only uses fnic
> and adds a macro concatenation instead of multiple macros.
> 
> Miscellanea:
> 
> o Add missing newlines
> o Coalesce formats
> o Align arguments
> 
> ---
> 
> compile tested only
> 
>  drivers/scsi/fnic/fnic.h  |  45 +++
>  drivers/scsi/fnic/fnic_fcs.c  |  92 +-
>  drivers/scsi/fnic/fnic_isr.c  |   9 +-
>  drivers/scsi/fnic/fnic_main.c |   9 +-
>  drivers/scsi/fnic/fnic_scsi.c | 280 
> --
>  5 files changed, 162 insertions(+), 273 deletions(-)
> 
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 69f373b53132..f12cd6ed9296 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -130,36 +130,27 @@
>  extern unsigned int fnic_log_level;
>  extern unsigned int io_completions;
>  
> -#define FNIC_MAIN_LOGGING 0x01
> -#define FNIC_FCS_LOGGING 0x02
> -#define FNIC_SCSI_LOGGING 0x04
> -#define FNIC_ISR_LOGGING 0x08
> -
> -#define FNIC_CHECK_LOGGING(LEVEL, CMD)   \
> -do { \
> - if (unlikely(fnic_log_level & LEVEL))   \
> - do {\
> - CMD;\
> - } while (0);\
> +#define FNIC_DBG_MAIN0x01
> +#define FNIC_DBG_FCS 0x02
> +#define FNIC_DBG_SCSI0x04
> +#define FNIC_DBG_ISR 0x08
> +
> +#define fnic_dbg(fnic, TYPE, fmt, ...)   
> \
> +do { \
> + if (unlikely(fnic_log_level & FNIC_DBG_##TYPE)) \
> + shost_printk(KERN_DEBUG, (fnic)->lport->host,   \
> +  fmt, ##__VA_ARGS__);   \
>  } while (0)
>  
> -#define FNIC_MAIN_DBG(kern_level, host, fmt, args...)\
> - FNIC_CHECK_LOGGING(FNIC_MAIN_LOGGING,   \
> -  shost_printk(kern_level, host, fmt, ##args);)
> -
> -#define FNIC_FCS_DBG(kern_level, host, fmt, args...) \
> - FNIC_CHECK_LOGGING(FNIC_FCS_LOGGING,\
> -  shost_printk(kern_level, host, fmt, ##args);)
> -
> -#define FNIC_SCSI_DBG(kern_level, host, fmt, args...)\
> - FNIC_CHECK_LOGGING(FNIC_SCSI_LOGGING,   \
> -  shost_printk(kern_level, host, fmt, ##args);)
> -
> -#define FNIC_ISR_DBG(kern_level, host, fmt, args...) \
> - FNIC_CHECK_LOGGING(FNIC_ISR_LOGGING,\
> -  shost_printk(kern_level, host, fmt, ##args);)
> +#define fnic_dbg_ratelimited(fnic, TYPE, fmt, ...)   \
> +do { \
> + if (unlikely(fnic_log_level & FNIC_DBG_##TYPE) &&   \
> +

Re: [PATCH] Fix fnic driver to remove bogus ratelimit messages.

[Lee Duncan]

On 3/24/21 3:41 PM, Joe Perches wrote:
> On Tue, 2021-03-23 at 10:27 -0700, ldun...@suse.com wrote:
>> From: Lee Duncan 
>>
>> Commit b43abcbbd5b1 ("scsi: fnic: Ratelimit printks to avoid
>> looding when vlan is not set by the switch.i") added
>> printk_ratelimit() in front of a couple of debug-mode
>> messages, to reduce logging overrun when debugging the
>> driver. The code:
>>
>>>   if (printk_ratelimit())
>>>   FNIC_FCS_DBG(KERN_DEBUG, fnic->lport->host,
>>> "Start VLAN Discovery\n");
>>
>> ends up calling printk_ratelimit() quite often, triggering
>> many kernel messages about callbacks being surpressed.
>>
>> The fix is to decompose FNIC_FCS_DBG(), then change the order
>> of checks so that printk_ratelimit() is only called if
>> driver debugging is enabled.
> 
> Please make sure to cc the fnic maintainers when submitting patches
> to their drivers.
> 
> $ ./scripts/get_maintainer.pl drivers/scsi/fnic/
> Satish Kharat  (supporter:CISCO FCOE HBA DRIVER)
> Sesidhar Baddela  (supporter:CISCO FCOE HBA DRIVER)
> Karan Tilak Kumar  (supporter:CISCO FCOE HBA DRIVER)
> "James E.J. Bottomley"  (maintainer:SCSI SUBSYSTEM)
> "Martin K. Petersen"  (maintainer:SCSI SUBSYSTEM)
> linux-s...@vger.kernel.org (open list:CISCO FCOE HBA DRIVER)
> linux-kernel@vger.kernel.org (open list)

Apologies for leaving the first three of those off. James and Martin are
already seeing the submission to linux-scsi and linux-kernel.

> 
> 
> My preference would be to rewrite the FNIC__DBG macros to use
> a single fnic_dbg macro and add a new fnic_dbg_ratelimited macro.
> 
> Something like the below:

My changes was purposely meant to be minimal. I though about adding a
macro for debug printing with ratelimit, but since there were only two
cases where that would be used, and I was trying for minimal change, I
didn't redesign the debugging statements.

But I'm happy to withdraw my submission and support yours, since it also
fixes the problem.

In this case, if this was my driver, I'd be tempted to optimize this
even further and ifdef out that debug code, since it's so ridiculously
verbose when used.

If you submit this, please cc me so I can withdraw my submission.

> 
> This converts a few uses at KERN_INFO to KERN_DEBUG, only uses fnic
> and adds a macro concatenation instead of multiple macros.
> 
> Miscellanea:
> 
> o Add missing newlines
> o Coalesce formats
> o Align arguments
> 
> ---
> 
> compile tested only
> 
>  drivers/scsi/fnic/fnic.h  |  45 +++
>  drivers/scsi/fnic/fnic_fcs.c  |  92 +-
>  drivers/scsi/fnic/fnic_isr.c  |   9 +-
>  drivers/scsi/fnic/fnic_main.c |   9 +-
>  drivers/scsi/fnic/fnic_scsi.c | 280 
> --
>  5 files changed, 162 insertions(+), 273 deletions(-)
> 
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 69f373b53132..f12cd6ed9296 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -130,36 +130,27 @@
>  extern unsigned int fnic_log_level;
>  extern unsigned int io_completions;
>  
> -#define FNIC_MAIN_LOGGING 0x01
> -#define FNIC_FCS_LOGGING 0x02
> -#define FNIC_SCSI_LOGGING 0x04
> -#define FNIC_ISR_LOGGING 0x08
> -
> -#define FNIC_CHECK_LOGGING(LEVEL, CMD)   \
> -do { \
> - if (unlikely(fnic_log_level & LEVEL))   \
> - do {\
> - CMD;\
> - } while (0);\
> +#define FNIC_DBG_MAIN0x01
> +#define FNIC_DBG_FCS 0x02
> +#define FNIC_DBG_SCSI0x04
> +#define FNIC_DBG_ISR 0x08
> +
> +#define fnic_dbg(fnic, TYPE, fmt, ...)   
> \
> +do { \
> + if (unlikely(fnic_log_level & FNIC_DBG_##TYPE)) \
> + shost_printk(KERN_DEBUG, (fnic)->lport->host,   \
> +  fmt, ##__VA_ARGS__);   \
>  } while (0)
>  
> -#define FNIC_MAIN_DBG(kern_level, host, fmt, args...)\
> - FNIC_CHECK_LOGGING(FNIC_MAIN_LOGGING,   \
> -  shost_printk(kern_level, host, fmt, ##args);)
> -
> -#define FNIC_FCS_DBG(kern_level, host, fmt, args...) \
> - FNIC_CHECK_LOGGING(FNIC_FCS_LOGGING,\
> -  shost_printk(k

Re: [PATCH 5.10 083/102] scsi: iscsi: Restrict sessions and handles to admin capabilities

[Lee Duncan]

On 3/5/21 2:42 PM, Pavel Machek wrote:
> Hi!
> 
>> From: Lee Duncan 
>>
>> commit 688e8128b7a92df982709a4137ea4588d16f24aa upstream.
>>
>> Protect the iSCSI transport handle, available in sysfs, by requiring
>> CAP_SYS_ADMIN to read it. Also protect the netlink socket by restricting
>> reception of messages to ones sent with CAP_SYS_ADMIN. This disables
>> normal users from being able to end arbitrary iSCSI sessions.
> 
> Should not normal filesystem permissions be used?
> 
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -132,6 +132,9 @@ show_transport_handle(struct device *dev
>>char *buf)
>>  {
>>  struct iscsi_internal *priv = dev_to_iscsi_internal(dev);
>> +
>> +if (!capable(CAP_SYS_ADMIN))
>> +return -EACCES;
>>  return sprintf(buf, "%llu\n", (unsigned long 
>> long)iscsi_handle(priv->iscsi_transport));
>>  }
>>  static DEVICE_ATTR(handle, S_IRUGO, show_transport_handle, NULL);
> 
> AFAICT we make the file 0444 (world readable) and then fail the read
> with capability check. If the file is not supposed to be
> world-readable, it should have 0400 permissions, right?
> 
> Best regards,
>   Pavel
> 

I am ok with changing file permissions, but there's nothing wrong with
checking capabilities upon entry, as well, since capability checks are a
higher degree of security than ACLs.
-- 
Lee Duncan

Re: [PATCH] fnic: fixup patch to resolve stack frame issues

[Lee Duncan]

On 1/26/21 11:46 PM, Greg KH wrote:
> On Tue, Jan 26, 2021 at 05:21:24PM -0800, Lee Duncan wrote:
>> From: Hannes Reinecke 
>>
>> Commit 42ec15ceaea7 fixed a gcc issue with unused variables, but
>> introduced errors since it allocated an array of two u64-s but
>> then used more than that. Set the arrays to the proper size.
>>
>> Fixes: 42ec15ceaea74b5f7a621fc6686cbf69ca66c4cf
> 
> Please use the documented way to show sha1 commit ids to make it easier
> to understand:
> 
>   42ec15ceaea7 ("scsi: fnic: fix invalid stack access")
> 
> thanks,
> 
> greg k-h
> 

I will Greg. Thank you.

Please delete this patch, though. It is not correct.

My apologies for the churn.
-- 
Lee Duncan

Re: [PATCH] fnic: fixup patch to resolve stack frame issues

[Lee Duncan]

On 1/26/21 5:21 PM, Lee Duncan wrote:
> From: Hannes Reinecke 
> 
> Commit 42ec15ceaea7 fixed a gcc issue with unused variables, but
> introduced errors since it allocated an array of two u64-s but
> then used more than that. Set the arrays to the proper size.
> 
> Fixes: 42ec15ceaea74b5f7a621fc6686cbf69ca66c4cf
> Cc: sta...@vger.kernel.org
> Signed-off-by: Hannes Reinecke 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/scsi/fnic/vnic_dev.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/fnic/vnic_dev.c b/drivers/scsi/fnic/vnic_dev.c
> index 5988c300cc82..d2064b89 100644
> --- a/drivers/scsi/fnic/vnic_dev.c
> +++ b/drivers/scsi/fnic/vnic_dev.c
> @@ -697,7 +697,7 @@ int vnic_dev_hang_notify(struct vnic_dev *vdev)
>  
>  int vnic_dev_mac_addr(struct vnic_dev *vdev, u8 *mac_addr)
>  {
> - u64 a[2] = {};
> + u64 a[ETH_ALEN] = {};
>   int wait = 1000;
>   int err, i;
>  
> @@ -734,7 +734,7 @@ void vnic_dev_packet_filter(struct vnic_dev *vdev, int 
> directed, int multicast,
>  
>  void vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
>  {
> - u64 a[2] = {};
> + u64 a[ETH_ALEN] = {};
>   int wait = 1000;
>   int err;
>   int i;
> @@ -749,7 +749,7 @@ void vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
>  
>  void vnic_dev_del_addr(struct vnic_dev *vdev, u8 *addr)
>  {
> - u64 a[2] = {};
> + u64 a[ETH_ALEN] = {};
>   int wait = 1000;
>   int err;
>   int i;
> 

This may not be correct. Please do not review this yet. I will re-submit
once I clear up my confusion.

-- 
Lee Duncan

[PATCH] fnic: fixup patch to resolve stack frame issues

[Lee Duncan]

From: Hannes Reinecke 

Commit 42ec15ceaea7 fixed a gcc issue with unused variables, but
introduced errors since it allocated an array of two u64-s but
then used more than that. Set the arrays to the proper size.

Fixes: 42ec15ceaea74b5f7a621fc6686cbf69ca66c4cf
Cc: sta...@vger.kernel.org
Signed-off-by: Hannes Reinecke 
Signed-off-by: Lee Duncan 
---
 drivers/scsi/fnic/vnic_dev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/fnic/vnic_dev.c b/drivers/scsi/fnic/vnic_dev.c
index 5988c300cc82..d2064b89 100644
--- a/drivers/scsi/fnic/vnic_dev.c
+++ b/drivers/scsi/fnic/vnic_dev.c
@@ -697,7 +697,7 @@ int vnic_dev_hang_notify(struct vnic_dev *vdev)
 
 int vnic_dev_mac_addr(struct vnic_dev *vdev, u8 *mac_addr)
 {
-   u64 a[2] = {};
+   u64 a[ETH_ALEN] = {};
int wait = 1000;
int err, i;
 
@@ -734,7 +734,7 @@ void vnic_dev_packet_filter(struct vnic_dev *vdev, int 
directed, int multicast,
 
 void vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
 {
-   u64 a[2] = {};
+   u64 a[ETH_ALEN] = {};
int wait = 1000;
int err;
int i;
@@ -749,7 +749,7 @@ void vnic_dev_add_addr(struct vnic_dev *vdev, u8 *addr)
 
 void vnic_dev_del_addr(struct vnic_dev *vdev, u8 *addr)
 {
-   u64 a[2] = {};
+   u64 a[ETH_ALEN] = {};
int wait = 1000;
int err;
int i;
-- 
2.26.2

Re: [PATCH v2 1/1] scsi: libiscsi: fix NOP race condition

[Lee Duncan]

On 10/8/20 1:54 PM, Mike Christie wrote:
> On 10/8/20 12:11 PM, Mike Christie wrote:
>> On 9/25/20 1:41 PM, ldun...@suse.com wrote:
>>> From: Lee Duncan 
>>>
>>> iSCSI NOPs are sometimes "lost", mistakenly sent to the
>>> user-land iscsid daemon instead of handled in the kernel,
>>> as they should be, resulting in a message from the daemon like:
>>>
>>>> iscsid: Got nop in, but kernel supports nop handling.
>>>
>>> This can occur because of the forward- and back-locks
>>> in the kernel iSCSI code, and the fact that an iSCSI NOP
>>> response can be processed before processing of the NOP send
>>> is complete. This can result in "conn->ping_task" being NULL
>>> in iscsi_nop_out_rsp(), when the pointer is actually in
>>> the process of being set.
>>>
>>> To work around this, we add a new state to the "ping_task"
>>> pointer. In addition to NULL (not assigned) and a pointer
>>> (assigned), we add the state "being set", which is signaled
>>> with an INVALID pointer (using "-1").
>>>
>>> Signed-off-by: Lee Duncan 
>>> ---
>>>  drivers/scsi/libiscsi.c | 13 ++---
>>>  include/scsi/libiscsi.h |  3 +++
>>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>>> index 1e9c3171fa9f..cade108c33b6 100644
>>> --- a/drivers/scsi/libiscsi.c
>>> +++ b/drivers/scsi/libiscsi.c
>>> @@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
>>> iscsi_hdr *hdr,
>>>task->conn->session->age);
>>> }
>>>  
>>> +   if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
>>> +   WRITE_ONCE(conn->ping_task, task);
>>> +
>>> if (!ihost->workq) {
>>> if (iscsi_prep_mgmt_task(conn, task))
>>> goto free_task;
>>
>> I think the API gets a little weird now where in some cases
>> __iscsi_conn_send_pdu checks the opcode to see what type of request
>> it is but above we the caller sets the ping_task.
>>
>> For login, tmfs and passthrough, we assume the __iscsi_conn_send_pdu
>> has sent or cleaned up everything. I think it might be nicer to just
>> have __iscsi_conn_send_pdu set the ping_task field before doing the
>> xmit/queue call. It would then work similar to the conn->login_task
>> case where that function knows about that special task too.
>>
>> So in __iscsi_conn_send_pdu add a "if (opcode == ISCSI_OP_NOOP_OUT)",
>> and check if it's a nop we need to track. If so set conn->ping_task.
>>
> Ignore this. It won't work nicely either. To figure out if the nop is
> our internal transport test ping vs a userspace ping that also needs
> a reply, we would need to do something like you did above so there is
> no point.
> 

Hi Mike:

I've read this a few times, and I'm still no sure I'm parsing it correctly.

Are you saying that my original patch submission is ok, or are you
saying there's nothing we can do and we're up the proverbial creek?
-- 
Lee Duncan

Re: [PATCH v2 1/1] scsi: libiscsi: fix NOP race condition

[Lee Duncan]

On 9/25/20 11:41 AM, ldun...@suse.com wrote:
> From: Lee Duncan 
> 
> iSCSI NOPs are sometimes "lost", mistakenly sent to the
> user-land iscsid daemon instead of handled in the kernel,
> as they should be, resulting in a message from the daemon like:
> 
>> iscsid: Got nop in, but kernel supports nop handling.
> 
> This can occur because of the forward- and back-locks
> in the kernel iSCSI code, and the fact that an iSCSI NOP
> response can be processed before processing of the NOP send
> is complete. This can result in "conn->ping_task" being NULL
> in iscsi_nop_out_rsp(), when the pointer is actually in
> the process of being set.
> 
> To work around this, we add a new state to the "ping_task"
> pointer. In addition to NULL (not assigned) and a pointer
> (assigned), we add the state "being set", which is signaled
> with an INVALID pointer (using "-1").
> 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/scsi/libiscsi.c | 13 ++---
>  include/scsi/libiscsi.h |  3 +++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 1e9c3171fa9f..cade108c33b6 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -738,6 +738,9 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
> iscsi_hdr *hdr,
>  task->conn->session->age);
>   }
>  
> + if (unlikely(READ_ONCE(conn->ping_task) == INVALID_SCSI_TASK))
> + WRITE_ONCE(conn->ping_task, task);
> +
>   if (!ihost->workq) {
>   if (iscsi_prep_mgmt_task(conn, task))
>   goto free_task;
> @@ -941,8 +944,11 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
> struct iscsi_nopin *rhdr)
>  struct iscsi_nopout hdr;
>   struct iscsi_task *task;
>  
> - if (!rhdr && conn->ping_task)
> - return -EINVAL;
> + if (!rhdr) {
> + if (READ_ONCE(conn->ping_task))
> + return -EINVAL;
> + WRITE_ONCE(conn->ping_task, INVALID_SCSI_TASK);
> + }
>  
>   memset(&hdr, 0, sizeof(struct iscsi_nopout));
>   hdr.opcode = ISCSI_OP_NOOP_OUT | ISCSI_OP_IMMEDIATE;
> @@ -957,11 +963,12 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
> struct iscsi_nopin *rhdr)
>  
>   task = __iscsi_conn_send_pdu(conn, (struct iscsi_hdr *)&hdr, NULL, 0);
>   if (!task) {
> + if (!rhdr)
> + WRITE_ONCE(conn->ping_task, NULL);
>   iscsi_conn_printk(KERN_ERR, conn, "Could not send nopout\n");
>   return -EIO;
>   } else if (!rhdr) {
>   /* only track our nops */
> - conn->ping_task = task;
>   conn->last_ping = jiffies;
>   }
>  
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index c25fb86ffae9..b3bbd10eb3f0 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -132,6 +132,9 @@ struct iscsi_task {
>   void*dd_data;   /* driver/transport data */
>  };
>  
> +/* invalid scsi_task pointer */
> +#define  INVALID_SCSI_TASK   (struct iscsi_task *)-1l
> +
>  static inline int iscsi_task_has_unsol_data(struct iscsi_task *task)
>  {
>   return task->unsol_r2t.data_length > task->unsol_r2t.sent;
> 

Ping?

-- 
Lee Duncan

Re: [PATCH v9 6/7] scsi: libiscsi: use sendpage_ok() in iscsi_tcp_segment_map()

[Lee Duncan]

On 10/1/20 12:54 AM, Coly Li wrote:
> In iscsci driver, iscsi_tcp_segment_map() uses the following code to
> check whether the page should or not be handled by sendpage:
> if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg)))
> 
> The "page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg)" part is to
> make sure the page can be sent to network layer's zero copy path. This
> part is exactly what sendpage_ok() does.
> 
> This patch uses  use sendpage_ok() in iscsi_tcp_segment_map() to replace
> the original open coded checks.
> 
> Signed-off-by: Coly Li 
> Acked-by: Martin K. Petersen 
> Cc: Vasily Averin 
> Cc: Cong Wang 
> Cc: Mike Christie 
> Cc: Lee Duncan 
> Cc: Chris Leech 
> Cc: Christoph Hellwig 
> Cc: Hannes Reinecke 
> ---
>  drivers/scsi/libiscsi_tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
> index 37e5d4e48c2f..83f14b2c8804 100644
> --- a/drivers/scsi/libiscsi_tcp.c
> +++ b/drivers/scsi/libiscsi_tcp.c
> @@ -128,7 +128,7 @@ static void iscsi_tcp_segment_map(struct iscsi_segment 
> *segment, int recv)
>* coalescing neighboring slab objects into a single frag which
>* triggers one of hardened usercopy checks.
>*/
> - if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg)))
> + if (!recv && sendpage_ok(sg_page(sg)))
>   return;
>  
>   if (recv) {
> 

Reviewed-by: Lee Duncan 

Re: [PATCH] scsi: Fix reference count leak in iscsi_boot_create_kobj.

[Lee Duncan]

On 5/28/20 1:13 PM, wu000...@umn.edu wrote:
> From: Qiushi Wu 
> 
> kobject_init_and_add() should be handled when it return an error,
> because kobject_init_and_add() takes reference even when it fails.
> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object. Previous
> commit "b8eb718348b8" fixed a similar problem. Thus replace calling
> kfree() by calling kobject_put().
> 
> Signed-off-by: Qiushi Wu 
> ---
>  drivers/scsi/iscsi_boot_sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/iscsi_boot_sysfs.c b/drivers/scsi/iscsi_boot_sysfs.c
> index e4857b728033..a64abe38db2d 100644
> --- a/drivers/scsi/iscsi_boot_sysfs.c
> +++ b/drivers/scsi/iscsi_boot_sysfs.c
> @@ -352,7 +352,7 @@ iscsi_boot_create_kobj(struct iscsi_boot_kset *boot_kset,
>   boot_kobj->kobj.kset = boot_kset->kset;
>   if (kobject_init_and_add(&boot_kobj->kobj, &iscsi_boot_ktype,
>NULL, name, index)) {
> - kfree(boot_kobj);
> + kobject_put(&boot_kobj->kobj);
>   return NULL;
>   }
>   boot_kobj->data = data;
> 

Reviewed-by: Lee Duncan 

Re: [PATCH] scsi: qedi: remove unused variable udev & uctrl

[Lee Duncan]

On 5/5/20 5:19 AM, Xie XiuQi wrote:
> uctrl and udev are unused after commit 9632a6b4b747
> ("scsi: qedi: Move LL2 producer index processing in BH.")
> 
> Remove them.
> 
> Signed-off-by: Xie XiuQi 
> ---
>  drivers/scsi/qedi/qedi_main.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> index b995b19865ca..313f7e10aed9 100644
> --- a/drivers/scsi/qedi/qedi_main.c
> +++ b/drivers/scsi/qedi/qedi_main.c
> @@ -658,8 +658,6 @@ static struct qedi_ctx *qedi_host_alloc(struct pci_dev 
> *pdev)
>  static int qedi_ll2_rx(void *cookie, struct sk_buff *skb, u32 arg1, u32 arg2)
>  {
>   struct qedi_ctx *qedi = (struct qedi_ctx *)cookie;
> - struct qedi_uio_dev *udev;
> - struct qedi_uio_ctrl *uctrl;
>   struct skb_work_list *work;
>   struct ethhdr *eh;
>  
> @@ -698,9 +696,6 @@ static int qedi_ll2_rx(void *cookie, struct sk_buff *skb, 
> u32 arg1, u32 arg2)
> "Allowed frame ethertype [0x%x] len [0x%x].\n",
> eh->h_proto, skb->len);
>  
> - udev = qedi->udev;
> - uctrl = udev->uctrl;
> -
>   work = kzalloc(sizeof(*work), GFP_ATOMIC);
>   if (!work) {
>   QEDI_WARN(&qedi->dbg_ctx,
> 

Reviewed-by: Lee Duncan 

-- 
Lee

Re: [PATCH v2] scsi:libiscsi: Hold back_lock when calling iscsi_complete_task

[Lee Duncan]

On 3/6/19 10:23 AM, Chris Leech wrote:
> On Mon, Feb 25, 2019 at 09:41:30AM -0800, Lee Duncan wrote:
>> From: Lee Duncan 
>>
>> If there is an error queueing an iscsi command in
>> iscsi_queuecommand(), for example if the transport fails
>> to take the command in sessuin->tt->xmit_task(), then
>> the error path can call iscsi_complete_task() without
>> first aquiring the back_lock as required. This can
>> lead to things like ITT pool can get corrupt, resulting
>> in duplicate ITTs being sent out.
>>
>> The solution is to hold the back_lock around
>> iscsi_complete_task() calls, and to add a little commenting
>> to help others understand when back_lock must be held.
>>
>> Signed-off-by: Lee Duncan 
>> ---
> 
> Lee,
> 
> Quick question, can you confirm that you tested this with lockdep?
> 
> It seems right to me, it's just that we've been hit with lockdep
> problems dealing with these locks before.
> 
> - Chris

I'm glad you asked, to keep me honest -- I had not done it yet, because
it seemed obvious to me.

But I did check today, and did not find any deadlock nor leaks. I was
testing on a 5.0.0-1 kernel.

> 
>>  drivers/scsi/libiscsi.c | 22 --
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index b8d325ce8754..ef7871e8c6bd 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -838,7 +838,7 @@ EXPORT_SYMBOL_GPL(iscsi_conn_send_pdu);
>>   * @datalen: len of buffer
>>   *
>>   * iscsi_cmd_rsp sets up the scsi_cmnd fields based on the PDU and
>> - * then completes the command and task.
>> + * then completes the command and task. called under back_lock
>>   **/
>>  static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, struct iscsi_hdr 
>> *hdr,
>> struct iscsi_task *task, char *data,
>> @@ -941,6 +941,9 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, 
>> struct iscsi_hdr *hdr,
>>   * @conn: iscsi connection
>>   * @hdr:  iscsi pdu
>>   * @task: scsi command task
>> + *
>> + * iscsi_data_in_rsp sets up the scsi_cmnd fields based on the data received
>> + * then completes the command and task. called under back_lock
>>   **/
>>  static void
>>  iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>> @@ -1025,6 +1028,16 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
>> struct iscsi_nopin *rhdr)
>>  return 0;
>>  }
>>  
>> +/**
>> + * iscsi_nop_out_rsp - SCSI NOP Response processing
>> + * @task: scsi command task
>> + * @nop: the nop structure
>> + * @data: where to put the data
>> + * @datalen: length of data
>> + *
>> + * iscsi_nop_out_rsp handles nop response from use or
>> + * from user space. called under back_lock
>> + **/
>>  static int iscsi_nop_out_rsp(struct iscsi_task *task,
>>   struct iscsi_nopin *nop, char *data, int datalen)
>>  {
>> @@ -1791,7 +1804,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
>> scsi_cmnd *sc)
>>  return 0;
>>  
>>  prepd_reject:
>> +spin_lock_bh(&session->back_lock);
>>  iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
>> +spin_unlock_bh(&session->back_lock);
>>  reject:
>>  spin_unlock_bh(&session->frwd_lock);
>>  ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
>> @@ -1799,7 +1814,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
>> scsi_cmnd *sc)
>>  return SCSI_MLQUEUE_TARGET_BUSY;
>>  
>>  prepd_fault:
>> +spin_lock_bh(&session->back_lock);
>>  iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
>> +spin_unlock_bh(&session->back_lock);
>>  fault:
>>  spin_unlock_bh(&session->frwd_lock);
>>  ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
>> @@ -3121,8 +3138,9 @@ fail_mgmt_tasks(struct iscsi_session *session, struct 
>> iscsi_conn *conn)
>>  state = ISCSI_TASK_ABRT_SESS_RECOV;
>>  if (task->state == ISCSI_TASK_PENDING)
>>  state = ISCSI_TASK_COMPLETED;
>> +spin_lock_bh(&session->back_lock);
>>  iscsi_complete_task(task, state);
>> -
>> +spin_unlock_bh(&session->back_lock);
>>  }
>>  }
>>  
>> -- 
>> 2.16.4
>>
> 

[PATCH v2] scsi:libiscsi: Hold back_lock when calling iscsi_complete_task

[Lee Duncan]

From: Lee Duncan 

If there is an error queueing an iscsi command in
iscsi_queuecommand(), for example if the transport fails
to take the command in sessuin->tt->xmit_task(), then
the error path can call iscsi_complete_task() without
first aquiring the back_lock as required. This can
lead to things like ITT pool can get corrupt, resulting
in duplicate ITTs being sent out.

The solution is to hold the back_lock around
iscsi_complete_task() calls, and to add a little commenting
to help others understand when back_lock must be held.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/libiscsi.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b8d325ce8754..ef7871e8c6bd 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -838,7 +838,7 @@ EXPORT_SYMBOL_GPL(iscsi_conn_send_pdu);
  * @datalen: len of buffer
  *
  * iscsi_cmd_rsp sets up the scsi_cmnd fields based on the PDU and
- * then completes the command and task.
+ * then completes the command and task. called under back_lock
  **/
 static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
   struct iscsi_task *task, char *data,
@@ -941,6 +941,9 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, 
struct iscsi_hdr *hdr,
  * @conn: iscsi connection
  * @hdr:  iscsi pdu
  * @task: scsi command task
+ *
+ * iscsi_data_in_rsp sets up the scsi_cmnd fields based on the data received
+ * then completes the command and task. called under back_lock
  **/
 static void
 iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
@@ -1025,6 +1028,16 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
struct iscsi_nopin *rhdr)
return 0;
 }
 
+/**
+ * iscsi_nop_out_rsp - SCSI NOP Response processing
+ * @task: scsi command task
+ * @nop: the nop structure
+ * @data: where to put the data
+ * @datalen: length of data
+ *
+ * iscsi_nop_out_rsp handles nop response from use or
+ * from user space. called under back_lock
+ **/
 static int iscsi_nop_out_rsp(struct iscsi_task *task,
 struct iscsi_nopin *nop, char *data, int datalen)
 {
@@ -1791,7 +1804,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)
return 0;
 
 prepd_reject:
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
+   spin_unlock_bh(&session->back_lock);
 reject:
spin_unlock_bh(&session->frwd_lock);
ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
@@ -1799,7 +1814,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)
return SCSI_MLQUEUE_TARGET_BUSY;
 
 prepd_fault:
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
+   spin_unlock_bh(&session->back_lock);
 fault:
spin_unlock_bh(&session->frwd_lock);
ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
@@ -3121,8 +3138,9 @@ fail_mgmt_tasks(struct iscsi_session *session, struct 
iscsi_conn *conn)
state = ISCSI_TASK_ABRT_SESS_RECOV;
if (task->state == ISCSI_TASK_PENDING)
state = ISCSI_TASK_COMPLETED;
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, state);
-
+   spin_unlock_bh(&session->back_lock);
}
 }
 
-- 
2.16.4

Re: [RESEND] [PATCH] Hold back_lock when calling iscsi_complete_task

[Lee Duncan]

On 2/25/19 2:02 AM, Johannes Thumshirn wrote:
> On 22/02/2019 17:29, Lee Duncan wrote:
>> From: Lee Duncan 
>>
>> If there is an error queueing an iscsi command in
>> iscsi_queuecommand(), for example if the transport fails
>> to take the command in sessuin->tt->xmit_task(), then
>> the error path can call iscsi_complete_task() without
>> first aquiring the back_lock as required. This can
>> lead to things like ITT pool can get corrupt, resulting
>> in duplicate ITTs being sent out.
>>
>> The solution is to hold the back_lock around
>> iscsi_complete_task() calls, and to add a little commenting
>> to help others understand when back_lock must be held.
> 
> You're missing a S-o-b here.
> 
> Byte,
>   Johannes
> 

Doh!

I will resend.

-- 
The Lee-Man

[RESEND] [PATCH] Hold back_lock when calling iscsi_complete_task

[Lee Duncan]

From: Lee Duncan 

If there is an error queueing an iscsi command in
iscsi_queuecommand(), for example if the transport fails
to take the command in sessuin->tt->xmit_task(), then
the error path can call iscsi_complete_task() without
first aquiring the back_lock as required. This can
lead to things like ITT pool can get corrupt, resulting
in duplicate ITTs being sent out.

The solution is to hold the back_lock around
iscsi_complete_task() calls, and to add a little commenting
to help others understand when back_lock must be held.
---
 drivers/scsi/libiscsi.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b8d325ce8754..ef7871e8c6bd 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -838,7 +838,7 @@ EXPORT_SYMBOL_GPL(iscsi_conn_send_pdu);
  * @datalen: len of buffer
  *
  * iscsi_cmd_rsp sets up the scsi_cmnd fields based on the PDU and
- * then completes the command and task.
+ * then completes the command and task. called under back_lock
  **/
 static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
   struct iscsi_task *task, char *data,
@@ -941,6 +941,9 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, 
struct iscsi_hdr *hdr,
  * @conn: iscsi connection
  * @hdr:  iscsi pdu
  * @task: scsi command task
+ *
+ * iscsi_data_in_rsp sets up the scsi_cmnd fields based on the data received
+ * then completes the command and task. called under back_lock
  **/
 static void
 iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
@@ -1025,6 +1028,16 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
struct iscsi_nopin *rhdr)
return 0;
 }
 
+/**
+ * iscsi_nop_out_rsp - SCSI NOP Response processing
+ * @task: scsi command task
+ * @nop: the nop structure
+ * @data: where to put the data
+ * @datalen: length of data
+ *
+ * iscsi_nop_out_rsp handles nop response from use or
+ * from user space. called under back_lock
+ **/
 static int iscsi_nop_out_rsp(struct iscsi_task *task,
 struct iscsi_nopin *nop, char *data, int datalen)
 {
@@ -1791,7 +1804,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)
return 0;
 
 prepd_reject:
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
+   spin_unlock_bh(&session->back_lock);
 reject:
spin_unlock_bh(&session->frwd_lock);
ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
@@ -1799,7 +1814,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)
return SCSI_MLQUEUE_TARGET_BUSY;
 
 prepd_fault:
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
+   spin_unlock_bh(&session->back_lock);
 fault:
spin_unlock_bh(&session->frwd_lock);
ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
@@ -3121,8 +3138,9 @@ fail_mgmt_tasks(struct iscsi_session *session, struct 
iscsi_conn *conn)
state = ISCSI_TASK_ABRT_SESS_RECOV;
if (task->state == ISCSI_TASK_PENDING)
state = ISCSI_TASK_COMPLETED;
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, state);
-
+   spin_unlock_bh(&session->back_lock);
}
 }
 
-- 
2.16.4

[PATCH] Hold back_lock when calling iscsi_complete_task

[Lee Duncan]

From: Lee Duncan 

If there is an error queueing an iscsi command in
iscsi_queuecommand(), for example if the transport fails
to take the command in sessuin->tt->xmit_task(), then
the error path can call iscsi_complete_task() without
first aquiring the back_lock as required. This can
lead to things like ITT pool can get corrupt, resulting
in duplicate ITTs being sent out.

The solution is to hold the back_lock around
iscsi_complete_task() calls, and to add a little commenting
to help others understand when back_lock must be held.
---
 drivers/scsi/libiscsi.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b8d325ce8754..ef7871e8c6bd 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -838,7 +838,7 @@ EXPORT_SYMBOL_GPL(iscsi_conn_send_pdu);
  * @datalen: len of buffer
  *
  * iscsi_cmd_rsp sets up the scsi_cmnd fields based on the PDU and
- * then completes the command and task.
+ * then completes the command and task. called under back_lock
  **/
 static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
   struct iscsi_task *task, char *data,
@@ -941,6 +941,9 @@ static void iscsi_scsi_cmd_rsp(struct iscsi_conn *conn, 
struct iscsi_hdr *hdr,
  * @conn: iscsi connection
  * @hdr:  iscsi pdu
  * @task: scsi command task
+ *
+ * iscsi_data_in_rsp sets up the scsi_cmnd fields based on the data received
+ * then completes the command and task. called under back_lock
  **/
 static void
 iscsi_data_in_rsp(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
@@ -1025,6 +1028,16 @@ static int iscsi_send_nopout(struct iscsi_conn *conn, 
struct iscsi_nopin *rhdr)
return 0;
 }
 
+/**
+ * iscsi_nop_out_rsp - SCSI NOP Response processing
+ * @task: scsi command task
+ * @nop: the nop structure
+ * @data: where to put the data
+ * @datalen: length of data
+ *
+ * iscsi_nop_out_rsp handles nop response from use or
+ * from user space. called under back_lock
+ **/
 static int iscsi_nop_out_rsp(struct iscsi_task *task,
 struct iscsi_nopin *nop, char *data, int datalen)
 {
@@ -1791,7 +1804,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)
return 0;
 
 prepd_reject:
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
+   spin_unlock_bh(&session->back_lock);
 reject:
spin_unlock_bh(&session->frwd_lock);
ISCSI_DBG_SESSION(session, "cmd 0x%x rejected (%d)\n",
@@ -1799,7 +1814,9 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
scsi_cmnd *sc)
return SCSI_MLQUEUE_TARGET_BUSY;
 
 prepd_fault:
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, ISCSI_TASK_REQUEUE_SCSIQ);
+   spin_unlock_bh(&session->back_lock);
 fault:
spin_unlock_bh(&session->frwd_lock);
ISCSI_DBG_SESSION(session, "iscsi: cmd 0x%x is not queued (%d)\n",
@@ -3121,8 +3138,9 @@ fail_mgmt_tasks(struct iscsi_session *session, struct 
iscsi_conn *conn)
state = ISCSI_TASK_ABRT_SESS_RECOV;
if (task->state == ISCSI_TASK_PENDING)
state = ISCSI_TASK_COMPLETED;
+   spin_lock_bh(&session->back_lock);
iscsi_complete_task(task, state);
-
+   spin_unlock_bh(&session->back_lock);
}
 }
 
-- 
2.16.4

Re: [PATCH] scsi: iscsi_tcp: set BDI_CAP_STABLE_WRITES when data digest enabled

[Lee Duncan]

I reviewed this several days before but mistakenly replied only to the 
open-iscsi list. 

Signed-off-by: Lee Duncan 

--
Lee-Man Duncan
Sent from my iPhone, dude


> On Mar 14, 2018, at 10:11 PM, Martin K. Petersen  
> wrote:
> 
> 
>> iscsi tcp will first send out data, then calculate and send data
>> digest. If we don't have BDI_CAP_STABLE_WRITES, the page cache will
>> be written in spite of the on going writeback. Consequently, wrong
>> digest will be got and sent to target.
>> 
>> To fix this, set BDI_CAP_STABLE_WRITES when data digest is enabled
>> in iscsi_tcp .slave_configure callback.
> 
> Lee, Chris: Please review!
> 
> -- 
> Martin K. PetersenOracle Linux Engineering
> 
> -- 
> You received this message because you are subscribed to a topic in the Google 
> Groups "open-iscsi" group.
> To unsubscribe from this topic, visit 
> https://groups.google.com/d/topic/open-iscsi/owLIZAXfgoA/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to 
> open-iscsi+unsubscr...@googlegroups.com.
> To post to this group, send email to open-is...@googlegroups.com.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] scsi: libiscsi: fix shifting of DID_REQUEUE host byte

[Lee Duncan]

On 10/09/2017 04:33 AM, Johannes Thumshirn wrote:
> The SCSI host byte should be shifted left by 16 in order to have
> scsi_decide_disposition() do the right thing (.i.e. requeue the command).
> 
> Signed-off-by: Johannes Thumshirn 
> Fixes: 661134ad3765 ("[SCSI] libiscsi, bnx2i: make bound ep check common")
> Cc: Lee Duncan 
> Cc: Hannes Reinecke 
> Cc: Bart Van Assche 
> Cc: Chris Leech 
> ---
>  drivers/scsi/libiscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index bd4605a34f54..9cba4913b43c 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1728,7 +1728,7 @@ int iscsi_queuecommand(struct Scsi_Host *host, struct 
> scsi_cmnd *sc)
>  
>   if (test_bit(ISCSI_SUSPEND_BIT, &conn->suspend_tx)) {
>   reason = FAILURE_SESSION_IN_RECOVERY;
> - sc->result = DID_REQUEUE;
> + sc->result = DID_REQUEUE << 16;
>   goto fault;
>   }
>  
> 
Good catch.

I know you're working on fixing all drivers to use the correct macros
rather than rolling there own.

Acked-by: Lee Duncan 
-- 
Lee Duncan
SUSE Labs

Re: [PATCH 2/2] uapi: add a compatibility layer between linux/uio.h and glibc

[Lee Duncan]

Ping? I never saw any response to this.

On Wed, 22 Feb 2017 05:29:47 +0300, Dmitry V Levin wrote:
> Do not define struct iovec in linux/uio.h when  or 
> is already included and provides these definitions.
> 
> This fixes the following compilation error when  or 
> is included before :
> 
> /usr/include/linux/uio.h:16:8: error: redefinition of 'struct iovec'
> 
> Signed-off-by: Dmitry V. Levin 
> ---
>  include/uapi/linux/libc-compat.h | 10 ++
>  include/uapi/linux/uio.h |  3 +++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/include/uapi/linux/libc-compat.h 
> b/include/uapi/linux/libc-compat.h
> index 481e3b1..9b88586 100644
> --- a/include/uapi/linux/libc-compat.h
> +++ b/include/uapi/linux/libc-compat.h
> @@ -205,6 +205,13 @@
>  #define __UAPI_DEF_TIMEZONE  1
>  #endif
>  
> +/* Coordinate with glibc bits/uio.h header. */
> +#if defined(_SYS_UIO_H) || defined(_FCNTL_H)
> +#define __UAPI_DEF_IOVEC 0
> +#else
> +#define __UAPI_DEF_IOVEC 1
> +#endif
> +
>  /* Definitions for xattr.h */
>  #if defined(_SYS_XATTR_H)
>  #define __UAPI_DEF_XATTR 0
> @@ -261,6 +268,9 @@
>  #define __UAPI_DEF_TIMEVAL   1
>  #define __UAPI_DEF_TIMEZONE  1
>  
> +/* Definitions for uio.h */
> +#define __UAPI_DEF_IOVEC 1
> +
>  /* Definitions for xattr.h */
>  #define __UAPI_DEF_XATTR 1
>  
> diff --git a/include/uapi/linux/uio.h b/include/uapi/linux/uio.h
> index 2731d56..e6e12cf 100644
> --- a/include/uapi/linux/uio.h
> +++ b/include/uapi/linux/uio.h
> @@ -9,15 +9,18 @@
>  #ifndef _UAPI__LINUX_UIO_H
>  #define _UAPI__LINUX_UIO_H
>  
> +#include 
>  #include 
>  #include 
>  
>  
> +#if __UAPI_DEF_IOVEC
>  struct iovec
>  {
>   void __user *iov_base;  /* BSD uses caddr_t (1003.1g requires void *) */
>   __kernel_size_t iov_len; /* Must be size_t (1003.1g) */
>  };
> +#endif /* __UAPI_DEF_IOVEC */
>  
>  /*
>   *   UIO_MAXIOV shall be at least 16 1003.1g (5.4.1.1)
> -- 
> ldv

-- 
Lee Duncan
SUSE Labs

Re: [Patch v2 2/2] libiscsi: Remove iscsi_destroy_session

[Lee Duncan]

On 07/13/2017 09:11 AM, Khazhismel Kumykov wrote:
> iscsi_session_teardown was the only user of this function. Function
> currently is just short for iscsi_remove_session + iscsi_free_session.
> 
> Signed-off-by: Khazhismel Kumykov 

Why is this needed? I dislike changes that don't fix anything.

> ---
>  drivers/scsi/scsi_transport_iscsi.c | 16 
>  include/scsi/scsi_transport_iscsi.h |  1 -
>  2 files changed, 17 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c 
> b/drivers/scsi/scsi_transport_iscsi.c
> index a424eaeafeb0..924ac408d8a9 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2210,22 +2210,6 @@ void iscsi_free_session(struct iscsi_cls_session 
> *session)
>  }
>  EXPORT_SYMBOL_GPL(iscsi_free_session);
>  
> -/**
> - * iscsi_destroy_session - destroy iscsi session
> - * @session: iscsi_session
> - *
> - * Can be called by a LLD or iscsi_transport. There must not be
> - * any running connections.
> - */
> -int iscsi_destroy_session(struct iscsi_cls_session *session)
> -{
> - iscsi_remove_session(session);
> - ISCSI_DBG_TRANS_SESSION(session, "Completing session destruction\n");
> - iscsi_free_session(session);
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(iscsi_destroy_session);
> -
>  /**
>   * iscsi_create_conn - create iscsi class connection
>   * @session: iscsi cls session
> diff --git a/include/scsi/scsi_transport_iscsi.h 
> b/include/scsi/scsi_transport_iscsi.h
> index 6183d20a01fb..b266d2a3bcb1 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -434,7 +434,6 @@ extern struct iscsi_cls_session 
> *iscsi_create_session(struct Scsi_Host *shost,
>   unsigned int target_id);
>  extern void iscsi_remove_session(struct iscsi_cls_session *session);
>  extern void iscsi_free_session(struct iscsi_cls_session *session);
> -extern int iscsi_destroy_session(struct iscsi_cls_session *session);
>  extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session 
> *sess,
>   int dd_size, uint32_t cid);
>  extern int iscsi_destroy_conn(struct iscsi_cls_conn *conn);
> 

-- 
Lee Duncan
SUSE Labs

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Lee Duncan]

On 06/16/2017 05:41 PM, Jason A. Donenfeld wrote:
> Hi Lee,
> 
> On Fri, Jun 16, 2017 at 11:58 PM, Lee Duncan  wrote:
>> It seems like what you are doing is basically "good", i.e. if there is
>> not enough random data, don't use it. But what happens in that case? The
>> authentication fails? How does the user know to wait and try again?
> 
> The process just remains in interruptible (kill-able) sleep until
> there is enough entropy, so the process doesn't need to do anything.
> If the waiting is interrupted by a signal, it returns -ESYSRESTART,
> which follows the usual semantics of restartable syscalls.
> 
> Jason
> 

In your testing, how long might a process have to wait? Are we talking
seconds? Longer? What about timeouts?

Sorry, but your changing something that isn't exactly broken, so I just
want to be sure we're not introducing some regression, like clients
can't connect the first 5 minutes are a reboot.
-- 
Lee Duncan

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Lee Duncan]

On 06/08/2017 05:09 AM, Jason A. Donenfeld wrote:
> On Thu, Jun 8, 2017 at 4:43 AM, Theodore Ts'o  wrote:
>> What was the testing that was done for commit?  It looks safe, but I'm
>> unfamiliar enough with how the iSCSI authentication works that I'd
>> prefer getting an ack'ed by from the iSCSI maintainers or
>> alternativel, information about how to kick off some kind of automated
>> test suite ala xfstests for file systems.
> 
> Only very basic testing from my end.
> 
> I'm thus adding the iSCSI list to see if they'll have a look (patch 
> reattached).
> 
> Jason
> 

It seems like what you are doing is basically "good", i.e. if there is
not enough random data, don't use it. But what happens in that case? The
authentication fails? How does the user know to wait and try again?
-- 
Lee Duncan
SUSE Labs

Re: [PATCH 5/8] linux: drop __bitwise__ everywhere

[Lee Duncan]

ypedef u64 dma_addr_t;
>  typedef u32 dma_addr_t;
>  #endif
>  
> -typedef unsigned __bitwise__ gfp_t;
> -typedef unsigned __bitwise__ fmode_t;
> +typedef unsigned __bitwise gfp_t;
> +typedef unsigned __bitwise fmode_t;
>  
>  #ifdef CONFIG_PHYS_ADDR_T_64BIT
>  typedef u64 phys_addr_t;
> diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h
> index c1260d8..df156f1 100644
> --- a/include/scsi/iscsi_proto.h
> +++ b/include/scsi/iscsi_proto.h
> @@ -74,7 +74,7 @@ static inline int iscsi_sna_gte(u32 n1, u32 n2)
>  #define zero_data(p) {p[0]=0;p[1]=0;p[2]=0;}
>  
>  /* initiator tags; opaque for target */
> -typedef uint32_t __bitwise__ itt_t;
> +typedef uint32_t __bitwise itt_t;
>  /* below makes sense only for initiator that created this tag */
>  #define build_itt(itt, age) ((__force itt_t)\
>   ((itt) | ((age) << ISCSI_AGE_SHIFT)))
> diff --git a/include/target/target_core_base.h 
> b/include/target/target_core_base.h
> index c211900..0055828 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -149,7 +149,7 @@ enum se_cmd_flags_table {
>   * Used by transport_send_check_condition_and_sense()
>   * to signal which ASC/ASCQ sense payload should be built.
>   */
> -typedef unsigned __bitwise__ sense_reason_t;
> +typedef unsigned __bitwise sense_reason_t;
>  
>  enum tcm_sense_reason_table {
>  #define R(x) (__force sense_reason_t )(x)
> diff --git a/include/uapi/linux/virtio_types.h 
> b/include/uapi/linux/virtio_types.h
> index e845e8c..55c3b73 100644
> --- a/include/uapi/linux/virtio_types.h
> +++ b/include/uapi/linux/virtio_types.h
> @@ -39,8 +39,8 @@
>   * - __le{16,32,64} for standard-compliant virtio devices
>   */
>  
> -typedef __u16 __bitwise__ __virtio16;
> -typedef __u32 __bitwise__ __virtio32;
> -typedef __u64 __bitwise__ __virtio64;
> +typedef __u16 __bitwise __virtio16;
> +typedef __u32 __bitwise __virtio32;
> +typedef __u64 __bitwise __virtio64;
>  
>  #endif /* _UAPI_LINUX_VIRTIO_TYPES_H */
> diff --git a/net/ieee802154/6lowpan/6lowpan_i.h 
> b/net/ieee802154/6lowpan/6lowpan_i.h
> index 5ac7789..ac7c96b 100644
> --- a/net/ieee802154/6lowpan/6lowpan_i.h
> +++ b/net/ieee802154/6lowpan/6lowpan_i.h
> @@ -7,7 +7,7 @@
>  #include 
>  #include 
>  
> -typedef unsigned __bitwise__ lowpan_rx_result;
> +typedef unsigned __bitwise lowpan_rx_result;
>  #define RX_CONTINUE  ((__force lowpan_rx_result) 0u)
>  #define RX_DROP_UNUSABLE ((__force lowpan_rx_result) 1u)
>  #define RX_DROP  ((__force lowpan_rx_result) 2u)
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index d37a577..b2069fb 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -159,7 +159,7 @@ enum ieee80211_bss_valid_data_flags {
>   IEEE80211_BSS_VALID_ERP = BIT(3)
>  };
>  
> -typedef unsigned __bitwise__ ieee80211_tx_result;
> +typedef unsigned __bitwise ieee80211_tx_result;
>  #define TX_CONTINUE  ((__force ieee80211_tx_result) 0u)
>  #define TX_DROP  ((__force ieee80211_tx_result) 1u)
>  #define TX_QUEUED((__force ieee80211_tx_result) 2u)
> @@ -180,7 +180,7 @@ struct ieee80211_tx_data {
>  };
>  
>  
> -typedef unsigned __bitwise__ ieee80211_rx_result;
> +typedef unsigned __bitwise ieee80211_rx_result;
>  #define RX_CONTINUE  ((__force ieee80211_rx_result) 0u)
>  #define RX_DROP_UNUSABLE ((__force ieee80211_rx_result) 1u)
>  #define RX_DROP_MONITOR  ((__force ieee80211_rx_result) 2u)
>

For iscsi initiator, looks good.

Akced-by: Lee Duncan 

-- 
Lee Duncan

Re: [PATCHv2] MAINTAINERS: Update open-iscsi maintainers

[Lee Duncan]

Hello Martin:

On 09/26/2016 06:26 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan  writes:
> 
> Lee,
> 
> Lee> Chris Leech and I are taking over as open-iscsi maintainers.
> 
> Do you want me to queue the MAINTAINER update?

Yes, that would be great. Thank you.

> 
> Lee>  * Removed git repository, since code in tree
> 
> Is it your plan to go through the SCSI tree?
> 

Yes, the iscsi initiator kernel code updates have been going through the
Linux SCSI mailing list and repository for a while, now. The open-iscsi
kernel code git repository currently mentioned in the MAINTAINERS file
no longer exists.
-- 
Lee Duncan

[PATCHv2] MAINTAINERS: Update open-iscsi maintainers

[Lee Duncan]

Chris Leech and I are taking over as open-iscsi maintainers.

Changes since v1:
 * Updated URL to open-iscsi.com
 * Removed git repository, since code in tree
---
 MAINTAINERS | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 01bff8ea28d8..81384a2562e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6448,10 +6448,10 @@ S:  Maintained
 F: drivers/firmware/iscsi_ibft*
 
 ISCSI
-M: Mike Christie 
+M: Lee Duncan 
+M: Chris Leech 
 L: open-is...@googlegroups.com
-W: www.open-iscsi.org
-T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
+W: www.open-iscsi.com
 S: Maintained
 F: drivers/scsi/*iscsi*
 F: include/scsi/*iscsi*
-- 
2.1.4

Re: [PATCH] MAINTAINERS: Update open-iscsi maintainers

[Lee Duncan]

[Added linux-scsi to the cc list.]

I will resubmit an updated version of this patch.

On 09/23/2016 02:34 PM, Lee Duncan wrote:
> Chris Leech and I are taking over open-iscsi
> maintenance from Mike Christie.
> 
> Signed-off-by: Lee Duncan 
> ---
>  MAINTAINERS | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 01bff8ea28d8..0afaf42d5416 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6448,7 +6448,8 @@ S:  Maintained
>  F:   drivers/firmware/iscsi_ibft*
>  
>  ISCSI
> -M:   Mike Christie 
> +M:   Lee Duncan 
> +M:   Chris Leech 
>  L:   open-is...@googlegroups.com
>  W:   www.open-iscsi.org
>  T:   git 
> git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
> 

-- 
Lee Duncan

[PATCH] MAINTAINERS: Update open-iscsi maintainers

[Lee Duncan]

Chris Leech and I are taking over open-iscsi
maintenance from Mike Christie.

Signed-off-by: Lee Duncan 
---
 MAINTAINERS | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 01bff8ea28d8..0afaf42d5416 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6448,7 +6448,8 @@ S:Maintained
 F: drivers/firmware/iscsi_ibft*
 
 ISCSI
-M: Mike Christie 
+M: Lee Duncan 
+M: Chris Leech 
 L: open-is...@googlegroups.com
 W: www.open-iscsi.org
 T: git 
git://git.kernel.org/pub/scm/linux/kernel/git/mnc/linux-2.6-iscsi.git
-- 
2.1.4

Re: [PATCHv3 0/2] target: make location of /var/targets configurable

[Lee Duncan]

Ping?

We really need to move the target database out of /var/target

On 04/14/2016 06:18 PM, Lee Duncan wrote:
> These patches make the location of "/var/target" configurable,
> though it still defauls to "/var/target".
> 
> This "target database directory" can only be changed
> after the target_core_mod loads but before any
> fabric drivers are loaded, and must be the pathname
> of an existing directory.
> 
> This configuration is accomplished via the configfs
> top-level target attribute "dbroot", i.e. dumping
> out "/sys/kernel/config/target/dbroot" will normally
> return "/var/target". Writing to this attribute
> changes the loation where the kernel looks for the
> target database.
> 
> The first patch creates this configurable value for
> the "dbroot", and the second patch modifies users
> of this directory to use this new attribute.
> 
> Changes from v2:
>  * Add locking around access to target driver list
> 
> Changes from v1:
>  * Only allow changing target DB root before it
>can be used by others
>  * Validate that new DB root is a valid directory
> 
> Lee Duncan (2):
>   target: make target db location configurable
>   target: use new "dbroot" target attribute
> 
>  drivers/target/target_core_alua.c |  6 ++--
>  drivers/target/target_core_configfs.c | 62 
> +++++++++++
>  drivers/target/target_core_internal.h |  6 
>  drivers/target/target_core_pr.c   |  2 +-
>  4 files changed, 72 insertions(+), 4 deletions(-)
> 

-- 
Lee Duncan
SUSE Labs

Re: [PATCHv3 0/2] target: make location of /var/targets configurable

[Lee Duncan]

On 04/14/2016 06:18 PM, Lee Duncan wrote:
> These patches make the location of "/var/target" configurable,
> though it still defauls to "/var/target".
> 
> This "target database directory" can only be changed
> after the target_core_mod loads but before any
> fabric drivers are loaded, and must be the pathname
> of an existing directory.
> 
> This configuration is accomplished via the configfs
> top-level target attribute "dbroot", i.e. dumping
> out "/sys/kernel/config/target/dbroot" will normally
> return "/var/target". Writing to this attribute
> changes the loation where the kernel looks for the
> target database.
> 
> The first patch creates this configurable value for
> the "dbroot", and the second patch modifies users
> of this directory to use this new attribute.
> 
> Changes from v2:
>  * Add locking around access to target driver list
> 
> Changes from v1:
>  * Only allow changing target DB root before it
>can be used by others
>  * Validate that new DB root is a valid directory
> 
> Lee Duncan (2):
>   target: make target db location configurable
>   target: use new "dbroot" target attribute
> 
>  drivers/target/target_core_alua.c |  6 ++--
>  drivers/target/target_core_configfs.c | 62 
> +++++++
>  drivers/target/target_core_internal.h |  6 
>  drivers/target/target_core_pr.c   |  2 +-
>  4 files changed, 72 insertions(+), 4 deletions(-)
> 

Ping?
-- 
Lee Duncan

[PATCHv3 0/2] target: make location of /var/targets configurable

[Lee Duncan]

These patches make the location of "/var/target" configurable,
though it still defauls to "/var/target".

This "target database directory" can only be changed
after the target_core_mod loads but before any
fabric drivers are loaded, and must be the pathname
of an existing directory.

This configuration is accomplished via the configfs
top-level target attribute "dbroot", i.e. dumping
out "/sys/kernel/config/target/dbroot" will normally
return "/var/target". Writing to this attribute
changes the loation where the kernel looks for the
target database.

The first patch creates this configurable value for
the "dbroot", and the second patch modifies users
of this directory to use this new attribute.

Changes from v2:
 * Add locking around access to target driver list

Changes from v1:
 * Only allow changing target DB root before it
   can be used by others
 * Validate that new DB root is a valid directory

Lee Duncan (2):
  target: make target db location configurable
  target: use new "dbroot" target attribute

 drivers/target/target_core_alua.c |  6 ++--
 drivers/target/target_core_configfs.c | 62 +++
 drivers/target/target_core_internal.h |  6 
 drivers/target/target_core_pr.c   |  2 +-
 4 files changed, 72 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCHv3 1/2] target: make target db location configurable

[Lee Duncan]

This commit adds the read-write attribute "dbroot",
in the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it. Changing this
attribute is only allowed when no fabric drivers
are loaded and the supplied value specifies an
existing directory.

Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.

Signed-off-by: Lee Duncan 
---
 drivers/target/target_core_configfs.c | 62 +++
 drivers/target/target_core_internal.h |  6 
 2 files changed, 68 insertions(+)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 713c63d9681b..8cce79317971 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,67 @@ static ssize_t target_core_item_version_show(struct 
config_item *item,
 
 CONFIGFS_ATTR_RO(target_core_item_, version);
 
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+static char db_root_stage[DB_ROOT_LEN];
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   ssize_t read_bytes;
+   struct file *fp;
+
+   mutex_lock(&g_tf_lock);
+   if (!list_empty(&g_tf_list)) {
+   mutex_unlock(&g_tf_lock);
+   pr_err("db_root: cannot be changed: target drivers registered");
+   return -EINVAL;
+   }
+
+   if (count > (DB_ROOT_LEN - 1)) {
+   mutex_unlock(&g_tf_lock);
+   pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
+  (int)count, DB_ROOT_LEN - 1);
+   return -EINVAL;
+   }
+
+   read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
+   if (!read_bytes) {
+   mutex_unlock(&g_tf_lock);
+   return -EINVAL;
+   }
+   if (db_root_stage[read_bytes - 1] == '\n')
+   db_root_stage[read_bytes - 1] = '\0';
+
+   /* validate new db root before accepting it */
+   fp = filp_open(db_root_stage, O_RDONLY, 0);
+   if (IS_ERR(fp)) {
+   mutex_unlock(&g_tf_lock);
+   pr_err("db_root: cannot open: %s\n", db_root_stage);
+   return -EINVAL;
+   }
+   if (!S_ISDIR(fp->f_inode->i_mode)) {
+   filp_close(fp, 0);
+   mutex_unlock(&g_tf_lock);
+   pr_err("db_root: not a directory: %s\n", db_root_stage);
+   return -EINVAL;
+   }
+   filp_close(fp, 0);
+
+   strncpy(db_root, db_root_stage, read_bytes);
+
+   mutex_unlock(&g_tf_lock);
+
+   return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
 static struct target_fabric_configfs *target_core_get_fabric(
const char *name)
 {
@@ -249,6 +310,7 @@ static struct configfs_group_operations 
target_core_fabric_group_ops = {
  */
 static struct configfs_attribute *target_core_fabric_item_attrs[] = {
&target_core_item_attr_version,
+   &target_core_item_attr_dbroot,
NULL,
 };
 
diff --git a/drivers/target/target_core_internal.h 
b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void   
target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
 /* target_core_xcopy.c */
 extern struct se_portal_group xcopy_pt_tpg;
 
+/* target_core_configfs.c */
+#define DB_ROOT_LEN4096
+#defineDB_ROOT_DEFAULT "/var/target"
+
+extern char db_root[];
+
 #endif /* TARGET_CORE_INTERNAL_H */
-- 
2.1.4

[PATCHv3 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".

Signed-off-by: Lee Duncan 
Reviewed-by: Hannes Reinecke 
---
 drivers/target/target_core_alua.c | 6 +++---
 drivers/target/target_core_pr.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c 
b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
tg_pt_gp->tg_pt_gp_alua_access_status);
 
snprintf(path, ALUA_METADATA_PATH_LEN,
-   "/var/target/alua/tpgs_%s/%s", &wwn->unit_serial[0],
+   "%s/alua/tpgs_%s/%s", db_root, &wwn->unit_serial[0],
config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 
rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct 
se_lun *lun)
atomic_read(&lun->lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
 
-   snprintf(path, ALUA_METADATA_PATH_LEN, 
"/var/target/alua/%s/%s/lun_%llu",
-   se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+   snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+   db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
lun->unpacked_lun);
 
rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
return -EMSGSIZE;
}
 
-   snprintf(path, 512, "/var/target/pr/aptpl_%s", &wwn->unit_serial[0]);
+   snprintf(path, 512, "%s/pr/aptpl_%s", db_root, &wwn->unit_serial[0]);
file = filp_open(path, flags, 0600);
if (IS_ERR(file)) {
pr_err("filp_open(%s) for APTPL metadata"
-- 
2.1.4

Re: [PATCHv2 1/2] target: make target db location configurable

[Lee Duncan]

On 04/13/2016 11:10 PM, Hannes Reinecke wrote:
> On 04/13/2016 10:25 PM, Lee Duncan wrote:
>> This commit adds the read-write attribute "dbroot",
>> in the top-level CONFIGFS (core) target directory,
>> normally /sys/kernel/config/target. This attribute
>> defaults to "/var/target" but can be changed by
>> writing a new pathname string to it. Changing this
>> attribute is only allowed when no fabric drivers
>> are loaded and the supplied value specifies an
>> existing directory.
>>
>> Target modules that care about the target database
>> root directory will be modified to use this
>> attribute in a future commit.
>>
>> Signed-off-by: Lee Duncan 
>> ---
>>  drivers/target/target_core_configfs.c | 51 
>> +++
>>  drivers/target/target_core_internal.h |  6 +
>>  2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c 
>> b/drivers/target/target_core_configfs.c
>> index 713c63d9681b..bfedbd92b77f 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -99,6 +99,56 @@ static ssize_t target_core_item_version_show(struct 
>> config_item *item,
>>  
>>  CONFIGFS_ATTR_RO(target_core_item_, version);
>>  
>> +char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
>> +static char db_root_stage[DB_ROOT_LEN];
>> +
>> +static ssize_t target_core_item_dbroot_show(struct config_item *item,
>> +char *page)
>> +{
>> +return sprintf(page, "%s\n", db_root);
>> +}
>> +
>> +static ssize_t target_core_item_dbroot_store(struct config_item *item,
>> +const char *page, size_t count)
>> +{
>> +ssize_t read_bytes;
>> +struct file *fp;
>> +
>> +if (!list_empty(&g_tf_list)) {
>> +pr_err("db_root: cannot be changed: target drivers registered");
>> +return -EINVAL;
>> +}
> Locking?

Doh. I will resubmit with locking shortly.

> 
> Cheers,
> 
> Hannes
> 

-- 
Lee Duncan

[PATCHv2 1/2] target: make target db location configurable

[Lee Duncan]

This commit adds the read-write attribute "dbroot",
in the top-level CONFIGFS (core) target directory,
normally /sys/kernel/config/target. This attribute
defaults to "/var/target" but can be changed by
writing a new pathname string to it. Changing this
attribute is only allowed when no fabric drivers
are loaded and the supplied value specifies an
existing directory.

Target modules that care about the target database
root directory will be modified to use this
attribute in a future commit.

Signed-off-by: Lee Duncan 
---
 drivers/target/target_core_configfs.c | 51 +++
 drivers/target/target_core_internal.h |  6 +
 2 files changed, 57 insertions(+)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 713c63d9681b..bfedbd92b77f 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -99,6 +99,56 @@ static ssize_t target_core_item_version_show(struct 
config_item *item,
 
 CONFIGFS_ATTR_RO(target_core_item_, version);
 
+char db_root[DB_ROOT_LEN] = DB_ROOT_DEFAULT;
+static char db_root_stage[DB_ROOT_LEN];
+
+static ssize_t target_core_item_dbroot_show(struct config_item *item,
+   char *page)
+{
+   return sprintf(page, "%s\n", db_root);
+}
+
+static ssize_t target_core_item_dbroot_store(struct config_item *item,
+   const char *page, size_t count)
+{
+   ssize_t read_bytes;
+   struct file *fp;
+
+   if (!list_empty(&g_tf_list)) {
+   pr_err("db_root: cannot be changed: target drivers registered");
+   return -EINVAL;
+   }
+   if (count > (DB_ROOT_LEN - 1)) {
+   pr_err("db_root: count %d exceeds DB_ROOT_LEN-1: %u\n",
+  (int)count, DB_ROOT_LEN - 1);
+   return -EINVAL;
+   }
+
+   read_bytes = snprintf(db_root_stage, DB_ROOT_LEN, "%s", page);
+   if (!read_bytes)
+   return -EINVAL;
+   if (db_root_stage[read_bytes - 1] == '\n')
+   db_root_stage[read_bytes - 1] = '\0';
+
+   /* validate new db root before accepting it */
+   fp = filp_open(db_root_stage, O_RDONLY, 0);
+   if (IS_ERR(fp)) {
+   pr_err("db_root: cannot open: %s\n", db_root_stage);
+   return -EINVAL;
+   }
+   if (!S_ISDIR(fp->f_inode->i_mode)) {
+   pr_err("db_root: not a directory: %s\n", db_root_stage);
+   filp_close(fp, 0);
+   return -EINVAL;
+   }
+   filp_close(fp, 0);
+   strncpy(db_root, db_root_stage, read_bytes);
+
+   return read_bytes;
+}
+
+CONFIGFS_ATTR(target_core_item_, dbroot);
+
 static struct target_fabric_configfs *target_core_get_fabric(
const char *name)
 {
@@ -249,6 +299,7 @@ static struct configfs_group_operations 
target_core_fabric_group_ops = {
  */
 static struct configfs_attribute *target_core_fabric_item_attrs[] = {
&target_core_item_attr_version,
+   &target_core_item_attr_dbroot,
NULL,
 };
 
diff --git a/drivers/target/target_core_internal.h 
b/drivers/target/target_core_internal.h
index 040cf5202e54..c2a18b960c5d 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -156,4 +156,10 @@ void   
target_stat_setup_mappedlun_default_groups(struct se_lun_acl *);
 /* target_core_xcopy.c */
 extern struct se_portal_group xcopy_pt_tpg;
 
+/* target_core_configfs.c */
+#define DB_ROOT_LEN4096
+#defineDB_ROOT_DEFAULT "/var/target"
+
+extern char db_root[];
+
 #endif /* TARGET_CORE_INTERNAL_H */
-- 
2.1.4

[PATCHv2 0/2] target: make location of /var/targets configurable

[Lee Duncan]

These patches make the location of "/var/target" configurable,
though it still defauls to "/var/target".

This "target database directory" can only be changed
after the target_core_mod loads but before any
fabric drivers are loaded, and must be the pathname
of an existing directory.

This configuration is accomplished via the configfs
top-level target attribute "dbroot", i.e. dumping
out "/sys/kernel/config/target/dbroot" will normally
return "/var/target". Writing to this attribute
changes the loation where the kernel looks for the
target database.

The first patch creates this configurable value for
the "dbroot", and the second patch modifies users
of this directory to use this new attribute.

Changes from v1:
 * Only allow changing target DB root before it
   can be used by others
 * Validate that new DB root is a valid directory

Lee Duncan (2):
  target: make target db location configurable
  target: use new "dbroot" target attribute

 drivers/target/target_core_alua.c |  6 ++---
 drivers/target/target_core_configfs.c | 51 +++
 drivers/target/target_core_internal.h |  6 +
 drivers/target/target_core_pr.c   |  2 +-
 4 files changed, 61 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH 2/2] target: use new "dbroot" target attribute

[Lee Duncan]

This commit updates the target core ALUA and PR
modules to use the new "dbroot" attribute instead
of assuming the target database is in "/var/target".

Signed-off-by: Lee Duncan 
---
 drivers/target/target_core_alua.c | 6 +++---
 drivers/target/target_core_pr.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c 
b/drivers/target/target_core_alua.c
index 49aba4a31747..4c82bbe19003 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -932,7 +932,7 @@ static int core_alua_update_tpg_primary_metadata(
tg_pt_gp->tg_pt_gp_alua_access_status);
 
snprintf(path, ALUA_METADATA_PATH_LEN,
-   "/var/target/alua/tpgs_%s/%s", &wwn->unit_serial[0],
+   "%s/alua/tpgs_%s/%s", db_root, &wwn->unit_serial[0],
config_item_name(&tg_pt_gp->tg_pt_gp_group.cg_item));
 
rc = core_alua_write_tpg_metadata(path, md_buf, len);
@@ -1275,8 +1275,8 @@ static int core_alua_update_tpg_secondary_metadata(struct 
se_lun *lun)
atomic_read(&lun->lun_tg_pt_secondary_offline),
lun->lun_tg_pt_secondary_stat);
 
-   snprintf(path, ALUA_METADATA_PATH_LEN, 
"/var/target/alua/%s/%s/lun_%llu",
-   se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
+   snprintf(path, ALUA_METADATA_PATH_LEN, "%s/alua/%s/%s/lun_%llu",
+   db_root, se_tpg->se_tpg_tfo->get_fabric_name(), wwn,
lun->unpacked_lun);
 
rc = core_alua_write_tpg_metadata(path, md_buf, len);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index b1795735eafc..47463c99c318 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1985,7 +1985,7 @@ static int __core_scsi3_write_aptpl_to_file(
return -EMSGSIZE;
}
 
-   snprintf(path, 512, "/var/target/pr/aptpl_%s", &wwn->unit_serial[0]);
+   snprintf(path, 512, "%s/pr/aptpl_%s", db_root, &wwn->unit_serial[0]);
file = filp_open(path, flags, 0600);
if (IS_ERR(file)) {
pr_err("filp_open(%s) for APTPL metadata"
-- 
2.1.4

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Lee Duncan]

On 02/12/2016 09:54 AM, James Bottomley wrote:
> On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
>> The scsi_transport_iscsi module already uses the ida_simple
>> routines for managing the target ID, if requested to do
>> so. This change replaces an ever-increasing atomic integer
>> that tracks the session ID itself with the ida_simple
>> family of routines. This means that the session ID
>> will be reclaimed and can be reused when the session
>> is freed.
> 
> Is reusing session ID's really a good idea?  For sequential sessions it
> means that the ID of the next session will be re-used, i.e. the same as
> the previous sessions, which could lead to target confusion.  I think
> local uniqueness of session IDs is more important than wrap around
> because sessions are short lived entities and the chances of the same
> session being alive by the time we've wrapped is pretty tiny.
> 
> If you can demostrate a multi-target problem, perhaps we should rather
> fix this by making the next session id a target local quantity?
> 
> James
> 

It looks like Mike and Chris are good with it. And I'd really like to
get rid of yet another atomic int.

Are you satisfied with this one?
-- 
Lee

[PATCH] Use ida_simple for SCSI iSCSI transport session id

[Lee Duncan]

The scsi_transport_iscsi module already uses the ida_simple
routines for managing the target ID, if requested to do
so. This change replaces an ever-increasing atomic integer
that tracks the session ID itself with the ida_simple
family of routines. This means that the session ID
will be reclaimed and can be reused when the session
is freed.

Note that no maximum is placed on this value, though
user-space currently only seems to use the lower 24-bits.
It seems better to handle this in user space, though,
than to limit the value range for the session ID here.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/scsi_transport_iscsi.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c 
b/drivers/scsi/scsi_transport_iscsi.c
index 441481623fb9..50a10bf214a8 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -79,7 +79,8 @@ struct iscsi_internal {
struct transport_container session_cont;
 };
 
-static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
+static DEFINE_IDA(iscsi_session_id_ida);
+
 static struct workqueue_struct *iscsi_eh_timer_workq;
 
 static DEFINE_IDA(iscsi_sess_ida);
@@ -2074,7 +2075,12 @@ int iscsi_add_session(struct iscsi_cls_session *session, 
unsigned int target_id)
int err;
 
ihost = shost->shost_data;
-   session->sid = atomic_add_return(1, &iscsi_session_nr);
+   session->sid = ida_simple_get(&iscsi_session_id_ida, 0, 0, GFP_KERNEL);
+   if (session->sid < 0) {
+   iscsi_cls_session_printk(KERN_ERR, session,
+"Failure in Session ID Allocation\n");
+   return session->sid;
+   }
 
if (target_id == ISCSI_MAX_TARGET) {
id = ida_simple_get(&iscsi_sess_ida, 0, 0, GFP_KERNEL);
@@ -2082,7 +2088,8 @@ int iscsi_add_session(struct iscsi_cls_session *session, 
unsigned int target_id)
if (id < 0) {
iscsi_cls_session_printk(KERN_ERR, session,
"Failure in Target ID Allocation\n");
-   return id;
+   err = id;
+   goto release_session_id_ida;
}
session->target_id = (unsigned int)id;
session->ida_used = true;
@@ -2109,6 +2116,8 @@ int iscsi_add_session(struct iscsi_cls_session *session, 
unsigned int target_id)
 release_ida:
if (session->ida_used)
ida_simple_remove(&iscsi_sess_ida, session->target_id);
+release_session_id_ida:
+   ida_simple_remove(&iscsi_session_id_ida, session->target_id);
 
return err;
 }
@@ -2214,6 +2223,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
 {
ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
iscsi_session_event(session, ISCSI_KEVENT_DESTROY_SESSION);
+   ida_simple_remove(&iscsi_session_id_ida, session->target_id);
put_device(&session->dev);
 }
 EXPORT_SYMBOL_GPL(iscsi_free_session);
@@ -4524,8 +4534,6 @@ static __init int iscsi_transport_init(void)
printk(KERN_INFO "Loading iSCSI transport class v%s.\n",
ISCSI_TRANSPORT_VERSION);
 
-   atomic_set(&iscsi_session_nr, 0);
-
err = class_register(&iscsi_transport_class);
if (err)
return err;
@@ -4598,6 +4606,7 @@ static void __exit iscsi_transport_exit(void)
class_unregister(&iscsi_endpoint_class);
class_unregister(&iscsi_iface_class);
class_unregister(&iscsi_transport_class);
+   ida_destroy(&iscsi_session_id_ida);
 }
 
 module_init(iscsi_transport_init);
-- 
2.1.4

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 01/05/2016 03:53 PM, Martin K. Petersen wrote:

"Lee" == Lee Duncan  writes:


Lee> Do you need me to resubmit this patch now that it's accepted?

Please resend.

Thanks!



Done, submitted against scsi tree, misc branch.
--
Lee Duncan

[PATCHv2] SCSI: usd ida for host number management

[Lee Duncan]

Update the SCSI hosts module to use ida to manage its
host_no index instead of an ATOMIC integer. This means
that the SCSI host number will now be reclaimable.

Use the ida "simple" mechanism, since there should be no
need for a separate spin lock current usage. Ida was chosen
over idr because the hosts module already has its
own instance and locking mechanisms that aren't easily
changed.

Changes from v1:
* First version used regular ida routines

Reviewed-by: Hannes Reinecke 
Reviewed-by: Johannes Thumshirn 
Signed-off-by: Lee Duncan 
---
 drivers/scsi/hosts.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 82ac1cd818ac..94025c5cf797 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include 
 #include 
 #include 
@@ -42,7 +42,7 @@
 #include "scsi_logging.h"
 
 
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new 
host */
+static DEFINE_IDA(host_index_ida);
 
 
 static void scsi_host_cls_release(struct device *dev)
@@ -355,6 +355,8 @@ static void scsi_host_dev_release(struct device *dev)
 
kfree(shost->shost_data);
 
+   ida_simple_remove(&host_index_ida, shost->host_no);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -388,6 +390,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 {
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+   int index;
 
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -406,11 +409,11 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
 
-   /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
-   shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+   index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
+   if (index < 0)
+   goto fail_kfree;
+   shost->host_no = index;
+
shost->dma_channel = 0xff;
 
/* These three are default values which can be overridden */
@@ -495,7 +498,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
-   goto fail_kfree;
+   goto fail_index_remove;
}
 
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -511,6 +514,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 
  fail_kthread:
kthread_stop(shost->ehandler);
+ fail_index_remove:
+   ida_simple_remove(&host_index_ida, shost->host_no);
  fail_kfree:
kfree(shost);
return NULL;
@@ -606,6 +611,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
class_unregister(&shost_class);
+   ida_destroy(&host_index_ida);
 }
 
 int scsi_is_host_device(const struct device *dev)
-- 
2.1.4

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 12/17/2015 11:24 AM, Lee Duncan wrote:
> On 12/14/2015 05:55 PM, Martin K. Petersen wrote:
>>>>>>> "Hannes" == Hannes Reinecke  writes:
>>
>>>> I'm not opposed to having the module option if others (Martin?) feel
>>>> they need it, but generally I think it's better to keep things as
>>>> simple as possible.  So, unless there are strong objections, I would
>>>> say no.
>>
>> Hannes> Agreeing with Ewan here.
>>
>> Hannes> I guess it's up to you to tell us whether you absolutely need a
>> Hannes> module parameter ...
>>
>> Still not a big ida fan but since the most people seem to be in favor of
>> this I guess I'll have to bite the bullet.
>>
>> I don't see much value in the module parameter since it will require
>> customers to tweak their configs and reproduce. Not worth the hassle.
>>
> 
> Thank you Martin. I'll look at further cleaning up the host module, but
> I think this still much better than leaving the code as is.
> 

James:

Do you need me to resubmit this patch now that it's accepted?
-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 12/14/2015 05:55 PM, Martin K. Petersen wrote:
>>>>>> "Hannes" == Hannes Reinecke  writes:
> 
>>> I'm not opposed to having the module option if others (Martin?) feel
>>> they need it, but generally I think it's better to keep things as
>>> simple as possible.  So, unless there are strong objections, I would
>>> say no.
> 
> Hannes> Agreeing with Ewan here.
> 
> Hannes> I guess it's up to you to tell us whether you absolutely need a
> Hannes> module parameter ...
> 
> Still not a big ida fan but since the most people seem to be in favor of
> this I guess I'll have to bite the bullet.
> 
> I don't see much value in the module parameter since it will require
> customers to tweak their configs and reproduce. Not worth the hassle.
> 

Thank you Martin. I'll look at further cleaning up the host module, but
I think this still much better than leaving the code as is.

-- 
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 12/11/2015 07:31 AM, Ewan Milne wrote:
> On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
>> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>>>> "Lee" == Lee Duncan  writes:
>>>
>>> Lee> Martin: I will be glad to update the patch, creating a modprobe
>>> Lee> parameter as suggested, if you find this acceptable.
>>>
>>> For development use a module parameter would be fine. But I am concerned
>>> about our support folks that rely on the incrementing host number when
>>> analyzing customer log files.
>>>
>>> Ewan: How do you folks feel about this change?
>>>
>>
>> Ewan?
> 
> 
> Personally, I think having host numbers that increase essentially
> without limit (I think I've seen this with iSCSI sessions) are a
> problem, the numbers start to lose meaning for people when they
> are not easily recognizable.  Yes, it can help when you're analyzing
> a log file, but it seems to me that you would want to track the
> host state throughout anyway, so you could just follow the number
> as it changes.
> 
> If we change the behavior, we have to change documentation, and
> our support people will get calls.  But that's not a reason not
> to do it.
> 
> -Ewan
> 

Ewan:

Thank you for your reply. I agree with you, which is why I generated
this patch.

If we *do* make this change, do you think it would be useful to have a
module option to revert to the old numbering behavior? I actually think
it would be more confusing to support two behaviors than it would be to
bite the bullet (so to speak) and make the change.

-- 
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan  writes:
> 
> Lee> Martin: I will be glad to update the patch, creating a modprobe
> Lee> parameter as suggested, if you find this acceptable.
> 
> For development use a module parameter would be fine. But I am concerned
> about our support folks that rely on the incrementing host number when
> analyzing customer log files.
> 
> Ewan: How do you folks feel about this change?
> 

Ewan?

-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 11/16/2015 04:10 AM, Hannes Reinecke wrote:
> On 11/13/2015 10:54 PM, Martin K. Petersen wrote:
>>>>>>> "Lee" == Lee Duncan  writes:
>>
>>>> Well, I'm a bit worried about the loss of a monotonically increasing
>>>> host number from the debugging perspective.  Right now, if you look
>>>> at any log, hostX always refers to one and only one incarnation
>>>> throughout the system lifetime for any given value of X.
>>
>> That's a feature that I would absolutely hate to lose. I spend a huge
>> amount of time looking at system logs.
>>
> Right. Then have it enabled via a modprobe parameters.
> 
> We actually had customers running into a host_no overflow due to
> excessive host allocations and freeing done by iSCSI.
> 
> Cheers,
> 
> Hannes
> 

Martin: I will be glad to update the patch, creating a modprobe
parameter as suggested, if you find this acceptable.
-- 
Lee Duncan
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 10/14/2015 08:53 PM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James

James?

It looks like both Hannes and GregKH agreed this was the proper approach.

-- 
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

Adding linux-usb and linux-hotplug to cc list, in case they wish to comment.

Summary: I want to change SCSI host number so that it gets re-used, like
disk index numbers, instead of always increasing.

Please see below.

On 10/14/2015 11:53 AM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James
> 
> 
> 
> 

-- 
Lee Duncan
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 10/14/2015 11:53 AM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James
> 

James:

Understand your point, but I've never seen the host number not repeating
be a benefit in debugging or testing. And Hannes suggested this fix, so
I can only assume he also did not see a benefit of unique host
numbering. And purely aesthetically, seeing "host4595483528" in sysfs
would not be very user-friendly.

But one possible solution to address your concern would be to increase
the host number until it ran out of room (or hit some large maximum),
and only then start re-using host numbers. This would preserve the
current monotonically-increasing behavior at least initially. But I
worry that having this bi-modal numbering scheme might confuse some.
-- 
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 10/14/2015 06:55 AM, James Bottomley wrote:
> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>> Update the SCSI hosts module to use the ida_simple*() routines
>> to manage its host_no index instead of an ATOMIC integer. This
>> means that the SCSI host number will now be reclaimable.
> 
> OK, but why would we want to do this?  We do it for sd because our minor
> space for the device nodes is very constrained, so packing is essential.
> For HBAs, there's no device space density to worry about, they're
> largely statically allocated at boot time and not reusing the numbers
> allows easy extraction of hotplug items for the logs (quite useful for
> USB) because each separate hotplug has a separate and monotonically
> increasing host number.
> 
> James
> 

Good question, James. Apologies for not making the need clear.

The iSCSI subsystem uses a host structure for discovery, then throws it
away. So each time it does discovery it gets a new host structure. With
the current approach, that number is ever increasing. It's only a matter
of time until some user with a hundreds of disks and perhaps thousands
of LUNs, that likes to do periodic discovery (think super-computers)
will run out of host numbers. Or, worse yet, get a negative number
number (because the value is signed right now).

And this use case is a real one right now, by the way.

As you can see from the patch, it's a small amount of code to ensure
that the host number management is handled more cleanly.

-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 0/1] Update SCSI hosts to use idr for host number mgmt

[Lee Duncan]

Duplicate email, please ignore

On 10/07/2015 04:47 PM, Lee Duncan wrote:
> This patch updates the SCSI hosts module to use the idr
> index-management routines to manage its host_no index instead
> of using an ATOMIC integer. This means that host numbers
> can now be reclaimed and re-used.
> 
> It also updates the hosts module to use the idr routine idr_find()
> to lookup hosts based on the host number, hopefully speeding
> up said lookup.
> 
> After noticing that my idr calling sequences where very close
> to those in other modules, I considered creating some idr helper
> functions (and using them), but because idr usage almost always
> requires the caller to manage their own locks, I gave up on
> this approach (as suggested by Tejon -- thank you).
> 
> Changes from v1:
>  - no longer using helper routines
> Changes from v2:
>  - added back missing scsi_host_get() in scsi_host_lookup()
> 
> Lee Duncan (1):
>   SCSI: update hosts module to use idr index management
> 
>  drivers/scsi/hosts.c | 61 
> ++----------
>  1 file changed, 31 insertions(+), 30 deletions(-)
> 

-- 
Lee Duncan
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

Update the SCSI hosts module to use the ida_simple*() routines
to manage its host_no index instead of an ATOMIC integer. This
means that the SCSI host number will now be reclaimable.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/hosts.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..b6a5ffa886b7 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include 
 #include 
 #include 
@@ -42,7 +42,7 @@
 #include "scsi_logging.h"
 
 
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new 
host */
+static DEFINE_IDA(host_index_ida);
 
 
 static void scsi_host_cls_release(struct device *dev)
@@ -337,6 +337,8 @@ static void scsi_host_dev_release(struct device *dev)
 
kfree(shost->shost_data);
 
+   ida_simple_remove(&host_index_ida, shost->host_no);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -370,6 +372,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 {
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+   int index;
 
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -388,11 +391,11 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
 
-   /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
-   shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+   index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
+   if (index < 0)
+   goto fail_kfree;
+   shost->host_no = index;
+
shost->dma_channel = 0xff;
 
/* These three are default values which can be overridden */
@@ -477,7 +480,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
-   goto fail_kfree;
+   goto fail_index_remove;
}
 
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +496,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 
  fail_kthread:
kthread_stop(shost->ehandler);
+ fail_index_remove:
+   ida_simple_remove(&host_index_ida, shost->host_no);
  fail_kfree:
kfree(shost);
return NULL;
@@ -588,6 +593,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
class_unregister(&shost_class);
+   ida_destroy(&host_index_ida);
 }
 
 int scsi_is_host_device(const struct device *dev)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv4 0/1] Update SCSI hosts to use ida for host number mgmt

[Lee Duncan]

This patch updates the SCSI hosts module to use the ida
index-management routines to manage its host_no index instead
of using an ATOMIC integer. This means that host numbers
can now be reclaimed and re-used.

NOTE: it was not feasible to use idr_*() functions instead of
ida_*() functions, since using idr_find() without additional
locking to find our Scsi_Host structure left a window between
idr lookup and host structure deletion that would have
required additional locking to close.

Changes from v3:
 - Switched from idr to ida since managing our instance
   pointer required extra locking
Changes from v2 and v1:
 - First two version used idr instead of ida

Lee Duncan (1):
  SCSI: hosts: update to use ida_simple for host_no management

 drivers/scsi/hosts.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 1/1] SCSI: update hosts module to use idr index management

[Lee Duncan]

On 10/06/2015 12:19 PM, James Bottomley wrote:
> On Tue, 2015-10-06 at 12:08 -0700, Lee Duncan wrote:
>> Update the SCSI hosts module to use idr to manage
>> its host_no index instead of an ATOMIC integer. This
>> also allows using idr_find() to look up the SCSI
>> host structure given the host number.
>>
>> This means that the SCSI host number will now
>> be reclaimable.
>>
>> Signed-off-by: Lee Duncan 
>> Reviewed-by: Hannes Reinecke 
>> ---
>>  drivers/scsi/hosts.c | 61 
>> ++--
>>  1 file changed, 31 insertions(+), 30 deletions(-
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 8bb173e01084..afe7bd962ddb 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
> [...]
>> +spin_lock(&host_index_lock);
>> +shost = idr_find(&host_index_idr, hostnum);
>> +spin_unlock(&host_index_lock);
>> +
>> +return shost ? scsi_host_get(shost) : NULL;
> 
> So the thing I don't like here is that there's a race between
> scsi_host_get() and the final put.  What could happen is that idr_find()
> returns the host just before but scsi_host_dev_release() is executed
> before the return.  In that instance, we'll reference freed memory in
> scsi_host_get() ... probably completely harmlessly, but it will show up
> occasionally on some of the traces ... particularly the ones doing a
> fuzz/stress test around host create/destroy.
> 
> James

Good point. The scenario you mention may be a corner case, but I also
don't like it. I cannot see another good way to synchronize lookup and
removal other than adding a new lock, which would be dumb.

I will submit my "host number" patch again, without the change to
scsi_host_lookup().

-- 
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv3 0/1] Update SCSI hosts to use idr for host number mgmt

[Lee Duncan]

This patch updates the SCSI hosts module to use the idr
index-management routines to manage its host_no index instead
of using an ATOMIC integer. This means that host numbers
can now be reclaimed and re-used.

It also updates the hosts module to use the idr routine idr_find()
to lookup hosts based on the host number, hopefully speeding
up said lookup.

After noticing that my idr calling sequences where very close
to those in other modules, I considered creating some idr helper
functions (and using them), but because idr usage almost always
requires the caller to manage their own locks, I gave up on
this approach (as suggested by Tejon -- thank you).

Changes from v1:
 - no longer using helper routines
Changes from v2:
 - added back missing scsi_host_get() in scsi_host_lookup()

Lee Duncan (1):
  SCSI: update hosts module to use idr index management

 drivers/scsi/hosts.c | 61 ++--
 1 file changed, 31 insertions(+), 30 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv3 1/1] SCSI: update hosts module to use idr index management

[Lee Duncan]

Update the SCSI hosts module to use idr to manage
its host_no index instead of an ATOMIC integer. This
also allows using idr_find() to look up the SCSI
host structure given the host number.

This means that the SCSI host number will now
be reclaimable.

Signed-off-by: Lee Duncan 
Reviewed-by: Hannes Reinecke 
---
 drivers/scsi/hosts.c | 61 ++--
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..afe7bd962ddb 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include 
 #include 
 #include 
@@ -41,8 +41,8 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
-
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new 
host */
+static DEFINE_IDR(host_index_idr);
+static DEFINE_SPINLOCK(host_index_lock);
 
 
 static void scsi_host_cls_release(struct device *dev)
@@ -337,6 +337,10 @@ static void scsi_host_dev_release(struct device *dev)
 
kfree(shost->shost_data);
 
+   spin_lock(&host_index_lock);
+   idr_remove(&host_index_idr, shost->host_no);
+   spin_unlock(&host_index_lock);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -370,6 +374,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 {
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+   int index;
 
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -388,11 +393,15 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
 
-   /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
-   shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+   idr_preload(GFP_KERNEL);
+   spin_lock(&host_index_lock);
+   index = idr_alloc(&host_index_idr, shost, 0, 0, GFP_NOWAIT);
+   spin_unlock(&host_index_lock);
+   idr_preload_end();
+   if (index < 0)
+   goto fail_kfree;
+   shost->host_no = index;
+
shost->dma_channel = 0xff;
 
/* These three are default values which can be overridden */
@@ -477,7 +486,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
-   goto fail_kfree;
+   goto fail_idr_remove;
}
 
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +502,10 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
 
  fail_kthread:
kthread_stop(shost->ehandler);
+ fail_idr_remove:
+   spin_lock(&host_index_lock);
+   idr_remove(&host_index_idr, shost->host_no);
+   spin_unlock(&host_index_lock);
  fail_kfree:
kfree(shost);
return NULL;
@@ -522,15 +535,6 @@ void scsi_unregister(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_unregister);
 
-static int __scsi_host_match(struct device *dev, const void *data)
-{
-   struct Scsi_Host *p;
-   const unsigned short *hostnum = data;
-
-   p = class_to_shost(dev);
-   return p->host_no == *hostnum;
-}
-
 /**
  * scsi_host_lookup - get a reference to a Scsi_Host by host no
  * @hostnum:   host number to locate
@@ -539,21 +543,17 @@ static int __scsi_host_match(struct device *dev, const 
void *data)
  * A pointer to located Scsi_Host or NULL.
  *
  * The caller must do a scsi_host_put() to drop the reference
- * that scsi_host_get() took. The put_device() below dropped
- * the reference from class_find_device().
+ * that scsi_host_get() takes.
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-   struct device *cdev;
-   struct Scsi_Host *shost = NULL;
-
-   cdev = class_find_device(&shost_class, NULL, &hostnum,
-__scsi_host_match);
-   if (cdev) {
-   shost = scsi_host_get(class_to_shost(cdev));
-   put_device(cdev);
-   }
-   return shost;
+   struct Scsi_Host *shost;
+
+   spin_lock(&host_index_lock);
+   shost = idr_find(&host_index_idr, hostnum);
+   spin_unlock(&host_index_lock);
+
+   return shost ? scsi_host_get(shost) : NULL;
 }
 EXPORT_SYMBOL(scsi_host_lookup);
 
@@ -588,6 +588,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
class_unregister(&shost_class);
+   idr_destroy(&host_index_idr);
 }

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Lee Duncan]

On 10/06/2015 02:40 AM, Christoph Hellwig wrote:
>>  struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
>>  {
>> -struct device *cdev;
>> -struct Scsi_Host *shost = NULL;
>> -
>> -cdev = class_find_device(&shost_class, NULL, &hostnum,
>> - __scsi_host_match);
>> -if (cdev) {
>> -shost = scsi_host_get(class_to_shost(cdev));
>> -put_device(cdev);
>> -}
>> +struct Scsi_Host *shost;
>> +
>> +spin_lock(&host_index_lock);
>> +shost = idr_find(&host_index_idr, hostnum);
>> +spin_unlock(&host_index_lock);
>> +
>>  return shost;
> 
> How does this actually grab a reference to the host?

Good catch -- I should have noticed that.

I will resubmit the patch.
-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv2 0/1] Update SCSI hosts to use idr for host number mgmt

[Lee Duncan]

This patch updates the SCSI hosts module to use the idr
index-management routines to manage its host_no index instead
of using an ATOMIC integer. This means that host numbers
can now be reclaimed and re-used.

It also updates the hosts module to use the idr routine idr_find()
to lookup hosts based on the host number, hopefully speeding
up said lookup.

After noticing that my idr calling sequences where very close
to those in other modules, I considered creating some idr helper
functions (and using them), but because idr usage almost always
requires the caller to manage their own locks, I gave up on
this approach (as suggested by Tejon -- thank you).

Lee Duncan (1):
  SCSI: update hosts module to use idr index management

 drivers/scsi/hosts.c | 60 +---
 1 file changed, 29 insertions(+), 31 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Lee Duncan]

Update the SCSI hosts module to use idr to manage
its host_no index instead of an ATOMIC integer. This
also allows using idr_find() to look up the SCSI
host structure given the host number.

This means that the SCSI host number will now
be reclaimable.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/hosts.c | 60 +---
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..9dc8ff971f5a 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include 
 #include 
 #include 
@@ -41,8 +41,8 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
-
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new 
host */
+static DEFINE_IDR(host_index_idr);
+static DEFINE_SPINLOCK(host_index_lock);
 
 
 static void scsi_host_cls_release(struct device *dev)
@@ -337,6 +337,10 @@ static void scsi_host_dev_release(struct device *dev)
 
kfree(shost->shost_data);
 
+   spin_lock(&host_index_lock);
+   idr_remove(&host_index_idr, shost->host_no);
+   spin_unlock(&host_index_lock);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -370,6 +374,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 {
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+   int index;
 
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -388,11 +393,15 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
 
-   /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
-   shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+   idr_preload(GFP_KERNEL);
+   spin_lock(&host_index_lock);
+   index = idr_alloc(&host_index_idr, shost, 0, 0, GFP_NOWAIT);
+   spin_unlock(&host_index_lock);
+   idr_preload_end();
+   if (index < 0)
+   goto fail_kfree;
+   shost->host_no = index;
+
shost->dma_channel = 0xff;
 
/* These three are default values which can be overridden */
@@ -477,7 +486,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
-   goto fail_kfree;
+   goto fail_idr_remove;
}
 
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +502,10 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
 
  fail_kthread:
kthread_stop(shost->ehandler);
+ fail_idr_remove:
+   spin_lock(&host_index_lock);
+   idr_remove(&host_index_idr, shost->host_no);
+   spin_unlock(&host_index_lock);
  fail_kfree:
kfree(shost);
return NULL;
@@ -522,37 +535,21 @@ void scsi_unregister(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_unregister);
 
-static int __scsi_host_match(struct device *dev, const void *data)
-{
-   struct Scsi_Host *p;
-   const unsigned short *hostnum = data;
-
-   p = class_to_shost(dev);
-   return p->host_no == *hostnum;
-}
-
 /**
  * scsi_host_lookup - get a reference to a Scsi_Host by host no
  * @hostnum:   host number to locate
  *
  * Return value:
  * A pointer to located Scsi_Host or NULL.
- *
- * The caller must do a scsi_host_put() to drop the reference
- * that scsi_host_get() took. The put_device() below dropped
- * the reference from class_find_device().
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-   struct device *cdev;
-   struct Scsi_Host *shost = NULL;
-
-   cdev = class_find_device(&shost_class, NULL, &hostnum,
-__scsi_host_match);
-   if (cdev) {
-   shost = scsi_host_get(class_to_shost(cdev));
-   put_device(cdev);
-   }
+   struct Scsi_Host *shost;
+
+   spin_lock(&host_index_lock);
+   shost = idr_find(&host_index_idr, hostnum);
+   spin_unlock(&host_index_lock);
+
return shost;
 }
 EXPORT_SYMBOL(scsi_host_lookup);
@@ -588,6 +585,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
class_unregister(&shost_class);
+   idr_destroy(&host_index_idr);
 }
 
 int scsi_is_host_device(const struct device *dev)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/5] SCSI: sd: simplify ida usage

[Lee Duncan]

Simplify ida index allocation and removal by
using the ida_simple_* helper functions.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/sd.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3b2fcb4fada0..3d77ac8f0d4c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -118,7 +118,6 @@ static void scsi_disk_release(struct device *cdev);
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
 static void sd_print_result(const struct scsi_disk *, const char *, int);
 
-static DEFINE_SPINLOCK(sd_index_lock);
 static DEFINE_IDA(sd_index_ida);
 
 /* This semaphore is used to mediate the 0->1 reference get in the
@@ -2948,19 +2947,12 @@ 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);
-
-   if (error) {
+   error = ida_simple_get(&sd_index_ida, 0, 0, GFP_KERNEL);
+   if (error < 0) {
sdev_printk(KERN_WARNING, sdp, "sd_probe: memory exhausted.\n");
goto out_put;
}
+   index = error;
 
error = sd_format_disk_name("sd", index, gd->disk_name, DISK_NAME_LEN);
if (error) {
@@ -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_simple_remove(&sd_index_ida, 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_simple_remove(&sd_index_ida, sdkp->index);
 
blk_integrity_unregister(disk);
disk->private_data = NULL;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/5] base: soc: siplify ida usage

[Lee Duncan]

Simplify ida index allocation and removal by
using the ida_simple_* helper functions

Signed-off-by: Lee Duncan 
---
 drivers/base/soc.c | 21 +
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 39fca01c8fa1..75b98aad6faf 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -16,7 +16,6 @@
 #include 
 
 static DEFINE_IDA(soc_ida);
-static DEFINE_SPINLOCK(soc_lock);
 
 static ssize_t soc_info_get(struct device *dev,
struct device_attribute *attr,
@@ -122,20 +121,10 @@ 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);
-
-   if (ret)
+   ret = ida_simple_get(&soc_ida, 0, 0, GFP_KERNEL);
+   if (ret < 0)
goto out2;
+   soc_dev->soc_dev_num = ret;
 
soc_dev->attr = soc_dev_attr;
soc_dev->dev.bus = &soc_bus_type;
@@ -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_simple_remove(&soc_ida, 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_simple_remove(&soc_ida, soc_dev->soc_dev_num);
 
device_unregister(&soc_dev->dev);
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/5] block: rsxx: core: simplify ida usage

[Lee Duncan]

Simplify ida index allocation and removal by
using the ida_simple_* helper functions.

Signed-off-by: Lee Duncan 
---
 drivers/block/rsxx/core.c | 20 
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/block/rsxx/core.c b/drivers/block/rsxx/core.c
index d8b2488aaade..d2279a759b2e 100644
--- a/drivers/block/rsxx/core.c
+++ b/drivers/block/rsxx/core.c
@@ -58,7 +58,6 @@ MODULE_PARM_DESC(sync_start, "On by Default: Driver load will 
not complete "
 "until the card startup has completed.");
 
 static DEFINE_IDA(rsxx_disk_ida);
-static DEFINE_SPINLOCK(rsxx_ida_lock);
 
 /* Debugfs Setup --- */
 
@@ -774,19 +773,10 @@ 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);
-
-   if (st)
+   st = ida_simple_get(&rsxx_disk_ida, 0, 0, GFP_KERNEL);
+   if (st < 0)
goto failed_ida_get;
+   card->disk_id = st;
 
st = pci_enable_device(dev);
if (st)
@@ -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_simple_remove(&rsxx_disk_ida, card->disk_id);
 failed_ida_get:
kfree(card);
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/5] block: mtip32xx: simplify ida usage

[Lee Duncan]

Simplify ida index allocation and removal by
using the ida_simple_* helper functions

Signed-off-by: Lee Duncan 
---
 drivers/block/mtip32xx/mtip32xx.c | 26 ++
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 4a2ef09e6704..e62d170b0641 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -118,7 +118,6 @@ static struct dentry *dfs_device_status;
 
 static u32 cpu_use[NR_CPUS];
 
-static DEFINE_SPINLOCK(rssd_index_lock);
 static DEFINE_IDA(rssd_index_ida);
 
 static int mtip_block_initialize(struct driver_data *dd);
@@ -3821,17 +3820,10 @@ 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);
-
-   if (rv)
+   rv = ida_simple_get(&rssd_index_ida, 0, 0, GFP_KERNEL);
+   if (rv < 0)
goto ida_get_error;
+   index = rv;
 
rv = rssd_disk_name_format("rssd",
index,
@@ -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_simple_remove(&rssd_index_ida, 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_simple_remove(&rssd_index_ida, 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_simple_remove(&rssd_index_ida, dd->index);
return 0;
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/5] Modify ida_* users to use ida_simple_*

[Lee Duncan]

The ida index management routines are used in several
driver modules to manage allocation and release of
index values. Reviewing the way in which the
ida routines were called, together with the small
number of such clients, led to the belief that
these users should all be able to share a simple
built-in lock in the ida module by calling the
ida_simple_*() functions instead of the non-simple
versions. This means that ida does all the
required locking so that clients don't have to
manage that.

This will greatly simplify the client calling code,
and if there is any problem with these clients
sharing a "simple" lock, the ida code can be
transparently expanded to allocate a lock per client,
without having to change any of the clients again.

NOTE: this patch series replaces an earlier attempt
to create a new set of ida helper functions titled:

  "Create and use ida and idr helper routines"

Another set will soon be sent out soon to (1) add idr
helper functions, (2) modify clients to use them,
and (3) update SCSI host_no to use them.

Lee Duncan (5):
  SCSI: sd: simplify ida usage
  block: rsxx: core: simplify ida usage
  block: nvme-core: simplify ida usage
  block: mtip32xx: simplify ida usage
  base: soc: siplify ida usage

 drivers/base/soc.c| 21 +
 drivers/block/mtip32xx/mtip32xx.c | 26 ++
 drivers/block/nvme-core.c | 16 
 drivers/block/rsxx/core.c | 20 
 drivers/scsi/sd.c | 22 +-
 5 files changed, 24 insertions(+), 81 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/5] block: nvme-core: simplify ida usage

[Lee Duncan]

Simplify ida index allocation and removal by
using the ida_simple_* helper functions.

Signed-off-by: Lee Duncan 
---
 drivers/block/nvme-core.c | 16 
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index d1d6141920d3..d354a3391e4a 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -2713,18 +2713,10 @@ static DEFINE_IDA(nvme_instance_ida);
 
 static int nvme_set_instance(struct nvme_dev *dev)
 {
-   int instance, error;
+   int instance;
 
-   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);
-
-   if (error)
+   instance = ida_simple_get(&nvme_instance_ida, 0, 0, GFP_KERNEL);
+   if (instance < 0)
return -ENODEV;
 
dev->instance = instance;
@@ -2734,7 +2726,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);
+   ida_simple_remove(&nvme_instance_ida, dev->instance);
spin_unlock(&dev_list_lock);
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 01/17] Add ida and idr helper routines.

[Lee Duncan]

On 09/15/2015 11:41 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Sep 15, 2015 at 11:38:42AM -0700, James Bottomley wrote:
>> For most of the SCSI stuff, yes.  I'm less sure about the sd numbers.
>> They go up very high and get hammered a lot during system bring up and
>> hot plug.  I think having their own lock rather than wrapping everything
>> around simple_ida_lock makes more sense here just because the system is
>> heavily contended on getting indexes at bring up.
>>
>> To continue the thought, why not move simple_ida_lock into struct ida so
>> we don't have to worry about the contention and can sue ida_simple_...
>> everywhere?
> 
> We sure can do that if necessary but I'm rather doubtful that even
> with sd number hammering this is likely to be a problem.  Let's
> convert the users to the simple interface and make the lock per-ida if
> we actually see contention on the lock.
> 
> Thanks.
> 

To be clear: you would like a patch series that converts the users of
the ida_* routines in my patches to instead use the ida_simple_*
routines, correct? And of course the ida_* helper routines I was adding
in idr.h would not be needed.

If this is correct, I will supply a version 2 patch series that
addresses this issue as well as the two patch-naming issues that were
raised.
-- 
Lee Duncan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 05/17] Update the md driver to use idr helper functions.

[Lee Duncan]

On 09/15/2015 11:05 AM, Mike Snitzer wrote:
> Subject should really be:
> "dm: update to use idr helper functions"

Yes, I grabbed the wrong part of the driver pathname.

Would it be better to resubmit just this patch, or to resubmit the series?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 02/17] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

Each Scsi_Host instance gets a host number starting
at 0, but this was implemented with an atomic integer,
and rollover wasn't considered. Another problem with
this design is that scsi host numbers used by iscsi
are never reused, thereby making rollover more likely.
This patch converts Scsi_Host instances to use idr
to manage their instance numbers and to simplify
instance number to pointer lookups.

This also means that host instance numbers will be
reused, when available.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/hosts.c | 59 
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..61201bc03b98 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include 
 #include 
 #include 
@@ -42,8 +42,6 @@
 #include "scsi_logging.h"
 
 
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new 
host */
-
 
 static void scsi_host_cls_release(struct device *dev)
 {
@@ -304,6 +302,19 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct 
device *dev,
 }
 EXPORT_SYMBOL(scsi_add_host_with_dma);
 
+static DEFINE_SPINLOCK(host_index_lock);
+static DEFINE_IDR(host_index_idr);
+
+static inline int host_get_index(void *ptr)
+{
+   return idr_get_index(&host_index_idr, &host_index_lock, ptr);
+}
+
+static inline void host_put_index(int index)
+{
+   idr_put_index(&host_index_idr, &host_index_lock, index);
+}
+
 static void scsi_host_dev_release(struct device *dev)
 {
struct Scsi_Host *shost = dev_to_shost(dev);
@@ -337,6 +348,8 @@ static void scsi_host_dev_release(struct device *dev)
 
kfree(shost->shost_data);
 
+   host_put_index(shost->host_no);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -370,6 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 {
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+   int index;
 
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -388,11 +402,11 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
 
-   /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
-   shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+   index = host_get_index(shost);
+   if (index < 0)
+   goto fail_kfree;
+   shost->host_no = index;
+
shost->dma_channel = 0xff;
 
/* These three are default values which can be overridden */
@@ -477,7 +491,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
-   goto fail_kfree;
+   goto fail_idr_remove;
}
 
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +507,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 
  fail_kthread:
kthread_stop(shost->ehandler);
+ fail_idr_remove:
+   host_put_index(shost->host_no);
  fail_kfree:
kfree(shost);
return NULL;
@@ -522,38 +538,16 @@ void scsi_unregister(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_unregister);
 
-static int __scsi_host_match(struct device *dev, const void *data)
-{
-   struct Scsi_Host *p;
-   const unsigned short *hostnum = data;
-
-   p = class_to_shost(dev);
-   return p->host_no == *hostnum;
-}
-
 /**
  * scsi_host_lookup - get a reference to a Scsi_Host by host no
  * @hostnum:   host number to locate
  *
  * Return value:
  * A pointer to located Scsi_Host or NULL.
- *
- * The caller must do a scsi_host_put() to drop the reference
- * that scsi_host_get() took. The put_device() below dropped
- * the reference from class_find_device().
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-   struct device *cdev;
-   struct Scsi_Host *shost = NULL;
-
-   cdev = class_find_device(&shost_class, NULL, &hostnum,
-__scsi_host_match);
-   if (cdev) {
-   shost = scsi_host_get(class_to_shost(cdev));
-   put_device(cdev);
-   }
-   return shost;
+   return idr_find(&host_index_idr, hostnum);
 }
 EXPORT_SYMBOL(scsi_host_lookup);
 
@@ -588,6 +582,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
class_unregister(&shost_class);
+   idr_destroy(&am

[PATCH 04/17] Update the ch driver to use idr helper functions.

[Lee Duncan]

Note: when allocating an index, in the error case,
where the just-allocated index has to be released,
the required locking around the index removal was
not present, so this conversion has the side effect
of adding locking for that error condition. This
should close a possible race condition that could
have resulted in duplicate index allocation.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/ch.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index dad959fcf6d8..2edf1f8883f9 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -909,12 +909,8 @@ static int ch_probe(struct device *dev)
if (NULL == ch)
return -ENOMEM;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&ch_index_lock);
-   ret = idr_alloc(&ch_index_idr, ch, 0, CH_MAX_DEVS + 1, GFP_NOWAIT);
-   spin_unlock(&ch_index_lock);
-   idr_preload_end();
-
+   ret = idr_get_index_in_range(&ch_index_idr, &ch_index_lock, ch,
+0, CH_MAX_DEVS + 1);
if (ret < 0) {
if (ret == -ENOSPC)
ret = -ENODEV;
@@ -945,7 +941,7 @@ static int ch_probe(struct device *dev)
 
return 0;
 remove_idr:
-   idr_remove(&ch_index_idr, ch->minor);
+   idr_put_index(&ch_index_idr, &ch_index_lock, ch->minor);
 free_ch:
kfree(ch);
return ret;
@@ -955,9 +951,7 @@ static int ch_remove(struct device *dev)
 {
scsi_changer *ch = dev_get_drvdata(dev);
 
-   spin_lock(&ch_index_lock);
-   idr_remove(&ch_index_idr, ch->minor);
-   spin_unlock(&ch_index_lock);
+   idr_put_index(&ch_index_idr, &ch_index_lock, ch->minor);
 
device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
kfree(ch->dt);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 06/17] Update the infiniband uverbs driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/infiniband/core/uverbs_cmd.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c 
b/drivers/infiniband/core/uverbs_cmd.c
index bbb02ffe87df..1e5b2a66a501 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -120,24 +120,16 @@ static int idr_add_uobj(struct idr *idr, struct 
ib_uobject *uobj)
 {
int ret;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&ib_uverbs_idr_lock);
-
-   ret = idr_alloc(idr, uobj, 0, 0, GFP_NOWAIT);
+   ret = idr_get_index(idr, &ib_uverbs_idr_lock, uobj);
if (ret >= 0)
uobj->id = ret;
 
-   spin_unlock(&ib_uverbs_idr_lock);
-   idr_preload_end();
-
return ret < 0 ? ret : 0;
 }
 
 void idr_remove_uobj(struct idr *idr, struct ib_uobject *uobj)
 {
-   spin_lock(&ib_uverbs_idr_lock);
-   idr_remove(idr, uobj->id);
-   spin_unlock(&ib_uverbs_idr_lock);
+   idr_put_index(idr, &ib_uverbs_idr_lock, uobj->id);
 }
 
 static struct ib_uobject *__idr_get_uobj(struct idr *idr, int id,
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 01/17] Add ida and idr helper routines.

[Lee Duncan]

Clients of the ida and idr index-management routines
tend to use the same calling sequences much of the time,
so this change adds helper functions for allocating and
releasing indexes of either flavor, i.e. with or
without pointer management.

Inline functions added for idr:
  idr_get_index_in_range
  idr_get_index (in range 0,0)
  idr_put_index
And for ida:
  ida_get_index
  ida_put_index

Signed-off-by: Lee Duncan 
---
 include/linux/idr.h | 102 
 1 file changed, 102 insertions(+)

diff --git a/include/linux/idr.h b/include/linux/idr.h
index 013fd9bc4cb6..341c4f2d9874 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -16,6 +16,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /*
  * We want shallower trees and thus more bits covered at each layer.  8
@@ -183,4 +185,104 @@ 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
+ * @idaidr 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
+ * @idaidr 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);
+}
+
+/**
+ * idr_get_index_in_range - allocate a new index, with locking
+ * within a range
+ * @idr:   idr handle
+ * @lock:  spin lock handle protecting the index
+ * @ptr:   pointer to associate with allocated index
+ * @start: starting index (see idr_alloc)
+ * @end:   ending index (0 -> use default max)
+ *
+ * This is a helper routine meant to make the common
+ * calling sequence to allocate an idr index easier.
+ * It uses a spin lock, and allocates a positive index
+ * in the range of [start,end), returning a negative
+ * errno on failure, else 0.
+ */
+static inline int idr_get_index_in_range(struct idr *idr, spinlock_t *lock,
+void *ptr, int start, int end)
+{
+   int ret;
+
+   idr_preload(GFP_KERNEL);
+   spin_lock(lock);
+   ret = idr_alloc(idr, ptr, start, end, GFP_NOWAIT);
+   spin_unlock(lock);
+   idr_preload_end();
+
+   return ret;
+}
+
+/**
+ * idr_get_index - allocate new index, with locking, using the
+ *default range (zero to max-1)
+ * @idr:   idr handle
+ * @lock:  spin lock handle protecting the index
+ * @ptr:   pointer to associate with allocated index
+ *
+ * Simple wrapper around idr_get_index_in_range() w/ @start and
+ * @end of 0, since this is a common case
+ */
+static inline int idr_get_index(struct idr *idr, spinlock_t *lock, void *ptr)
+{
+   return idr_get_index_in_range(idr, lock, ptr, 0, 0);
+}
+
+/**
+ * idr_put_index - free an allocated idr index value
+ * @idridr handle
+ * @lock   spinlock handle protecting this index
+ * @id the value of the allocated index
+ *
+ * A helper function that goes with @idr_get_index, which safely
+ * frees a previously-allocated index value.
+ */
+static inline void idr_put_index(struct idr *idr, spinlock_t *lock, int id)
+{
+   spin_lock(lock);
+   idr_remove(idr, id);
+   spin_unlock(lock);
+}
+
 #endif /* __IDR_H__ */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 07/17] Update the memstick driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/memstick/core/memstick.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index a0547dbf9806..8f40a3d5108b 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -512,25 +512,16 @@ int memstick_add_host(struct memstick_host *host)
 {
int rc;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&memstick_host_lock);
-
-   rc = idr_alloc(&memstick_host_idr, host, 0, 0, GFP_NOWAIT);
-   if (rc >= 0)
-   host->id = rc;
-
-   spin_unlock(&memstick_host_lock);
-   idr_preload_end();
+   rc = idr_get_index(&memstick_host_idr, &memstick_host_lock, host);
if (rc < 0)
return rc;
+   host->id = rc;
 
dev_set_name(&host->dev, "memstick%u", host->id);
 
rc = device_add(&host->dev);
if (rc) {
-   spin_lock(&memstick_host_lock);
-   idr_remove(&memstick_host_idr, host->id);
-   spin_unlock(&memstick_host_lock);
+   idr_put_index(&memstick_host_idr, &memstick_host_lock, 
host->id);
return rc;
}
 
@@ -554,9 +545,7 @@ void memstick_remove_host(struct memstick_host *host)
host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF);
mutex_unlock(&host->lock);
 
-   spin_lock(&memstick_host_lock);
-   idr_remove(&memstick_host_idr, host->id);
-   spin_unlock(&memstick_host_lock);
+   idr_put_index(&memstick_host_idr, &memstick_host_lock, host->id);
device_del(&host->dev);
 }
 EXPORT_SYMBOL(memstick_remove_host);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 11/17] Update the rtsx multifunction driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/mfd/rtsx_pcr.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index a66540a49079..8ddefb8c5e64 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -1191,15 +1191,10 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
}
handle->pcr = pcr;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&rtsx_pci_lock);
-   ret = idr_alloc(&rtsx_pci_idr, pcr, 0, 0, GFP_NOWAIT);
-   if (ret >= 0)
-   pcr->id = ret;
-   spin_unlock(&rtsx_pci_lock);
-   idr_preload_end();
+   ret = idr_get_index(&rtsx_pci_idr, &rtsc_pci_lock, pcr);
if (ret < 0)
goto free_handle;
+   pcr->id = ret;
 
pcr->pci = pcidev;
dev_set_drvdata(&pcidev->dev, handle);
@@ -1311,9 +1306,7 @@ static void rtsx_pci_remove(struct pci_dev *pcidev)
pci_release_regions(pcidev);
pci_disable_device(pcidev);
 
-   spin_lock(&rtsx_pci_lock);
-   idr_remove(&rtsx_pci_idr, pcr->id);
-   spin_unlock(&rtsx_pci_lock);
+   idr_put_index(&rtsx_pci_idr, &rtsx_pci_lock, pcr->id);
 
kfree(pcr->slots);
kfree(pcr);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 10/17] Update the DCA DMA driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/dca/dca-sysfs.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/dca/dca-sysfs.c b/drivers/dca/dca-sysfs.c
index 126cf295b198..8930707df295 100644
--- a/drivers/dca/dca-sysfs.c
+++ b/drivers/dca/dca-sysfs.c
@@ -55,23 +55,14 @@ int dca_sysfs_add_provider(struct dca_provider *dca, struct 
device *dev)
struct device *cd;
int ret;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&dca_idr_lock);
-
-   ret = idr_alloc(&dca_idr, dca, 0, 0, GFP_NOWAIT);
-   if (ret >= 0)
-   dca->id = ret;
-
-   spin_unlock(&dca_idr_lock);
-   idr_preload_end();
+   ret = idr_get_index(&dca_idr, &dca_idr_lock, dca);
if (ret < 0)
return ret;
+   dca->id = ret;
 
cd = device_create(dca_class, dev, MKDEV(0, 0), NULL, "dca%d", dca->id);
if (IS_ERR(cd)) {
-   spin_lock(&dca_idr_lock);
-   idr_remove(&dca_idr, dca->id);
-   spin_unlock(&dca_idr_lock);
+   idr_put_index(&dca_idr, &dca_idr_lock, dca->id);
return PTR_ERR(cd);
}
dca->cd = cd;
@@ -82,9 +73,7 @@ void dca_sysfs_remove_provider(struct dca_provider *dca)
 {
device_unregister(dca->cd);
dca->cd = NULL;
-   spin_lock(&dca_idr_lock);
-   idr_remove(&dca_idr, dca->id);
-   spin_unlock(&dca_idr_lock);
+   idr_put_index(&dca_idr, &dca_idr_lock, dca->id);
 }
 
 int __init dca_sysfs_init(void)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 12/17] Update the TI Flash Media driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/misc/tifm_core.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/tifm_core.c b/drivers/misc/tifm_core.c
index a511b2a713b3..46385f828a8f 100644
--- a/drivers/misc/tifm_core.c
+++ b/drivers/misc/tifm_core.c
@@ -198,22 +198,15 @@ int tifm_add_adapter(struct tifm_adapter *fm)
 {
int rc;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&tifm_adapter_lock);
-   rc = idr_alloc(&tifm_adapter_idr, fm, 0, 0, GFP_NOWAIT);
-   if (rc >= 0)
-   fm->id = rc;
-   spin_unlock(&tifm_adapter_lock);
-   idr_preload_end();
+   rc = idr_get_index(&tifm_adapter_idr, &tifm_adapter_lock, fm);
if (rc < 0)
return rc;
+   fm->id = rc;
 
dev_set_name(&fm->dev, "tifm%u", fm->id);
rc = device_add(&fm->dev);
if (rc) {
-   spin_lock(&tifm_adapter_lock);
-   idr_remove(&tifm_adapter_idr, fm->id);
-   spin_unlock(&tifm_adapter_lock);
+   idr_put_index(&tifm_adapter_idr, &tifm_adapter_lock, fm->id);
}
 
return rc;
@@ -230,9 +223,7 @@ void tifm_remove_adapter(struct tifm_adapter *fm)
device_unregister(&fm->sockets[cnt]->dev);
}
 
-   spin_lock(&tifm_adapter_lock);
-   idr_remove(&tifm_adapter_idr, fm->id);
-   spin_unlock(&tifm_adapter_lock);
+   idr_put_index(&tifm_adapter_idr, &tifm_adapter_lock, fm->id);
device_del(&fm->dev);
 }
 EXPORT_SYMBOL(tifm_remove_adapter);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 08/17] Update the mmc driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/mmc/core/host.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 99a9c9011c50..5aa2330f074c 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -40,9 +40,8 @@ static DEFINE_SPINLOCK(mmc_host_lock);
 static void mmc_host_classdev_release(struct device *dev)
 {
struct mmc_host *host = cls_dev_to_mmc_host(dev);
-   spin_lock(&mmc_host_lock);
-   idr_remove(&mmc_host_idr, host->index);
-   spin_unlock(&mmc_host_lock);
+
+   idr_put_index(&mmc_host_idr, &mmc_host_lock, host->index);
kfree(host);
 }
 
@@ -559,17 +558,12 @@ struct mmc_host *mmc_alloc_host(int extra, struct device 
*dev)
 
/* scanning will be enabled when we're ready */
host->rescan_disable = 1;
-   idr_preload(GFP_KERNEL);
-   spin_lock(&mmc_host_lock);
-   err = idr_alloc(&mmc_host_idr, host, 0, 0, GFP_NOWAIT);
-   if (err >= 0)
-   host->index = err;
-   spin_unlock(&mmc_host_lock);
-   idr_preload_end();
+   err = idr_get_index(&mmc_host_idr, &mmc_host_lock, host);
if (err < 0) {
kfree(host);
return NULL;
}
+   host->index = err;
 
dev_set_name(&host->class_dev, "mmc%d", host->index);
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 14/17] Update the rsxx flash adapter driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 15/17] Update the NVMe SSD driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 13/17] Update the SCSI disk driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/scsi/sd.c | 20 
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3b2fcb4fada0..60b2ad918208 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:
@@ -3063,10 +3053,8 @@ 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 09/17] Update the virtgpu driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/gpu/drm/virtio/virtgpu_vq.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c 
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 1698669f4185..380947dad306 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -41,21 +41,14 @@
 void virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
uint32_t *resid)
 {
-   int handle;
-
-   idr_preload(GFP_KERNEL);
-   spin_lock(&vgdev->resource_idr_lock);
-   handle = idr_alloc(&vgdev->resource_idr, NULL, 1, 0, GFP_NOWAIT);
-   spin_unlock(&vgdev->resource_idr_lock);
-   idr_preload_end();
-   *resid = handle;
+   *resid = idr_get_index_in_range(&vgdev->resource_idr,
+   &vgdev->resource_idr_lock,
+   NULL, 1, 0);
 }
 
 void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t id)
 {
-   spin_lock(&vgdev->resource_idr_lock);
-   idr_remove(&vgdev->resource_idr, id);
-   spin_unlock(&vgdev->resource_idr_lock);
+   idr_put_index(&vgdev->resource_idr, &vgdev->resource_idr_lock, id);
 }
 
 void virtio_gpu_ctrl_ack(struct virtqueue *vq)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 03/17] Update the st driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/scsi/st.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index b37b9b00c4b4..51e1ce721d9f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4265,11 +4265,8 @@ static int st_probe(struct device *dev)
tpnt->blksize_changed = 0;
mutex_init(&tpnt->lock);
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&st_index_lock);
-   error = idr_alloc(&st_index_idr, tpnt, 0, ST_MAX_TAPES + 1, GFP_NOWAIT);
-   spin_unlock(&st_index_lock);
-   idr_preload_end();
+   error = idr_get_index_in_range(&st_index_idr, &st_index_lock, tpnt,
+  0, ST_MAX_TAPES + 1);
if (error < 0) {
pr_warn("st: idr allocation failed: %d\n", error);
goto out_put_queue;
@@ -4303,9 +4300,7 @@ out_remove_devs:
remove_cdevs(tpnt);
kfree(tpnt->stats);
 out_idr_remove:
-   spin_lock(&st_index_lock);
-   idr_remove(&st_index_idr, tpnt->index);
-   spin_unlock(&st_index_lock);
+   idr_put_index(&st_index_idr, &st_index_lock, tpnt->index);
 out_put_queue:
blk_put_queue(disk->queue);
 out_put_disk:
@@ -4330,9 +4325,7 @@ static int st_remove(struct device *dev)
mutex_lock(&st_ref_mutex);
kref_put(&tpnt->kref, scsi_tape_release);
mutex_unlock(&st_ref_mutex);
-   spin_lock(&st_index_lock);
-   idr_remove(&st_index_idr, index);
-   spin_unlock(&st_index_lock);
+   idr_put_index(&st_index_idr, &st_index_lock, index);
return 0;
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 16/17] Update the Micron PCIe SSD driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 17/17] Update the ARM soc base driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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..cf70c3246123 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, &soc_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, &soc_lock, soc_dev->soc_dev_num);
 
device_unregister(&soc_dev->dev);
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 05/17] Update the md driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/md/dm.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f331d888e7f5..53d6895eb13d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2202,9 +2202,7 @@ static int dm_any_congested(void *congested_data, int 
bdi_bits)
  *---*/
 static void free_minor(int minor)
 {
-   spin_lock(&_minor_lock);
-   idr_remove(&_minor_idr, minor);
-   spin_unlock(&_minor_lock);
+   idr_put_index(&_minor_idr, &_minor_lock, minor);
 }
 
 /*
@@ -2217,13 +2215,8 @@ static int specific_minor(int minor)
if (minor >= (1 << MINORBITS))
return -EINVAL;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&_minor_lock);
-
-   r = idr_alloc(&_minor_idr, MINOR_ALLOCED, minor, minor + 1, GFP_NOWAIT);
-
-   spin_unlock(&_minor_lock);
-   idr_preload_end();
+   r = idr_get_index_in_range(&_minor_idr, &_minor_lock, MINOR_ALLOCED,
+  minor, minor + 1);
if (r < 0)
return r == -ENOSPC ? -EBUSY : r;
return 0;
@@ -2233,13 +2226,8 @@ static int next_free_minor(int *minor)
 {
int r;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&_minor_lock);
-
-   r = idr_alloc(&_minor_idr, MINOR_ALLOCED, 0, 1 << MINORBITS, 
GFP_NOWAIT);
-
-   spin_unlock(&_minor_lock);
-   idr_preload_end();
+   r = idr_get_index_in_range(&_minor_idr, &_minor_lock, MINOR_ALLOCED,
+  0, 1 << MINORBITS);
if (r < 0)
return r;
*minor = r;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 00/17] Create and use ida and idr helper routines [RESEND]

[Lee Duncan]

[Apologies if you see this twice. I had to resend it.]

The idr index management library supplies two sets of routines
for managing monotonically increasing index numbers. The "ida"
set of routines manage allocating and freeing simple index
numbers. The "idr" set of routines add the ability to save
an arbitrary pointer with each index.

Both sets of routines are used throughout the kernel, and it
was noted that many of them use the same or similar calling
sequences, making a helper function a useful addition.

This set of patches adds some helper functions, defined
as inline in . In addition, any of the clients
of these idr library functions that could benefit from
using these helper functions where modified to use them.

In addition to cleaning up the code in the clients of these
functions, the SCSI hosts module, which used to use a simple
atomic integer for index management is converted to using
the idr set of routines to manage its index values as
well as to simplify and speed up host number to instance
lookups.

I have functionally tested the SCSI host indexing change,
and I have compile tested all of the other changes. The
maintainers of each driver are cc-ed on the patch series,
where available.

Note: I did not mark this patch series as "v2" since
the scope of the patch set has grown considerably
since my first submissions.

Summary: There is one patch that adds helper functions, 11
patches that use the new "idr" helper functions, and 5 that
use the new "ida" helper functions.

Lee Duncan (17):
   1. Add ida and idr helper routines.
   2. Update scsi hosts to use idr for host number mgmt
   3. Update the st driver to use idr helper functions.
   4. Update the ch driver to use idr helper functions.
   5. Update the md driver to use idr helper functions.
   6. Update the infiniband uverbs driver to use idr helper functions.
   7. Update the memstick driver to use idr helper functions.
   8. Update the mmc driver to use idr helper functions.
   9. Update the virtgpu driver to use idr helper functions.
  10. Update the DCA DMA driver to use idr helper functions.
  11. Update the rtsx multifunction driver to use idr helper functions.
  12. Update the TI Flash Media driver to use idr helper functions.
  13. Update the SCSI disk driver to use ida helper functions.
  14. Update the rsxx flash adapter driver to use ida helper functions.
  15. Update the NVMe SSD driver to use ida helper functions.
  16. Update the Micron PCIe SSD driver to use ida helper functions.
  17. Update the ARM soc base driver to use 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/dca/dca-sysfs.c  |  19 ++-
 drivers/gpu/drm/virtio/virtgpu_vq.c  |  15 ++
 drivers/infiniband/core/uverbs_cmd.c |  12 +
 drivers/md/dm.c  |  22 ++--
 drivers/memstick/core/memstick.c |  19 ++-
 drivers/mfd/rtsx_pcr.c   |  13 ++---
 drivers/misc/tifm_core.c |  17 ++
 drivers/mmc/core/host.c  |  14 ++---
 drivers/scsi/ch.c|  14 ++---
 drivers/scsi/hosts.c |  59 ++--
 drivers/scsi/sd.c|  20 ++-
 drivers/scsi/st.c|  15 ++
 include/linux/idr.h  | 102 +++
 17 files changed, 182 insertions(+), 228 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 01/17] Add ida and idr helper routines.

[Lee Duncan]

Clients of the ida and idr index-management routines
tend to use the same calling sequences much of the time,
so this change adds helper functions for allocating and
releasing indexes of either flavor, i.e. with or
without pointer management.

Inline functions added for idr:
  idr_get_index_in_range
  idr_get_index (in range 0,0)
  idr_put_index
And for ida:
  ida_get_index
  ida_put_index

Signed-off-by: Lee Duncan 
---
 include/linux/idr.h | 102 
 1 file changed, 102 insertions(+)

diff --git a/include/linux/idr.h b/include/linux/idr.h
index 013fd9bc4cb6..341c4f2d9874 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -16,6 +16,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /*
  * We want shallower trees and thus more bits covered at each layer.  8
@@ -183,4 +185,104 @@ 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
+ * @idaidr 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
+ * @idaidr 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);
+}
+
+/**
+ * idr_get_index_in_range - allocate a new index, with locking
+ * within a range
+ * @idr:   idr handle
+ * @lock:  spin lock handle protecting the index
+ * @ptr:   pointer to associate with allocated index
+ * @start: starting index (see idr_alloc)
+ * @end:   ending index (0 -> use default max)
+ *
+ * This is a helper routine meant to make the common
+ * calling sequence to allocate an idr index easier.
+ * It uses a spin lock, and allocates a positive index
+ * in the range of [start,end), returning a negative
+ * errno on failure, else 0.
+ */
+static inline int idr_get_index_in_range(struct idr *idr, spinlock_t *lock,
+void *ptr, int start, int end)
+{
+   int ret;
+
+   idr_preload(GFP_KERNEL);
+   spin_lock(lock);
+   ret = idr_alloc(idr, ptr, start, end, GFP_NOWAIT);
+   spin_unlock(lock);
+   idr_preload_end();
+
+   return ret;
+}
+
+/**
+ * idr_get_index - allocate new index, with locking, using the
+ *default range (zero to max-1)
+ * @idr:   idr handle
+ * @lock:  spin lock handle protecting the index
+ * @ptr:   pointer to associate with allocated index
+ *
+ * Simple wrapper around idr_get_index_in_range() w/ @start and
+ * @end of 0, since this is a common case
+ */
+static inline int idr_get_index(struct idr *idr, spinlock_t *lock, void *ptr)
+{
+   return idr_get_index_in_range(idr, lock, ptr, 0, 0);
+}
+
+/**
+ * idr_put_index - free an allocated idr index value
+ * @idridr handle
+ * @lock   spinlock handle protecting this index
+ * @id the value of the allocated index
+ *
+ * A helper function that goes with @idr_get_index, which safely
+ * frees a previously-allocated index value.
+ */
+static inline void idr_put_index(struct idr *idr, spinlock_t *lock, int id)
+{
+   spin_lock(lock);
+   idr_remove(idr, id);
+   spin_unlock(lock);
+}
+
 #endif /* __IDR_H__ */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 00/17] Create and use ida and idr helper routines

[Lee Duncan]

The idr index management library supplies two sets of routines
for managing monotonically increasing index numbers. The "ida"
set of routines manage allocating and freeing simple index
numbers. The "idr" set of routines add the ability to save
an arbitrary pointer with each index.

Both sets of routines are used throughout the kernel, and it
was noted that many of them use the same or similar calling
sequences, making a helper function a useful addition.

This set of patches adds some helper functions, defined
as inline in . In addition, any of the clients
of these idr library functions that could benefit from
using these helper functions where modified to use them.

In addition to cleaning up the code in the clients of these
functions, the SCSI hosts module, which used to use a simple
atomic integer for index management is converted to using
the idr set of routines to manage its index values as
well as to simplify and speed up host number to instance
lookups.

I have functionally tested the SCSI host indexing change,
and I have compile tested all of the other changes. The
maintainers of each driver are cc-ed on the patch series,
where available.

Note: I did not mark this patch series as "v2" since
the scope of the patch set has grown considerably
since my first submissions.

Summary: There is one patch that adds helper functions, 11
patches that use the new "idr" helper functions, and 5 that
use the new "ida" helper functions.

Lee Duncan (17):
   1. Add ida and idr helper routines.
   2. Update scsi hosts to use idr for host number mgmt
   3. Update the st driver to use idr helper functions.
   4. Update the ch driver to use idr helper functions.
   5. Update the md driver to use idr helper functions.
   6. Update the infiniband uverbs driver to use idr helper functions.
   7. Update the memstick driver to use idr helper functions.
   8. Update the mmc driver to use idr helper functions.
   9. Update the virtgpu driver to use idr helper functions.
  10. Update the DCA DMA driver to use idr helper functions.
  11. Update the rtsx multifunction driver to use idr helper functions.
  12. Update the TI Flash Media driver to use idr helper functions.
  13. Update the SCSI disk driver to use ida helper functions.
  14. Update the rsxx flash adapter driver to use ida helper functions.
  15. Update the NVMe SSD driver to use ida helper functions.
  16. Update the Micron PCIe SSD driver to use ida helper functions.
  17. Update the ARM soc base driver to use 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/dca/dca-sysfs.c  |  19 ++-
 drivers/gpu/drm/virtio/virtgpu_vq.c  |  15 ++
 drivers/infiniband/core/uverbs_cmd.c |  12 +
 drivers/md/dm.c  |  22 ++--
 drivers/memstick/core/memstick.c |  19 ++-
 drivers/mfd/rtsx_pcr.c   |  13 ++---
 drivers/misc/tifm_core.c |  17 ++
 drivers/mmc/core/host.c  |  14 ++---
 drivers/scsi/ch.c|  14 ++---
 drivers/scsi/hosts.c |  59 ++--
 drivers/scsi/sd.c|  20 ++-
 drivers/scsi/st.c|  15 ++
 include/linux/idr.h  | 102 +++
 17 files changed, 182 insertions(+), 228 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 03/17] Update the st driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/scsi/st.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index b37b9b00c4b4..51e1ce721d9f 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4265,11 +4265,8 @@ static int st_probe(struct device *dev)
tpnt->blksize_changed = 0;
mutex_init(&tpnt->lock);
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&st_index_lock);
-   error = idr_alloc(&st_index_idr, tpnt, 0, ST_MAX_TAPES + 1, GFP_NOWAIT);
-   spin_unlock(&st_index_lock);
-   idr_preload_end();
+   error = idr_get_index_in_range(&st_index_idr, &st_index_lock, tpnt,
+  0, ST_MAX_TAPES + 1);
if (error < 0) {
pr_warn("st: idr allocation failed: %d\n", error);
goto out_put_queue;
@@ -4303,9 +4300,7 @@ out_remove_devs:
remove_cdevs(tpnt);
kfree(tpnt->stats);
 out_idr_remove:
-   spin_lock(&st_index_lock);
-   idr_remove(&st_index_idr, tpnt->index);
-   spin_unlock(&st_index_lock);
+   idr_put_index(&st_index_idr, &st_index_lock, tpnt->index);
 out_put_queue:
blk_put_queue(disk->queue);
 out_put_disk:
@@ -4330,9 +4325,7 @@ static int st_remove(struct device *dev)
mutex_lock(&st_ref_mutex);
kref_put(&tpnt->kref, scsi_tape_release);
mutex_unlock(&st_ref_mutex);
-   spin_lock(&st_index_lock);
-   idr_remove(&st_index_idr, index);
-   spin_unlock(&st_index_lock);
+   idr_put_index(&st_index_idr, &st_index_lock, index);
return 0;
 }
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 02/17] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

Each Scsi_Host instance gets a host number starting
at 0, but this was implemented with an atomic integer,
and rollover wasn't considered. Another problem with
this design is that scsi host numbers used by iscsi
are never reused, thereby making rollover more likely.
This patch converts Scsi_Host instances to use idr
to manage their instance numbers and to simplify
instance number to pointer lookups.

This also means that host instance numbers will be
reused, when available.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/hosts.c | 59 
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..61201bc03b98 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include 
 #include 
 #include 
@@ -42,8 +42,6 @@
 #include "scsi_logging.h"
 
 
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);/* host_no for next new 
host */
-
 
 static void scsi_host_cls_release(struct device *dev)
 {
@@ -304,6 +302,19 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct 
device *dev,
 }
 EXPORT_SYMBOL(scsi_add_host_with_dma);
 
+static DEFINE_SPINLOCK(host_index_lock);
+static DEFINE_IDR(host_index_idr);
+
+static inline int host_get_index(void *ptr)
+{
+   return idr_get_index(&host_index_idr, &host_index_lock, ptr);
+}
+
+static inline void host_put_index(int index)
+{
+   idr_put_index(&host_index_idr, &host_index_lock, index);
+}
+
 static void scsi_host_dev_release(struct device *dev)
 {
struct Scsi_Host *shost = dev_to_shost(dev);
@@ -337,6 +348,8 @@ static void scsi_host_dev_release(struct device *dev)
 
kfree(shost->shost_data);
 
+   host_put_index(shost->host_no);
+
if (parent)
put_device(parent);
kfree(shost);
@@ -370,6 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 {
struct Scsi_Host *shost;
gfp_t gfp_mask = GFP_KERNEL;
+   int index;
 
if (sht->unchecked_isa_dma && privsize)
gfp_mask |= __GFP_DMA;
@@ -388,11 +402,11 @@ struct Scsi_Host *scsi_host_alloc(struct 
scsi_host_template *sht, int privsize)
init_waitqueue_head(&shost->host_wait);
mutex_init(&shost->scan_mutex);
 
-   /*
-* subtract one because we increment first then return, but we need to
-* know what the next host number was before increment
-*/
-   shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+   index = host_get_index(shost);
+   if (index < 0)
+   goto fail_kfree;
+   shost->host_no = index;
+
shost->dma_channel = 0xff;
 
/* These three are default values which can be overridden */
@@ -477,7 +491,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
shost_printk(KERN_WARNING, shost,
"error handler thread failed to spawn, error = %ld\n",
PTR_ERR(shost->ehandler));
-   goto fail_kfree;
+   goto fail_idr_remove;
}
 
shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +507,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template 
*sht, int privsize)
 
  fail_kthread:
kthread_stop(shost->ehandler);
+ fail_idr_remove:
+   host_put_index(shost->host_no);
  fail_kfree:
kfree(shost);
return NULL;
@@ -522,38 +538,16 @@ void scsi_unregister(struct Scsi_Host *shost)
 }
 EXPORT_SYMBOL(scsi_unregister);
 
-static int __scsi_host_match(struct device *dev, const void *data)
-{
-   struct Scsi_Host *p;
-   const unsigned short *hostnum = data;
-
-   p = class_to_shost(dev);
-   return p->host_no == *hostnum;
-}
-
 /**
  * scsi_host_lookup - get a reference to a Scsi_Host by host no
  * @hostnum:   host number to locate
  *
  * Return value:
  * A pointer to located Scsi_Host or NULL.
- *
- * The caller must do a scsi_host_put() to drop the reference
- * that scsi_host_get() took. The put_device() below dropped
- * the reference from class_find_device().
  **/
 struct Scsi_Host *scsi_host_lookup(unsigned short hostnum)
 {
-   struct device *cdev;
-   struct Scsi_Host *shost = NULL;
-
-   cdev = class_find_device(&shost_class, NULL, &hostnum,
-__scsi_host_match);
-   if (cdev) {
-   shost = scsi_host_get(class_to_shost(cdev));
-   put_device(cdev);
-   }
-   return shost;
+   return idr_find(&host_index_idr, hostnum);
 }
 EXPORT_SYMBOL(scsi_host_lookup);
 
@@ -588,6 +582,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
class_unregister(&shost_class);
+   idr_destroy(&am

[PATCH 06/17] Update the infiniband uverbs driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/infiniband/core/uverbs_cmd.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c 
b/drivers/infiniband/core/uverbs_cmd.c
index bbb02ffe87df..1e5b2a66a501 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -120,24 +120,16 @@ static int idr_add_uobj(struct idr *idr, struct 
ib_uobject *uobj)
 {
int ret;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&ib_uverbs_idr_lock);
-
-   ret = idr_alloc(idr, uobj, 0, 0, GFP_NOWAIT);
+   ret = idr_get_index(idr, &ib_uverbs_idr_lock, uobj);
if (ret >= 0)
uobj->id = ret;
 
-   spin_unlock(&ib_uverbs_idr_lock);
-   idr_preload_end();
-
return ret < 0 ? ret : 0;
 }
 
 void idr_remove_uobj(struct idr *idr, struct ib_uobject *uobj)
 {
-   spin_lock(&ib_uverbs_idr_lock);
-   idr_remove(idr, uobj->id);
-   spin_unlock(&ib_uverbs_idr_lock);
+   idr_put_index(idr, &ib_uverbs_idr_lock, uobj->id);
 }
 
 static struct ib_uobject *__idr_get_uobj(struct idr *idr, int id,
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 04/17] Update the ch driver to use idr helper functions.

[Lee Duncan]

Note: when allocating an index, in the error case,
where the just-allocated index has to be released,
the required locking around the index removal was
not present, so this conversion has the side effect
of adding locking for that error condition. This
should close a possible race condition that could
have resulted in duplicate index allocation.

Signed-off-by: Lee Duncan 
---
 drivers/scsi/ch.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index dad959fcf6d8..2edf1f8883f9 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -909,12 +909,8 @@ static int ch_probe(struct device *dev)
if (NULL == ch)
return -ENOMEM;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&ch_index_lock);
-   ret = idr_alloc(&ch_index_idr, ch, 0, CH_MAX_DEVS + 1, GFP_NOWAIT);
-   spin_unlock(&ch_index_lock);
-   idr_preload_end();
-
+   ret = idr_get_index_in_range(&ch_index_idr, &ch_index_lock, ch,
+0, CH_MAX_DEVS + 1);
if (ret < 0) {
if (ret == -ENOSPC)
ret = -ENODEV;
@@ -945,7 +941,7 @@ static int ch_probe(struct device *dev)
 
return 0;
 remove_idr:
-   idr_remove(&ch_index_idr, ch->minor);
+   idr_put_index(&ch_index_idr, &ch_index_lock, ch->minor);
 free_ch:
kfree(ch);
return ret;
@@ -955,9 +951,7 @@ static int ch_remove(struct device *dev)
 {
scsi_changer *ch = dev_get_drvdata(dev);
 
-   spin_lock(&ch_index_lock);
-   idr_remove(&ch_index_idr, ch->minor);
-   spin_unlock(&ch_index_lock);
+   idr_put_index(&ch_index_idr, &ch_index_lock, ch->minor);
 
device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor));
kfree(ch->dt);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 11/17] Update the rtsx multifunction driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/mfd/rtsx_pcr.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
index a66540a49079..8ddefb8c5e64 100644
--- a/drivers/mfd/rtsx_pcr.c
+++ b/drivers/mfd/rtsx_pcr.c
@@ -1191,15 +1191,10 @@ static int rtsx_pci_probe(struct pci_dev *pcidev,
}
handle->pcr = pcr;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&rtsx_pci_lock);
-   ret = idr_alloc(&rtsx_pci_idr, pcr, 0, 0, GFP_NOWAIT);
-   if (ret >= 0)
-   pcr->id = ret;
-   spin_unlock(&rtsx_pci_lock);
-   idr_preload_end();
+   ret = idr_get_index(&rtsx_pci_idr, &rtsc_pci_lock, pcr);
if (ret < 0)
goto free_handle;
+   pcr->id = ret;
 
pcr->pci = pcidev;
dev_set_drvdata(&pcidev->dev, handle);
@@ -1311,9 +1306,7 @@ static void rtsx_pci_remove(struct pci_dev *pcidev)
pci_release_regions(pcidev);
pci_disable_device(pcidev);
 
-   spin_lock(&rtsx_pci_lock);
-   idr_remove(&rtsx_pci_idr, pcr->id);
-   spin_unlock(&rtsx_pci_lock);
+   idr_put_index(&rtsx_pci_idr, &rtsx_pci_lock, pcr->id);
 
kfree(pcr->slots);
kfree(pcr);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 10/17] Update the DCA DMA driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/dca/dca-sysfs.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/dca/dca-sysfs.c b/drivers/dca/dca-sysfs.c
index 126cf295b198..8930707df295 100644
--- a/drivers/dca/dca-sysfs.c
+++ b/drivers/dca/dca-sysfs.c
@@ -55,23 +55,14 @@ int dca_sysfs_add_provider(struct dca_provider *dca, struct 
device *dev)
struct device *cd;
int ret;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&dca_idr_lock);
-
-   ret = idr_alloc(&dca_idr, dca, 0, 0, GFP_NOWAIT);
-   if (ret >= 0)
-   dca->id = ret;
-
-   spin_unlock(&dca_idr_lock);
-   idr_preload_end();
+   ret = idr_get_index(&dca_idr, &dca_idr_lock, dca);
if (ret < 0)
return ret;
+   dca->id = ret;
 
cd = device_create(dca_class, dev, MKDEV(0, 0), NULL, "dca%d", dca->id);
if (IS_ERR(cd)) {
-   spin_lock(&dca_idr_lock);
-   idr_remove(&dca_idr, dca->id);
-   spin_unlock(&dca_idr_lock);
+   idr_put_index(&dca_idr, &dca_idr_lock, dca->id);
return PTR_ERR(cd);
}
dca->cd = cd;
@@ -82,9 +73,7 @@ void dca_sysfs_remove_provider(struct dca_provider *dca)
 {
device_unregister(dca->cd);
dca->cd = NULL;
-   spin_lock(&dca_idr_lock);
-   idr_remove(&dca_idr, dca->id);
-   spin_unlock(&dca_idr_lock);
+   idr_put_index(&dca_idr, &dca_idr_lock, dca->id);
 }
 
 int __init dca_sysfs_init(void)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 13/17] Update the SCSI disk driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/scsi/sd.c | 20 
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3b2fcb4fada0..60b2ad918208 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:
@@ -3063,10 +3053,8 @@ 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 14/17] Update the rsxx flash adapter driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 12/17] Update the TI Flash Media driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/misc/tifm_core.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/tifm_core.c b/drivers/misc/tifm_core.c
index a511b2a713b3..46385f828a8f 100644
--- a/drivers/misc/tifm_core.c
+++ b/drivers/misc/tifm_core.c
@@ -198,22 +198,15 @@ int tifm_add_adapter(struct tifm_adapter *fm)
 {
int rc;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&tifm_adapter_lock);
-   rc = idr_alloc(&tifm_adapter_idr, fm, 0, 0, GFP_NOWAIT);
-   if (rc >= 0)
-   fm->id = rc;
-   spin_unlock(&tifm_adapter_lock);
-   idr_preload_end();
+   rc = idr_get_index(&tifm_adapter_idr, &tifm_adapter_lock, fm);
if (rc < 0)
return rc;
+   fm->id = rc;
 
dev_set_name(&fm->dev, "tifm%u", fm->id);
rc = device_add(&fm->dev);
if (rc) {
-   spin_lock(&tifm_adapter_lock);
-   idr_remove(&tifm_adapter_idr, fm->id);
-   spin_unlock(&tifm_adapter_lock);
+   idr_put_index(&tifm_adapter_idr, &tifm_adapter_lock, fm->id);
}
 
return rc;
@@ -230,9 +223,7 @@ void tifm_remove_adapter(struct tifm_adapter *fm)
device_unregister(&fm->sockets[cnt]->dev);
}
 
-   spin_lock(&tifm_adapter_lock);
-   idr_remove(&tifm_adapter_idr, fm->id);
-   spin_unlock(&tifm_adapter_lock);
+   idr_put_index(&tifm_adapter_idr, &tifm_adapter_lock, fm->id);
device_del(&fm->dev);
 }
 EXPORT_SYMBOL(tifm_remove_adapter);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 07/17] Update the memstick driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/memstick/core/memstick.c | 19 ---
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index a0547dbf9806..8f40a3d5108b 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -512,25 +512,16 @@ int memstick_add_host(struct memstick_host *host)
 {
int rc;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&memstick_host_lock);
-
-   rc = idr_alloc(&memstick_host_idr, host, 0, 0, GFP_NOWAIT);
-   if (rc >= 0)
-   host->id = rc;
-
-   spin_unlock(&memstick_host_lock);
-   idr_preload_end();
+   rc = idr_get_index(&memstick_host_idr, &memstick_host_lock, host);
if (rc < 0)
return rc;
+   host->id = rc;
 
dev_set_name(&host->dev, "memstick%u", host->id);
 
rc = device_add(&host->dev);
if (rc) {
-   spin_lock(&memstick_host_lock);
-   idr_remove(&memstick_host_idr, host->id);
-   spin_unlock(&memstick_host_lock);
+   idr_put_index(&memstick_host_idr, &memstick_host_lock, 
host->id);
return rc;
}
 
@@ -554,9 +545,7 @@ void memstick_remove_host(struct memstick_host *host)
host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF);
mutex_unlock(&host->lock);
 
-   spin_lock(&memstick_host_lock);
-   idr_remove(&memstick_host_idr, host->id);
-   spin_unlock(&memstick_host_lock);
+   idr_put_index(&memstick_host_idr, &memstick_host_lock, host->id);
device_del(&host->dev);
 }
 EXPORT_SYMBOL(memstick_remove_host);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 17/17] Update the ARM soc base driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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..cf70c3246123 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, &soc_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, &soc_lock, soc_dev->soc_dev_num);
 
device_unregister(&soc_dev->dev);
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 16/17] Update the Micron PCIe SSD driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 05/17] Update the md driver to use idr helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 drivers/md/dm.c | 22 +-
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f331d888e7f5..53d6895eb13d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2202,9 +2202,7 @@ static int dm_any_congested(void *congested_data, int 
bdi_bits)
  *---*/
 static void free_minor(int minor)
 {
-   spin_lock(&_minor_lock);
-   idr_remove(&_minor_idr, minor);
-   spin_unlock(&_minor_lock);
+   idr_put_index(&_minor_idr, &_minor_lock, minor);
 }
 
 /*
@@ -2217,13 +2215,8 @@ static int specific_minor(int minor)
if (minor >= (1 << MINORBITS))
return -EINVAL;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&_minor_lock);
-
-   r = idr_alloc(&_minor_idr, MINOR_ALLOCED, minor, minor + 1, GFP_NOWAIT);
-
-   spin_unlock(&_minor_lock);
-   idr_preload_end();
+   r = idr_get_index_in_range(&_minor_idr, &_minor_lock, MINOR_ALLOCED,
+  minor, minor + 1);
if (r < 0)
return r == -ENOSPC ? -EBUSY : r;
return 0;
@@ -2233,13 +2226,8 @@ static int next_free_minor(int *minor)
 {
int r;
 
-   idr_preload(GFP_KERNEL);
-   spin_lock(&_minor_lock);
-
-   r = idr_alloc(&_minor_idr, MINOR_ALLOCED, 0, 1 << MINORBITS, 
GFP_NOWAIT);
-
-   spin_unlock(&_minor_lock);
-   idr_preload_end();
+   r = idr_get_index_in_range(&_minor_idr, &_minor_lock, MINOR_ALLOCED,
+  0, 1 << MINORBITS);
if (r < 0)
return r;
*minor = r;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 15/17] Update the NVMe SSD driver to use ida helper functions.

[Lee Duncan]

Signed-off-by: Lee Duncan 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/