arcmsr + archttp64 calls dma_free_coherent() with irqs disabled - dmesg filled with warnings

The following was reported by Joshua Hoblitt on Gentoo bugzilla when using the arcmsr driver (see https://bugs.gentoo.org/208493)

When starting up the Areca RAID card http pass through proxy called archttp dmesg is completely filled with messages as:

WARNING: at arch/x86/kernel/pci-dma_64.c:169 dma_free_coherent()

Pid: 8232, comm: archttp64 Not tainted 2.6.24-gentoo #1

Quoted 8 lines · Kim H??jgaard-Hansen

The following was reported by Joshua Hoblitt on Gentoo bugzilla when using the arcmsr driver (see https://bugs.gentoo.org/208493)

When starting up the Areca RAID card http pass through proxy called archttp dmesg is completely filled with messages as:

WARNING: at arch/x86/kernel/pci-dma_64.c:169 dma_free_coherent()

Pid: 8232, comm: archttp64 Not tainted 2.6.24-gentoo #1

Could you check git head, please; I think it's fixed upstream.

James

Hi James,

I can give it a try. What commit id do you think fixed it?

Quoted 3 lines · Joshua Hoblitt

Hi James,

I can give it a try. What commit id do you think fixed it?

commit 76d78300a6eb8b7f08e47703b7e68a659ffc2053

Author: Nick Cheng <[email protected]>
Date:   Mon Feb 4 23:53:24 2008 -0800

    [SCSI] arcmsr: updates (1.20.00.15)

James

I tried doing a build from that commit id but I get an oops from an unrelated subsystem. I'll try again from the head of the tree.

-J

--

Quoted 15 lines · James Bottomley

Quoted 3 lines · Joshua Hoblitt

Hi James,

I can give it a try. What commit id do you think fixed it?

commit 76d78300a6eb8b7f08e47703b7e68a659ffc2053

Author: Nick Cheng <[email protected]>
Date:   Mon Feb 4 23:53:24 2008 -0800

    [SCSI] arcmsr: updates (1.20.00.15)

James

I've got a few compilation errors on head. Before I try it, is it possible to backport the 76d78300a6eb8b7f08e47703b7e68a659ffc2053 change to 2.6.24.y?

I've got a few compilation errors on head. Before I try it, is it possible to backport the 76d78300a6eb8b7f08e47703b7e68a659ffc2053 change to 2.6.24.y?

Quoted 3 lines · Joshua Hoblitt

I've got a few compilation errors on head. Before I try it, is it possible to backport the 76d78300a6eb8b7f08e47703b7e68a659ffc2053 change to 2.6.24.y?

I don't see any reason why not but I'd have to cherry pick it to see ... so you could save me the bother by trying it yourself ...

James

Hi James,

Daniel took the time to patch up the 2.6.24 version. I've tested it and the warning messages are gone. Please take a look at:

drivers/scsi/arcmsr/arcmsr_hba.c +0 -0

index f4a202e..4f9ff32 100644
@@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 

drivers/scsi/arcmsr/arcmsr_hba.c +2 -1

+		if (!tmp) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}

@@ -1421,18 +1430,19 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 		memcpy(pcmdmessagefld->messagedatabuffer, (uint8_t *)ver_addr, allxfer_len);
 		pcmdmessagefld->cmdmessage.Length = allxfer_len;
 		pcmdmessagefld->cmdmessage.ReturnCode = ARCMSR_MESSAGE_RETURNCODE_OK;
-		pci_free_consistent(acb->pdev, 1032, ver_addr, buf_handle);
+		kfree(tmp);
 		}
 		break;
 
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {

Quoted 12 lines · Joshua Hoblitt

Hi James,

Daniel took the time to patch up the 2.6.24 version. I've tested it and the warning messages are gone. Please take a look at:

drivers/scsi/arcmsr/arcmsr_hba.c +0 -0

index f4a202e..4f9ff32 100644
@@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \ · line 1395

Quoted 10 lines

drivers/scsi/arcmsr/arcmsr_hba.c +2 -3

 	case ARCMSR_MESSAGE_READ_RQBUFFER: {
 		unsigned long *ver_addr;
-		dma_addr_t buf_handle;
 		uint8_t *pQbuffer, *ptmpQbuffer;
 		int32_t allxfer_len = 0;
+		void *tmp;
 
-		ver_addr = pci_alloc_consistent(acb->pdev, 1032, &buf_handle);
-		if (!ver_addr) {
+		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);

GFP_DMA is pretty pointless for a buffer which never actually gets anywhere near a DMA, isn't it?

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \ · line 1396

Quoted 1 line

drivers/scsi/arcmsr/arcmsr_hba.c +1 -0

+		ver_addr = (unsigned long *)tmp;

No cast needed from void *

Quoted 20 lines

drivers/scsi/arcmsr/arcmsr_hba.c +3 -2

+		if (!tmp) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}

@@ -1421,18 +1430,19 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 		memcpy(pcmdmessagefld->messagedatabuffer, (uint8_t *)ver_addr, allxfer_len);
 		pcmdmessagefld->cmdmessage.Length = allxfer_len;
 		pcmdmessagefld->cmdmessage.ReturnCode = ARCMSR_MESSAGE_RETURNCODE_OK;
-		pci_free_consistent(acb->pdev, 1032, ver_addr, buf_handle);
+		kfree(tmp);
 		}
 		break;
 
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
 		unsigned long *ver_addr;
