RE: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock

2016-11-10 Thread Kashyap Desai 
> -Original Message-
> From: i...@itu.dk [mailto:i...@itu.dk]
> Sent: Monday, October 17, 2016 1:00 PM
> To: Jiri Kosina
> Cc: Kashyap Desai; Sumit Saxena; Uday Lingala; James E.J. Bottomley;
Martin K.
> Petersen; megaraidlinux@avagotech.com; linux-scsi@vger.kernel.org;
Iago
> Abal
> Subject: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list
lock to
> avoid double lock
>
> From: Iago Abal 
>
> The EBA code analyzer (https://github.com/models-team/eba) reported the
> following double lock:
>
> 1. In function `megaraid_reset_handler' at 2571;
> 2. take `&adapter->pend_list_lock' for the first time at 2602:
>
>// FIRST
>spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>
> 3. enter the `list_for_each_entry_safe' loop at 2603;
> 4. call `megaraid_mbox_mm_done' at 2616;
> 5. call `megaraid_mbox_runpendq' at 3782;
> 6. take `&adapter->pend_list_lock' for the second time at 1892:
>
>// SECOND: DOUBLE LOCK !!!
>spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>
> From my shallow understanding of the code (so please review carefully),
I think
> that it is not necessary to hold `PENDING_LIST_LOCK(adapter)' while
executing
> the body of the `list_for_each_entry_safe' loop. I assume this because
both
> `megaraid_mbox_mm_done' and `megaraid_dealloc_scb' are called from
> several places where, as far as I can tell, this lock is not hold. In
fact, as reported
> by EBA, at some point `megaraid_mbox_mm_done' will acquire this lock
again.
>
> Fixes: c005fb4fb2d2 ("[SCSI] megaraid_{mm,mbox}: fix a bug in reset
handler")
> Signed-off-by: Iago Abal 
> ---
>  drivers/scsi/megaraid/megaraid_mbox.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/megaraid/megaraid_mbox.c
> b/drivers/scsi/megaraid/megaraid_mbox.c
> index f0987f2..7f11898 100644
> --- a/drivers/scsi/megaraid/megaraid_mbox.c
> +++ b/drivers/scsi/megaraid/megaraid_mbox.c
> @@ -2603,6 +2603,7 @@ static DEF_SCSI_QCMD(megaraid_queue_command)
>   list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
>   list_del_init(&scb->list);  // from pending list
>
> + spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
>   if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
>   con_log(CL_ANN, (KERN_WARNING
>   "megaraid: IOCTL packet with %d[%d:%d] being
> reset\n", @@ -2630,6 +2631,7 @@ static
> DEF_SCSI_QCMD(megaraid_queue_command)
>
>   megaraid_dealloc_scb(adapter, scb);
>   }
> + spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>   }
>   spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);

Sorry for delay. We had internal discussion and confirm that it is safe to
remove mbox driver from mainline as this product is discontinued and we
are planning to post patch to remove megaraid mbox driver from mainline.

>
> --
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock

2016-10-20 Thread Kashyap Desai 
> -Original Message-
> From: i...@itu.dk [mailto:i...@itu.dk]
> Sent: Monday, October 17, 2016 1:00 PM
> To: Jiri Kosina
> Cc: Kashyap Desai; Sumit Saxena; Uday Lingala; James E.J. Bottomley;
Martin K.
> Petersen; megaraidlinux@avagotech.com; linux-scsi@vger.kernel.org;
Iago
> Abal
> Subject: [PATCH] Fix: scsi: megaraid: reduce the scope of pending-list
lock to
> avoid double lock
>
> From: Iago Abal 
>
> The EBA code analyzer (https://github.com/models-team/eba) reported the
> following double lock:
>
> 1. In function `megaraid_reset_handler' at 2571;
> 2. take `&adapter->pend_list_lock' for the first time at 2602:
>
>// FIRST
>spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>
> 3. enter the `list_for_each_entry_safe' loop at 2603;
> 4. call `megaraid_mbox_mm_done' at 2616;
> 5. call `megaraid_mbox_runpendq' at 3782;
> 6. take `&adapter->pend_list_lock' for the second time at 1892:
>
>// SECOND: DOUBLE LOCK !!!
>spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>
> From my shallow understanding of the code (so please review carefully),
I think
> that it is not necessary to hold `PENDING_LIST_LOCK(adapter)' while
executing
> the body of the `list_for_each_entry_safe' loop. I assume this because
both
> `megaraid_mbox_mm_done' and `megaraid_dealloc_scb' are called from
> several places where, as far as I can tell, this lock is not hold. In
fact, as reported
> by EBA, at some point `megaraid_mbox_mm_done' will acquire this lock
again.
>
> Fixes: c005fb4fb2d2 ("[SCSI] megaraid_{mm,mbox}: fix a bug in reset
handler")
> Signed-off-by: Iago Abal 
> ---
>  drivers/scsi/megaraid/megaraid_mbox.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/scsi/megaraid/megaraid_mbox.c
> b/drivers/scsi/megaraid/megaraid_mbox.c
> index f0987f2..7f11898 100644
> --- a/drivers/scsi/megaraid/megaraid_mbox.c
> +++ b/drivers/scsi/megaraid/megaraid_mbox.c
> @@ -2603,6 +2603,7 @@ static DEF_SCSI_QCMD(megaraid_queue_command)
>   list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
>   list_del_init(&scb->list);  // from pending list
>
> + spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
>   if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
>   con_log(CL_ANN, (KERN_WARNING
>   "megaraid: IOCTL packet with %d[%d:%d] being
> reset\n", @@ -2630,6 +2631,7 @@ static
> DEF_SCSI_QCMD(megaraid_queue_command)
>
>   megaraid_dealloc_scb(adapter, scb);
>   }
> + spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
>   }
>   spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);

Looks correct, but please note that MEGARAID_MAILBOX and MEGARAID_MM is
not supported by LSI/ Broadcom.   We will revert back to you shortly if we
can safely remove those two modules.

.

>
> --
> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Fix: scsi: megaraid: reduce the scope of pending-list lock to avoid double lock

2016-10-17 Thread iari 
From: Iago Abal 

The EBA code analyzer (https://github.com/models-team/eba) reported
the following double lock:

1. In function `megaraid_reset_handler' at 2571;
2. take `&adapter->pend_list_lock' for the first time at 2602:

   // FIRST
   spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);

3. enter the `list_for_each_entry_safe' loop at 2603;
4. call `megaraid_mbox_mm_done' at 2616;
5. call `megaraid_mbox_runpendq' at 3782;
6. take `&adapter->pend_list_lock' for the second time at 1892:

   // SECOND: DOUBLE LOCK !!!
   spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);

>From my shallow understanding of the code (so please review carefully), I think
that it is not necessary to hold `PENDING_LIST_LOCK(adapter)' while executing
the body of the `list_for_each_entry_safe' loop. I assume this because both
`megaraid_mbox_mm_done' and `megaraid_dealloc_scb' are called from several
places where, as far as I can tell, this lock is not hold. In fact, as reported
by EBA, at some point `megaraid_mbox_mm_done' will acquire this lock again.

Fixes: c005fb4fb2d2 ("[SCSI] megaraid_{mm,mbox}: fix a bug in reset handler")
Signed-off-by: Iago Abal 
---
 drivers/scsi/megaraid/megaraid_mbox.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/megaraid/megaraid_mbox.c 
b/drivers/scsi/megaraid/megaraid_mbox.c
index f0987f2..7f11898 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -2603,6 +2603,7 @@ static DEF_SCSI_QCMD(megaraid_queue_command)
list_for_each_entry_safe(scb, tmp, &adapter->pend_list, list) {
list_del_init(&scb->list);  // from pending list
 
+   spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
if (scb->sno >= MBOX_MAX_SCSI_CMDS) {
con_log(CL_ANN, (KERN_WARNING
"megaraid: IOCTL packet with %d[%d:%d] being reset\n",
@@ -2630,6 +2631,7 @@ static DEF_SCSI_QCMD(megaraid_queue_command)
 
megaraid_dealloc_scb(adapter, scb);
}
+   spin_lock_irqsave(PENDING_LIST_LOCK(adapter), flags);
}
spin_unlock_irqrestore(PENDING_LIST_LOCK(adapter), flags);
 
-- 
1.9.1

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