modify the type of element of MessageUnit_B in arcmsr

Subject: [PATCH] scsi: modify the element type of MessageUnit_B in arcmsr-1.20.00.15-80227
From: Nick Cheng <[email protected]>
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

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:[email protected]]
Sent: Wednesday, March 05, 2008 12:40 AM
To: [email protected]
Cc: 'Andrew Morton'; [email protected]; [email protected]; 'Tomas Henzl'; [email protected]
Subject: Re: [PATCH] modify the type of element of MessageUnit_B in arcmsr

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

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

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:[email protected]]
Sent: Wednesday, March 05, 2008 11:30 PM
To: [email protected]
Cc: 'Andrew Morton'; [email protected]; [email protected]; 'Tomas Henzl'; [email protected]
Subject: RE: [PATCH] modify the type of element of MessageUnit_B in arcmsr

width.

repeatedly.

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

Heh, you know how to smooth talk

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.

James