-		dma_addr_t buf_handle;
 		int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
+		void *tmp;
 

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \ · line 1443

Quoted 3 lines

drivers/scsi/arcmsr/arcmsr_hba.c +1 -2

-		ver_addr = pci_alloc_consistent(acb->pdev, 1032, &buf_handle);
-		if (!ver_addr) {
+		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);

Quoted 41 lines

Quoted 6 lines

drivers/scsi/arcmsr/arcmsr_hba.c +0 -0

index f4a202e..4f9ff32 100644
@@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \ · line 1395

Quoted 10 lines

drivers/scsi/arcmsr/arcmsr_hba.c +2 -3

 	case ARCMSR_MESSAGE_READ_RQBUFFER: {
 		unsigned long *ver_addr;
-		dma_addr_t buf_handle;
 		uint8_t *pQbuffer, *ptmpQbuffer;
 		int32_t allxfer_len = 0;
+		void *tmp;
 
-		ver_addr = pci_alloc_consistent(acb->pdev, 1032, &buf_handle);
-		if (!ver_addr) {
+		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);

GFP_DMA is pretty pointless for a buffer which never actually gets anywhere near a DMA, isn't it?

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \ · line 1396

Quoted 1 line

drivers/scsi/arcmsr/arcmsr_hba.c +1 -0

+		ver_addr = (unsigned long *)tmp;

No cast needed from void *

Quoted 20 lines

drivers/scsi/arcmsr/arcmsr_hba.c +3 -2

+		if (!tmp) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}

@@ -1421,18 +1430,19 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 		memcpy(pcmdmessagefld->messagedatabuffer, (uint8_t *)ver_addr, allxfer_len);
 		pcmdmessagefld->cmdmessage.Length = allxfer_len;
 		pcmdmessagefld->cmdmessage.ReturnCode = ARCMSR_MESSAGE_RETURNCODE_OK;
-		pci_free_consistent(acb->pdev, 1032, ver_addr, buf_handle);
+		kfree(tmp);
 		}
 		break;
 
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
 		unsigned long *ver_addr;
-		dma_addr_t buf_handle;
 		int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
+		void *tmp;
 

Quoted 3 lines

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \ · line 1443

Quoted 3 lines

drivers/scsi/arcmsr/arcmsr_hba.c +1 -2

-		ver_addr = pci_alloc_consistent(acb->pdev, 1032, &buf_handle);
-		if (!ver_addr) {
+		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);

