[PATCH resend 1/2] be2iscsi: Remove unnecessary synchronize_irq() before free_irq()

Lars-Peter Clausen

author

2016-03-04 10:15 UTC #0 lore raw

PATCH RESEND 1/2 be2iscsi: Remove unnecessary synchronize_irq() before free_irq()

Calling synchronize_irq() right before free_irq() is quite useless. On one hand the IRQ can easily fire again before free_irq() is entered, on the other hand free_irq() itself calls synchronize_irq() internally (in a race condition free way), before any state associated with the IRQ is freed.

Patch was generated using the following semantic patch:
// <smpl>
@@
expression irq;
@@
-synchronize_irq(irq);
free_irq(irq, ...);
// </smpl>

Signed-off-by Lars-Peter Clausen <[email protected]>

---
 drivers/scsi/be2iscsi/be_main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

drivers/scsi/be2iscsi/be_main.c +1 -4

index b51e726..e89a0f8d 100644
@@ -5303,15 +5303,12 @@ static void beiscsi_quiesce(struct beiscsi_hba *phba,
 	if (phba->msix_enabled) {
 		for (i = 0; i <= phba->num_cpus; i++) {
 			msix_vec = phba->msix_entries[i].vector;
-			synchronize_irq(msix_vec);
 			free_irq(msix_vec, &phwi_context->be_eq[i]);
 			kfree(phba->msi_name[i]);
 		}
 	} else
-		if (phba->pcidev->irq) {
-			synchronize_irq(phba->pcidev->irq);
+		if (phba->pcidev->irq)
 			free_irq(phba->pcidev->irq, phba);
-		}
 	pci_disable_msix(phba->pcidev);
 	cancel_delayed_work_sync(&phba->beiscsi_hw_check_task);

Signature · 1 line

2.1.4

Lars-Peter Clausen

author

2016-03-04 10:15 UTC #1 lore raw

PATCH RESEND 2/2 mpt3sas: Remove unnecessary synchronize_irq() before free_irq()

Calling synchronize_irq() right before free_irq() is quite useless. On one hand the IRQ can easily fire again before free_irq() is entered, on the other hand free_irq() itself calls synchronize_irq() internally (in a race condition free way), before any state associated with the IRQ is freed.

Patch was generated using the following semantic patch:
// <smpl>
@@
expression irq;
@@
-synchronize_irq(irq);
free_irq(irq, ...);
// </smpl>

Signed-off-by Lars-Peter Clausen <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 1 -
 1 file changed, 1 deletion(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +0 -1

index 5bbbbf2..e4db5fb 100644
@@ -1820,7 +1820,6 @@ _base_free_irq(struct MPT3SAS_ADAPTER *ioc)
 			irq_set_affinity_hint(reply_q->vector, NULL);
 			free_cpumask_var(reply_q->affinity_hint);
 		}
-		synchronize_irq(reply_q->vector);
 		free_irq(reply_q->vector, reply_q);
 		kfree(reply_q);
 	}

Signature · 1 line

2.1.4

Martin K. Petersen

Applied

2016-03-08 02:05 UTC #2 lore raw

Re: [PATCH resend 2/2] mpt3sas: Remove unnecessary synchronize_irq() before free_irq()

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Lars-Peter" == Lars-Peter Clausen <[email protected]> writes:

Lars-Peter> Calling synchronize_irq() right before free_irq() is quite Lars-Peter> useless. On one hand the IRQ can easily fire again before Lars-Peter> free_irq() is entered, on the other hand free_irq() itself Lars-Peter> calls synchronize_irq() internally (in a race condition free Lars-Peter> way), before any state associated with the IRQ is freed.

Applied to 4.6/scsi-queue.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Martin K. Petersen

2016-03-09 01:53 UTC #3 lore raw

Re: [PATCH resend 2/2] mpt3sas: Remove unnecessary synchronize_irq() before free_irq()

In reply to Lars-Peter Clausen

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Lars-Peter" == Lars-Peter Clausen <[email protected]> writes:

Lars-Peter> Calling synchronize_irq() right before free_irq() is quite Lars-Peter> useless. On one hand the IRQ can easily fire again before Lars-Peter> free_irq() is entered, on the other hand free_irq() itself Lars-Peter> calls synchronize_irq() internally (in a race condition free Lars-Peter> way), before any state associated with the IRQ is freed.

Broadcom folks, please review.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Sreekanth Reddy

maintainer

2016-03-09 13:26 UTC #4 lore raw

Re: [PATCH resend 2/2] mpt3sas: Remove unnecessary synchronize_irq() before free_irq()

