[PATCH] modify the type of element of MessageUnit_B in arcmsr

[PATCH] modify the type of element of MessageUnit_B in arcmsr

[nickcheng]

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

Subject: [PATCH] scsi: modify the element type of MessageUnit_B in
arcmsr-1.20.00.15-80227
From: Nick Cheng <nick.cheng@areca.com.tw>
Description:
*** modify the element type of MessageUnit_B in arcmsr-1.20.00.15-80227 to
keep off the error while doing iounmap in arcmsr_free_ccb_pool()
Signed-off-by: Nick Cheng <nick.cheng@areca.com.tw>

[-- Attachment #2: patch4arcmsr --]
[-- Type: application/octet-stream, Size: 925 bytes --]

diff --git a/drivers/scsi/arcmsr/arcmsr.h b/drivers/scsi/arcmsr/arcmsr.h
index 0393707..3288be2 100644
--- a/drivers/scsi/arcmsr/arcmsr.h
+++ b/drivers/scsi/arcmsr/arcmsr.h
@@ -341,13 +341,13 @@ struct MessageUnit_B
 	uint32_t	done_qbuffer[ARCMSR_MAX_HBB_POSTQUEUE];
 	uint32_t	postq_index;
 	uint32_t	doneq_index;
-	uint32_t	__iomem *drv2iop_doorbell_reg;
-	uint32_t	__iomem *drv2iop_doorbell_mask_reg;
-	uint32_t	__iomem *iop2drv_doorbell_reg;
-	uint32_t	__iomem *iop2drv_doorbell_mask_reg;
-	uint32_t	__iomem *msgcode_rwbuffer_reg;
-	uint32_t	__iomem *ioctl_wbuffer_reg;
-	uint32_t	__iomem *ioctl_rbuffer_reg;
+	void		__iomem *drv2iop_doorbell_reg;
+	void		__iomem *drv2iop_doorbell_mask_reg;
+	void		__iomem *iop2drv_doorbell_reg;
+	void		__iomem *iop2drv_doorbell_mask_reg;
+	void		__iomem *msgcode_rwbuffer_reg;
+	void		__iomem *ioctl_wbuffer_reg;
+	void		__iomem *ioctl_rbuffer_reg;
 };
 
 /*

Re: [PATCH] modify the type of element of MessageUnit_B in arcmsr

[James Bottomley]

On Tue, 2008-03-04 at 17:49 +0800, nickcheng wrote:
> Subject: [PATCH] scsi: modify the element type of MessageUnit_B in
> arcmsr-1.20.00.15-80227
> From: Nick Cheng <nick.cheng@areca.com.tw>
> Description:
> *** modify the element type of MessageUnit_B in arcmsr-1.20.00.15-80227 to
> keep off the error while doing iounmap in arcmsr_free_ccb_pool()

What's the actual error this causes?

Looking at the code, all of these registers are genuinely 32 bits long,
so having them defined as uint32_t __iomem * is fine; so is having them
defined as void __iomem *, because void * can be transparently cast to
any pointer.

Because the readX/writeX routines are prototyped in terms of void
__iomem *, they do the casting transparently (and without warning), so
there's no useful typechecking with the uint32_t __iomem * definition,
thus I only have a tiny marginal preference for labelling the registers
with their correct width.  But I would like to understand what the error
is you're seeing.

Thanks,

James

RE: [PATCH] modify the type of element of MessageUnit_B in arcmsr

[nickcheng]

James,
Thanks for your care.
I agree with your point of labeling the registers with their correct width.
But I did see an error.
I have a script to load/unload driver modules for Type B Adapter repeatedly.
While unloading the former driver modules, it pops up the message,
iounmap: bad address ffffc2000021f000
Call Trace:
 [<ffffffff881ec2a2>] :arcmsr:arcmsr_free_ccb_pool+0x54/0x8f
 [<ffffffff881ec3c8>] :arcmsr:arcmsr_remove+0xeb/0x117
 [<ffffffff802668a2>] klist_release+0x0/0x45
 [<ffffffff8014f69c>] pci_device_remove+0x24/0x3a
 [<ffffffff801af54d>] __device_release_driver+0x79/0x9d
 [<ffffffff801af8c7>] driver_detach+0xad/0x101
 [<ffffffff801aebdd>] bus_remove_driver+0x6d/0x90
 [<ffffffff801af94e>] driver_unregister+0xd/0x16
 [<ffffffff8014f936>] pci_unregister_driver+0x10/0x5f
 [<ffffffff800a39e2>] sys_delete_module+0x196/0x1c5
 [<ffffffff8005d28d>] tracesys+0xd5/0xe0.

Although it has tiny possibility to hang the system, but for the safety I
determine to change.

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] 
Sent: Wednesday, March 05, 2008 12:40 AM
To: nick.cheng@areca.com.tw
Cc: 'Andrew Morton'; linux-scsi@vger.kernel.org; randy.dunlap@oracle.com;
'Tomas Henzl'; viro@ftp.linux.org.uk
Subject: Re: [PATCH] modify the type of element of MessageUnit_B in arcmsr


On Tue, 2008-03-04 at 17:49 +0800, nickcheng wrote:
> Subject: [PATCH] scsi: modify the element type of MessageUnit_B in
> arcmsr-1.20.00.15-80227
> From: Nick Cheng <nick.cheng@areca.com.tw>
> Description:
> *** modify the element type of MessageUnit_B in arcmsr-1.20.00.15-80227 to
> keep off the error while doing iounmap in arcmsr_free_ccb_pool()

What's the actual error this causes?

Looking at the code, all of these registers are genuinely 32 bits long,
so having them defined as uint32_t __iomem * is fine; so is having them
defined as void __iomem *, because void * can be transparently cast to
any pointer.

Because the readX/writeX routines are prototyped in terms of void
__iomem *, they do the casting transparently (and without warning), so
there's no useful typechecking with the uint32_t __iomem * definition,
thus I only have a tiny marginal preference for labelling the registers
with their correct width.  But I would like to understand what the error
is you're seeing.

Thanks,

James

RE: [PATCH] modify the type of element of MessageUnit_B in arcmsr

[James Bottomley]

On Wed, 2008-03-05 at 15:38 +0800, nickcheng wrote:
> James,
> Thanks for your care.
> I agree with your point of labeling the registers with their correct width.
> But I did see an error.
> I have a script to load/unload driver modules for Type B Adapter repeatedly.
> While unloading the former driver modules, it pops up the message,
> iounmap: bad address ffffc2000021f000
> Call Trace:
>  [<ffffffff881ec2a2>] :arcmsr:arcmsr_free_ccb_pool+0x54/0x8f

Aha, that's because of this, isn't it:

                iounmap(reg->drv2iop_doorbell_reg - ARCMSR_DRV2IOP_DOORBELL);
                iounmap(reg->ioctl_wbuffer_reg - ARCMSR_IOCTL_WBUFFER);

You're doing pointer subtraction on a uint32_t *, so it's subtracting in
units of four bytes.  In many ways, converting the two pointers to void
* isn't much better, because gcc can sometimes complain it doesn't know
the width of a void * for pointer subtraction ... but mostly it just
treats it as byte width.  What you actually want, to get the error to go
away is:

                iounmap((u8 *)reg->drv2iop_doorbell_reg - ARCMSR_DRV2IOP_DOORBELL);
                iounmap((u8 *)reg->ioctl_wbuffer_reg - ARCMSR_IOCTL_WBUFFER);

Which should ensure the pointer arithmetic is done correctly.  Of
course, the ideal would be to get rid of the pointer subtraction ... it
always ends up causing subtle and hard to find problems.

James

RE: [PATCH] modify the type of element of MessageUnit_B in arcmsr

[nickcheng]

James,
You are such a great teacher. 
I learned it.
So what about my last submitted patch?
Would it be turned down or go on?

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] 
Sent: Wednesday, March 05, 2008 11:30 PM
To: nick.cheng@areca.com.tw
Cc: 'Andrew Morton'; linux-scsi@vger.kernel.org; randy.dunlap@oracle.com;
'Tomas Henzl'; viro@ftp.linux.org.uk
Subject: RE: [PATCH] modify the type of element of MessageUnit_B in arcmsr

On Wed, 2008-03-05 at 15:38 +0800, nickcheng wrote:
> James,
> Thanks for your care.
> I agree with your point of labeling the registers with their correct
width.
> But I did see an error.
> I have a script to load/unload driver modules for Type B Adapter
repeatedly.
> While unloading the former driver modules, it pops up the message,
> iounmap: bad address ffffc2000021f000
> Call Trace:
>  [<ffffffff881ec2a2>] :arcmsr:arcmsr_free_ccb_pool+0x54/0x8f

Aha, that's because of this, isn't it:

                iounmap(reg->drv2iop_doorbell_reg -
ARCMSR_DRV2IOP_DOORBELL);
                iounmap(reg->ioctl_wbuffer_reg - ARCMSR_IOCTL_WBUFFER);

You're doing pointer subtraction on a uint32_t *, so it's subtracting in
units of four bytes.  In many ways, converting the two pointers to void
* isn't much better, because gcc can sometimes complain it doesn't know
the width of a void * for pointer subtraction ... but mostly it just
treats it as byte width.  What you actually want, to get the error to go
away is:

                iounmap((u8 *)reg->drv2iop_doorbell_reg -
ARCMSR_DRV2IOP_DOORBELL);
                iounmap((u8 *)reg->ioctl_wbuffer_reg -
ARCMSR_IOCTL_WBUFFER);

Which should ensure the pointer arithmetic is done correctly.  Of
course, the ideal would be to get rid of the pointer subtraction ... it
always ends up causing subtle and hard to find problems.

James

RE: [PATCH] modify the type of element of MessageUnit_B in arcmsr

[James Bottomley]

On Thu, 2008-03-06 at 10:04 +0800, nickcheng wrote:
> James,
> You are such a great teacher. 

Heh, you know how to smooth talk

> I learned it.
> So what about my last submitted patch?
> Would it be turned down or go on?

I'm fine with either fix.  For either we need a better changelog.

Something like:

The Type B Adapter teardown does iounmap on pointers subtracted by a
constant offset.  Since the offset is in bytes, we need the pointers to
be of type void * not uint32_t * so the subtraction is done in the
correct units and we iounmap the correct area.

Signed-off-by: ...

James