Actually, also all the code around here implies we're in atomic context, so that GFP_KERNEL can't be right either.

I assume you're aware that this patch is just a subset of commit 76d78300a6eb8 which you've already pushed up to Linus. Adding Nick Cheng (commit author) to CC so that he can go over the feedback.

Quoted 1 line · Daniel Drake

I assume you're aware that this patch is just a subset of commit 76d78300a6eb8 which you've already pushed up to Linus. Adding Nick Cheng (commit author) to CC so that he can go over the feedback.

Well, in case it's not obvious by now: The way to get bad code upstream is to send a patch that combines many changes (the more the better) so that any potential reviewer has no idea which change is meant by which hunk and then to make sure Andrew picks it up so he'll hound the subsystem Maintainer until it's applied. Best of all, mention that it fixes a bug and you're made.

The odd thing is, it should have triggered a might_sleep() warning under testing ... do you know why it didn't?

Here is a patch to address your comments.
Joshua, would you mind testing this before I submit it properly? It will apply cleanly to 2.6.24 on top of the previous patch you tested. I have compile-tested it.

From 0a9cd6133fe4f0c3a8906d6be1b9d1ef083345dc Mon Sep 17 00:00:00 2001

Quoted 4 lines · Daniel Drake

Here is a patch to address your comments.
Joshua, would you mind testing this before I submit it properly? It will apply cleanly to 2.6.24 on top of the previous patch you tested. I have compile-tested it.

It would help to include the patch.

Quoted 1 line

From 0a9cd6133fe4f0c3a8906d6be1b9d1ef083345dc Mon Sep 17 00:00:00 2001

From: Daniel Drake <[email protected]>
Date: Sat, 16 Feb 2008 23:25:02 +0000
Subject: [PATCH] arcmsr: fix message allocation

---
 drivers/scsi/arcmsr/arcmsr_hba.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

drivers/scsi/arcmsr/arcmsr_hba.c +11 -15

index 4f9ff32..f91f79c 100644
@@ -1387,18 +1387,16 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 	switch(controlcode) {
 
 	case ARCMSR_MESSAGE_READ_RQBUFFER: {
-		unsigned long *ver_addr;
+		unsigned char *ver_addr;
 		uint8_t *pQbuffer, *ptmpQbuffer;
 		int32_t allxfer_len = 0;
-		void *tmp;
 
-		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);
-		ver_addr = (unsigned long *)tmp;
-		if (!tmp) {
+		ver_addr = kmalloc(1032, GFP_ATOMIC);
+		if (!ver_addr) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}
-		ptmpQbuffer = (uint8_t *) ver_addr;
+		ptmpQbuffer = ver_addr;
 		while ((acb->rqbuf_firstindex != acb->rqbuf_lastindex)
 			&& (allxfer_len < 1031)) {
 			pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];

@@ -1427,26 +1425,24 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 			}
 			arcmsr_iop_message_read(acb);
 		}
-		memcpy(pcmdmessagefld->messagedatabuffer, (uint8_t *)ver_addr, allxfer_len);
+		memcpy(pcmdmessagefld->messagedatabuffer, ver_addr, allxfer_len);
 		pcmdmessagefld->cmdmessage.Length = allxfer_len;
 		pcmdmessagefld->cmdmessage.ReturnCode = ARCMSR_MESSAGE_RETURNCODE_OK;
-		kfree(tmp);
+		kfree(ver_addr);
 		}
 		break;
 
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
-		unsigned long *ver_addr;
+		unsigned char *ver_addr;
 		int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
-		void *tmp;
 
-		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);
-		ver_addr = (unsigned long *)tmp;
-		if (!tmp) {
+		ver_addr = kmalloc(1032, GFP_ATOMIC);
+		if (!ver_addr) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}
-		ptmpuserbuffer = (uint8_t *)ver_addr;
+		ptmpuserbuffer = ver_addr;
 		user_len = pcmdmessagefld->cmdmessage.Length;
 		memcpy(ptmpuserbuffer, pcmdmessagefld->messagedatabuffer, user_len);
 		wqbuf_lastindex = acb->wqbuf_lastindex;