Quoted 9 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Lars-Peter" == Lars-Peter Clausen <[email protected]> writes:
Lars-Peter> Calling synchronize_irq() right before free_irq() is quite Lars-Peter> useless. On one hand the IRQ can easily fire again before Lars-Peter> free_irq() is entered, on the other hand free_irq() itself Lars-Peter> calls synchronize_irq() internally (in a race condition free Lars-Peter> way), before any state associated with the IRQ is freed.
Broadcom folks, please review.

Martin,

Please consider this patch as Ack-by: Sreekanth Reddy <[email protected]>

Thanks,
Sreekanth

Quoted 3 lines

--
Martin K. Petersen Oracle Linux Engineering

Martin K. Petersen

Applied

2016-03-10 01:43 UTC #5 lore raw

Re: [PATCH resend 2/2] mpt3sas: Remove unnecessary synchronize_irq() before free_irq()

In reply to Lars-Peter Clausen

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Lars-Peter" == Lars-Peter Clausen <[email protected]> writes:

Lars-Peter> Calling synchronize_irq() right before free_irq() is quite Lars-Peter> useless. On one hand the IRQ can easily fire again before Lars-Peter> free_irq() is entered, on the other hand free_irq() itself Lars-Peter> calls synchronize_irq() internally (in a race condition free Lars-Peter> way), before any state associated with the IRQ is freed.

Applied to 4.6/scsi-queue.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Jitendra Bhivare

Reviewed-by on v1 1/2

2016-03-07 02:29 UTC #6 lore raw

In reply to Lars-Peter Clausen

Quoted 6 lines

-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Lars-Peter Clausen
Sent: Friday, March 04, 2016 3:45 PM
To: James E.J. Bottomley; Martin K. Petersen
Cc: Sathya Prakash; Chaitra P B; Suganath Prabu Subramani; Jitendra

Bhivare;

Quoted 3 lines

[email protected]; [email protected]; Lars-Peter Clausen
Subject: [PATCH resend 1/2] be2iscsi: Remove unnecessary

synchronize_irq()

Quoted 3 lines

before free_irq()
Calling synchronize_irq() right before free_irq() is quite useless. On

one hand the

Quoted 1 line

IRQ can easily fire again before free_irq() is entered, on the other hand

free_irq()

Quoted 1 line

itself calls synchronize_irq() internally (in a race condition free way),

before any

Quoted 11 lines

state associated with the IRQ is freed.
Patch was generated using the following semantic patch:
// <smpl>
@@
expression irq;
@@
-synchronize_irq(irq);
free_irq(irq, ...);
// </smpl>

Review

Quoted 6 lines

Signed-off-by Lars-Peter Clausen <[email protected]>
---
drivers/scsi/be2iscsi/be_main.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
(quoted diff) +0 -0

b/drivers/scsi/be2iscsi/be_main.c

drivers/scsi/be2iscsi/be_main.c static void beiscsi_quiesce(struct beiscsi_hba

Quoted 4 lines

index b51e726..e89a0f8d 100644
drivers/scsi/be2iscsi/be_main.c +0 -0
@@ -5303,15 +5303,12 @@ static void beiscsi_quiesce(struct beiscsi_hba

*phba,

Quoted 17 lines

drivers/scsi/be2iscsi/be_main.c +1 -5

 	if (phba->msix_enabled) {
 		for (i = 0; i <= phba->num_cpus; i++) {
 			msix_vec = phba->msix_entries[i].vector;
-			synchronize_irq(msix_vec);
 			free_irq(msix_vec, &phwi_context->be_eq[i]);
 			kfree(phba->msi_name[i]);
 		}
 	} else
-		if (phba->pcidev->irq) {
-			synchronize_irq(phba->pcidev->irq);
+		if (phba->pcidev->irq)
 			free_irq(phba->pcidev->irq, phba);
-		}
 	pci_disable_msix(phba->pcidev);
 	cancel_delayed_work_sync(&phba->beiscsi_hw_check_task);
 
--

Quoted 1 line

2.1.4

Reviewed-by Jitendra Bhivare <[email protected]>

Thanks,

JB

Martin K. Petersen

2016-03-08 02:07 UTC #7 lore raw

In reply to Lars-Peter Clausen

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Lars-Peter" == Lars-Peter Clausen <[email protected]> writes:

Lars-Peter> Calling synchronize_irq() right before free_irq() is quite Lars-Peter> useless. On one hand the IRQ can easily fire again before Lars-Peter> free_irq() is entered, on the other hand free_irq() itself Lars-Peter> calls synchronize_irq() internally (in a race condition free Lars-Peter> way), before any state associated with the IRQ is freed.

Sorry, replied to the wrong email. I applied the be2iscsi patch. The mpt one does not have any reviews yet...

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering