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

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

[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

Call Trace:
 [<ffffffff80212f24>] dma_alloc_coherent+0x1ba/0x1d2
 [<ffffffff80212c53>] dma_free_coherent+0x43/0x82
 [<ffffffff88044b65>] :arcmsr:arcmsr_queue_command+0x368/0x959
 [<ffffffff8042078f>] scsi_dispatch_cmd+0x1a9/0x1fe
 [<ffffffff804259c4>] scsi_request_fn+0x26f/0x33e
 [<ffffffff8038a781>] blk_execute_rq_nowait+0x7a/0x8e
 [<ffffffff80425471>] scsi_execute_async+0x345/0x394
 [<ffffffff8022d813>] __wake_up_common+0x41/0x74
 [<ffffffff8045b9dc>] sg_common_write+0x6b7/0x6ef
 [<ffffffff8045bc31>] sg_cmd_done+0x0/0x1d6
 [<ffffffff8022e200>] __dequeue_entity+0x1c/0x32
 [<ffffffff8045bc0e>] sg_new_write+0x1fa/0x21d
 [<ffffffff8045c31a>] sg_ioctl+0x218/0xa31
 [<ffffffff8024dc70>] hrtimer_cancel+0xc/0x16
 [<ffffffff8058d483>] do_nanosleep+0x55/0x7e
 [<ffffffff8024de3b>] hrtimer_nanosleep+0x5b/0xfe
 [<ffffffff802a5539>] do_ioctl+0x55/0x6b
 [<ffffffff802a579c>] vfs_ioctl+0x24d/0x266
 [<ffffffff802a57f1>] sys_ioctl+0x3c/0x5f
 [<ffffffff8020be2e>] system_call+0x7e/0x83

----------------------

The warning was added in dma_free_coherent()  (pci-dma_64.c and
pci-dma_32.c) in kernel 2.6.24 to avoid portability issues with drivers
using this function.

The commit details why this warning was added:

--------------------

dma_free_coherent() needs irqs enabled (sigh)

On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
call context requirement: unlike its dma_alloc_coherent() sibling, it may
not be called with IRQs disabled.  (This was new behavior on ARM as of late
2005, caused by ARM SMP updates.) This little surprise can be annoyingly
driver-visible.

Since it looks like that restriction won't be removed, this patch changes
the definition of the API to include that requirement.  Also, to help catch
nonportable drivers, it updates the x86 and swiotlb versions to include the
relevant warnings.  (I already observed that it trips on the
bus_reset_tasklet of the new firewire_ohci driver.)

--------------

the commit can be seen here:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=aa24886e379d2b641c5117e178b15ce1d5d366ba

If additional info is needed this can be provided and we would be happy to ask the user to test any available patches.

Best regards

Kim Højgaard-Hansen
Gentoo Kernel team

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

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

[James Bottomley]

On Sat, 2008-02-09 at 16:31 +0100, Kim Højgaard-Hansen wrote:
> 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


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

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

[Joshua Hoblitt]

Hi James,

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

-J

--
On Sat, Feb 09, 2008 at 12:01:16PM -0600, James Bottomley wrote:
> 
> On Sat, 2008-02-09 at 16:31 +0100, Kim H??jgaard-Hansen wrote:
> > 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
> 
> 

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

[James Bottomley]

On Sat, 2008-02-09 at 09:35 -1000, Joshua Hoblitt wrote:
> Hi James,
> 
> I can give it a try.  What commit id do you think fixed it?

commit 76d78300a6eb8b7f08e47703b7e68a659ffc2053
Author: Nick Cheng <nick.cheng@areca.com.tw>
Date:   Mon Feb 4 23:53:24 2008 -0800

    [SCSI] arcmsr: updates (1.20.00.15)

James

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

[Joshua Hoblitt]

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

--
On Sat, Feb 09, 2008 at 01:43:39PM -0600, James Bottomley wrote:
> 
> On Sat, 2008-02-09 at 09:35 -1000, Joshua Hoblitt wrote:
> > Hi James,
> > 
> > I can give it a try.  What commit id do you think fixed it?
> 
> commit 76d78300a6eb8b7f08e47703b7e68a659ffc2053
> Author: Nick Cheng <nick.cheng@areca.com.tw>
> Date:   Mon Feb 4 23:53:24 2008 -0800
> 
>     [SCSI] arcmsr: updates (1.20.00.15)
> 
> James
> 
> 

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

[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?

-J

--
On Tue, Feb 12, 2008 at 10:53:45AM -1000, Joshua Hoblitt wrote:
> 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
> 
> --
> On Sat, Feb 09, 2008 at 01:43:39PM -0600, James Bottomley wrote:
> > 
> > On Sat, 2008-02-09 at 09:35 -1000, Joshua Hoblitt wrote:
> > > Hi James,
> > > 
> > > I can give it a try.  What commit id do you think fixed it?
> > 
> > commit 76d78300a6eb8b7f08e47703b7e68a659ffc2053
> > Author: Nick Cheng <nick.cheng@areca.com.tw>
> > Date:   Mon Feb 4 23:53:24 2008 -0800
> > 
> >     [SCSI] arcmsr: updates (1.20.00.15)
> > 
> > James
> > 
> > 

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

[James Bottomley]

On Tue, 2008-02-12 at 12:21 -1000, Joshua Hoblitt wrote:
> 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

RE: arcmsr + archttp64 calls dma_free_coherent() with irqsdisabled - dmesg filled with warnings

[nickcheng]

Please keep me in the loop.

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] 
Sent: Wednesday, February 13, 2008 6:31 AM
To: Joshua Hoblitt
Cc: Kim H?jgaard-Hansen; erich@areca.com.tw; linux-scsi@vger.kernel.org;
dsd@gentoo.org; j_gentoo@hoblitt.com
Subject: Re: arcmsr + archttp64 calls dma_free_coherent() with irqsdisabled
- dmesg filled with warnings


On Tue, 2008-02-12 at 12:21 -1000, Joshua Hoblitt wrote:
> 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

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

[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:

http://bugs.gentoo.org/attachment.cgi?id=143585

Thanks,

-J

--
On Tue, Feb 12, 2008 at 04:30:36PM -0600, James Bottomley wrote:
> 
> On Tue, 2008-02-12 at 12:21 -1000, Joshua Hoblitt wrote:
> > 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
> 
> 

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

[James Bottomley]

On Fri, 2008-02-15 at 10:56 -1000, Joshua Hoblitt wrote:
> 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:
> 

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index f4a202e..4f9ff32 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
>  
>  	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?

> +		ver_addr = (unsigned long *)tmp;

No cast needed from void *

> +		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;
>  
> -		ver_addr = pci_alloc_consistent(acb->pdev, 1032, &buf_handle);
> -		if (!ver_addr) {
> +		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);
> +		ver_addr = (unsigned long *)tmp;

Actually, just do ver_addr = kmalloc(...) instead

> +		if (!tmp) {
>  			retvalue = ARCMSR_MESSAGE_FAIL;
>  			goto message_out;
>  		}
> @@ -1482,7 +1492,7 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
>  				retvalue = ARCMSR_MESSAGE_FAIL;
>  			}
>  			}
> -			pci_free_consistent(acb->pdev, 1032, ver_addr, buf_handle);
> +			kfree(tmp);
>  		}
>  		break;


I can't work out why this was pci_alloc_consistent in the first
place ... it's not used as consistent memory anywhere on the PCI bus


James

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

[James Bottomley]

On Fri, 2008-02-15 at 15:57 -0600, James Bottomley wrote:
> On Fri, 2008-02-15 at 10:56 -1000, Joshua Hoblitt wrote:
> > 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:
> > 
> 
> > diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> > index f4a202e..4f9ff32 100644
> > --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> > +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> > @@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
> >  
> >  	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?
> 
> > +		ver_addr = (unsigned long *)tmp;
> 
> No cast needed from void *
> 
> > +		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;
> >  
> > -		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.

James

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

[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.

James Bottomley wrote:
>>> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
>>> index f4a202e..4f9ff32 100644
>>> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
>>> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
>>> @@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
>>>  
>>>  	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?
>>
>>> +		ver_addr = (unsigned long *)tmp;
>> No cast needed from void *
>>
>>> +		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;
>>>  
>>> -		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.

Further confirmed by the fact that dma_free_coherent() was complaining 
about IRQs being disabled.

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

[James Bottomley]

On Sat, 2008-02-16 at 11:49 +0000, Daniel Drake wrote:
> 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.

In this case, the problems with the changes weren't obvious to me until
I saw the broken out diff for the backport. (And incidentally, never
send URLs to code; 95% of people don't click on them.  If you inline the
code, most people at least glance over it).

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

James

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

[Daniel Drake]

James Bottomley wrote:
> On Sat, 2008-02-16 at 11:49 +0000, Daniel Drake wrote:
>> 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.

Sorry, I didn't mean to sound that as a criticism. I'm sure you have a 
lot of patches flowing by you at any one time.

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.

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

No, and I can see that scsi_dispatch_command does invoke ->queuecommand 
under a spinlock so it must be atomic context...
I'm not sure which might_sleep() codepath you are looking at though. At 
a guess it depends on SLUB vs SLAB?

Daniel

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

[Daniel Drake]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

Daniel Drake wrote:
> 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.

[-- Attachment #2: 0001-arcmsr-fix-message-allocation.patch --]
[-- Type: text/plain, Size: 2578 bytes --]

>From 0a9cd6133fe4f0c3a8906d6be1b9d1ef083345dc Mon Sep 17 00:00:00 2001
From: Daniel Drake <dsd@gentoo.org>
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(-)

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index 4f9ff32..f91f79c 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -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;
 
-- 
1.5.4

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

[Joshua Hoblitt]

Thanks James.  I'll give a try tonight or tomorrow.

-J

--
On Sat, Feb 16, 2008 at 11:37:41PM +0000, Daniel Drake wrote:
> Daniel Drake wrote:
>> 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.

> >From 0a9cd6133fe4f0c3a8906d6be1b9d1ef083345dc Mon Sep 17 00:00:00 2001
> From: Daniel Drake <dsd@gentoo.org>
> 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(-)
> 
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 4f9ff32..f91f79c 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -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;
>  
> -- 
> 1.5.4
> 

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

[Joshua Hoblitt]

I have a test machine running:

0001-arcmsr-fix-message-allocation.patch
arcmsr-pci-alloc-consistent.patch
2.6.24-gentoo-r2

The arcmsr driver is loaded and archttp64 is running.  I've been access
archttp64 and there are no warnings in dmesg.  Without the patches this
would have generated 10s of MB of warning messages.  I haven't really
exercised the areca volumes yet but this looks to be both a fix for the
warnings and James' coding issues.  I'd vote for re-diffing and kicking
the patch upstream at this point.

-J

--
On Sat, Feb 16, 2008 at 11:37:41PM +0000, Daniel Drake wrote:
> Daniel Drake wrote:
>> 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.

> >From 0a9cd6133fe4f0c3a8906d6be1b9d1ef083345dc Mon Sep 17 00:00:00 2001
> From: Daniel Drake <dsd@gentoo.org>
> 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(-)
> 
> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 4f9ff32..f91f79c 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -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;
>  
> -- 
> 1.5.4
> 

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

[Daniel Drake]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]

Joshua Hoblitt wrote:
> 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?

Please just try this patch instead, it's what I am planning to submit to 
-stable and add to Gentoo if you report success.

Daniel


[-- Attachment #2: arcmsr-pci-alloc-consistent.patch --]
[-- Type: text/plain, Size: 2108 bytes --]

From: Nick Cheng <nick.cheng@areca.com.tw>

Partial backport of 76d78300 ("arcmsr: updates (1.20.00.15)") by
Daniel Drake <dsd@gentoo.org>. Removes pci_alloc_consistent usage, which
should not be used when IRQs are disabled for portability reasons. Fixes a
spew of dma_alloc_coherent irqs_disabled() warnings.

diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
index f4a202e..4f9ff32 100644
--- a/drivers/scsi/arcmsr/arcmsr_hba.c
+++ b/drivers/scsi/arcmsr/arcmsr_hba.c
@@ -1380,12 +1388,13 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 
 	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);
+		ver_addr = (unsigned long *)tmp;
+		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;
 
-		ver_addr = pci_alloc_consistent(acb->pdev, 1032, &buf_handle);
-		if (!ver_addr) {
+		tmp = kmalloc(1032, GFP_KERNEL|GFP_DMA);
+		ver_addr = (unsigned long *)tmp;
+		if (!tmp) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}
@@ -1482,7 +1492,7 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, \
 				retvalue = ARCMSR_MESSAGE_FAIL;
 			}
 			}
-			pci_free_consistent(acb->pdev, 1032, ver_addr, buf_handle);
+			kfree(tmp);
 		}
 		break;