@@ -1492,7 +1488,7 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 				retvalue = ARCMSR_MESSAGE_FAIL;
 			}
 			}
-			kfree(tmp);
+			kfree(ver_addr);
 		}
 		break;
 

Signature · 1 line

1.5.4

Quoted 4 lines · Daniel Drake

Here is a patch to address your comments.
Joshua, would you mind testing this before I submit it properly? It will apply cleanly to 2.6.24 on top of the previous patch you tested. I have compile-tested it.

It would help to include the patch.

Quoted 1 line

From 0a9cd6133fe4f0c3a8906d6be1b9d1ef083345dc Mon Sep 17 00:00:00 2001

From: Daniel Drake <[email protected]>
Date: Sat, 16 Feb 2008 23:25:02 +0000
Subject: [PATCH] arcmsr: fix message allocation

---
 drivers/scsi/arcmsr/arcmsr_hba.c |   26 +++++++++++---------------
 1 files changed, 11 insertions(+), 15 deletions(-)

drivers/scsi/arcmsr/arcmsr_hba.c +11 -15

index 4f9ff32..f91f79c 100644
@@ -1387,18 +1387,16 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 	switch(controlcode) {
 
 	case ARCMSR_MESSAGE_READ_RQBUFFER: {
-		unsigned long *ver_addr;
+		unsigned char *ver_addr;
 		uint8_t *pQbuffer, *ptmpQbuffer;
 		int32_t allxfer_len = 0;
-		void *tmp;
 
-		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);
-		ver_addr = (unsigned long *)tmp;
-		if (!tmp) {
+		ver_addr = kmalloc(1032, GFP_ATOMIC);
+		if (!ver_addr) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}
-		ptmpQbuffer = (uint8_t *) ver_addr;
+		ptmpQbuffer = ver_addr;
 		while ((acb->rqbuf_firstindex != acb->rqbuf_lastindex)
 			&& (allxfer_len < 1031)) {
 			pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex];

@@ -1427,26 +1425,24 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 			}
 			arcmsr_iop_message_read(acb);
 		}
-		memcpy(pcmdmessagefld->messagedatabuffer, (uint8_t *)ver_addr, allxfer_len);
+		memcpy(pcmdmessagefld->messagedatabuffer, ver_addr, allxfer_len);
 		pcmdmessagefld->cmdmessage.Length = allxfer_len;
 		pcmdmessagefld->cmdmessage.ReturnCode = ARCMSR_MESSAGE_RETURNCODE_OK;
-		kfree(tmp);
+		kfree(ver_addr);
 		}
 		break;
 
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
-		unsigned long *ver_addr;
+		unsigned char *ver_addr;
 		int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
-		void *tmp;
 
-		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);
-		ver_addr = (unsigned long *)tmp;
-		if (!tmp) {
+		ver_addr = kmalloc(1032, GFP_ATOMIC);
+		if (!ver_addr) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}
-		ptmpuserbuffer = (uint8_t *)ver_addr;
+		ptmpuserbuffer = ver_addr;
 		user_len = pcmdmessagefld->cmdmessage.Length;
 		memcpy(ptmpuserbuffer, pcmdmessagefld->messagedatabuffer, user_len);
 		wqbuf_lastindex = acb->wqbuf_lastindex;

@@ -1492,7 +1488,7 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 				retvalue = ARCMSR_MESSAGE_FAIL;
 			}
 			}
-			kfree(tmp);
+			kfree(ver_addr);
 		}
 		break;
 

Signature · 1 line

1.5.4

I've got a few compilation errors on head. Before I try it, is it possible to backport the 76d78300a6eb8b7f08e47703b7e68a659ffc2053 change to 2.6.24.y?