Hi there!
The attachted patch fixes a compiler warning in drivers/scsi/dpt_i2o.c.
If you reply to this post, please CC me as I'm not suscribed to the
list.
Best Regards
--
Stefan Roas
[email protected]
On Sun, Mar 13, 2005 at 11:43:51PM +0100, Stefan Roas wrote:
> Hi there!
>
> The attachted patch fixes a compiler warning in drivers/scsi/dpt_i2o.c.
>
> If you reply to this post, please CC me as I'm not suscribed to the
> list.
This patch looks suspiciously like it is sweeping the problem
`under the carpet`. Does bus_to_virt() return an `void __iomem *`?
reply should really be an `void __iomem *`
> Best Regards
>
> --
> Stefan Roas
> [email protected]
> diff -rNu a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> --- a/drivers/scsi/dpt_i2o.c 2005-03-02 14:01:26.910737000 +0100
> +++ b/drivers/scsi/dpt_i2o.c 2005-03-04 11:28:57.339003000 +0100
> @@ -2027,8 +2027,8 @@
> }
> reply = (ulong)bus_to_virt(m);
>
> - if (readl(reply) & MSG_FAIL) {
> - u32 old_m = readl(reply+28);
> + if (readl((void *)reply) & MSG_FAIL) {
> + u32 old_m = readl((void *)reply+28);
> ulong msg;
> u32 old_context;
> PDEBUG("%s: Failed message\n",pHba->name);
> @@ -2039,34 +2039,34 @@
> }
> // Transaction context is 0 in failed reply frame
> msg = (ulong)(pHba->msg_addr_virt + old_m);
> - old_context = readl(msg+12);
> + old_context = readl((void *)msg+12);
> writel(old_context, reply+12);
> adpt_send_nop(pHba, old_m);
> }
> - context = readl(reply+8);
> + context = readl((void *)reply+8);
> if(context & 0x40000000){ // IOCTL
> - ulong p = (ulong)(readl(reply+12));
> + ulong p = (ulong)(readl((void *)reply+12));
> if( p != 0) {
> memcpy((void*)p, (void*)reply, REPLY_FRAME_SIZE * 4);
> }
> // All IOCTLs will also be post wait
> }
> if(context & 0x80000000){ // Post wait message
> - status = readl(reply+16);
> + status = readl((void *)reply+16);
> if(status >> 24){
> status &= 0xffff; /* Get detail status */
> } else {
> status = I2O_POST_WAIT_OK;
> }
> if(!(context & 0x40000000)) {
> - cmd = (struct scsi_cmnd*) readl(reply+12);
> + cmd = (struct scsi_cmnd*) readl((void *)reply+12);
> if(cmd != NULL) {
> printk(KERN_WARNING"%s: Apparent SCSI cmd in Post Wait Context - cmd=%p context=%x\n", pHba->name, cmd, context);
> }
> }
> adpt_i2o_post_wait_complete(context, status);
> } else { // SCSI message
> - cmd = (struct scsi_cmnd*) readl(reply+12);
> + cmd = (struct scsi_cmnd*) readl((void *)reply+12);
> if(cmd != NULL){
> if(cmd->serial_number != 0) { // If not timedout
> adpt_i2o_to_scsi(reply, cmd);
> @@ -2236,16 +2236,16 @@
> adpt_hba* pHba;
> u32 hba_status;
> u32 dev_status;
> - u32 reply_flags = readl(reply) & 0xff00; // Leave it shifted up 8 bits
> + u32 reply_flags = readl((void *)reply) & 0xff00; // Leave it shifted up 8 bits
> // I know this would look cleaner if I just read bytes
> // but the model I have been using for all the rest of the
> // io is in 4 byte words - so I keep that model
> - u16 detailed_status = readl(reply+16) &0xffff;
> + u16 detailed_status = readl((void *)reply+16) &0xffff;
> dev_status = (detailed_status & 0xff);
> hba_status = detailed_status >> 8;
>
> // calculate resid for sg
> - cmd->resid = cmd->request_bufflen - readl(reply+5);
> + cmd->resid = cmd->request_bufflen - readl((void *)reply+5);
>
> pHba = (adpt_hba*) cmd->device->host->hostdata[0];
>
> @@ -2256,7 +2256,7 @@
> case I2O_SCSI_DSC_SUCCESS:
> cmd->result = (DID_OK << 16);
> // handle underflow
> - if(readl(reply+5) < cmd->underflow ) {
> + if(readl((void *)reply+5) < cmd->underflow ) {
> cmd->result = (DID_ERROR <<16);
> printk(KERN_WARNING"%s: SCSI CMD underflow\n",pHba->name);
> }
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Mon Mar 14, 2005 at 18:24:44, Ben Dooks wrote:
> This patch looks suspiciously like it is sweeping the problem
> `under the carpet`. Does bus_to_virt() return an `void __iomem *`?
>
> reply should really be an `void __iomem *`
bus_to_virt returns void *.
adpt_isr casts the return value to ulong though and adpt_i2o_to_scsi
takes an ulong as its first argument and passes it to readl without a
cast then.
But I agree, the patch just silences the compiler warning. Maybe it
would be a better solution to change the types of reply and msg in
adpt_isr as well as the first argument to adpt_i2o_to_scsi to void
__iomem *.
Best Regards,
- --
Stefan Roas
[email protected]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
iD8DBQFCNe7CwvfNuQ9pAq8RAmDBAJ0XyRMogsfgX3L6+SzIzp6VPYuynQCgjpZ0
94sI0Fr7V8anGTsQQw+COcM=
=+qfk
-----END PGP SIGNATURE-----
On Mon, Mar 14, 2005 at 09:06:26PM +0100, Stefan Roas wrote:
> On Mon Mar 14, 2005 at 18:24:44, Ben Dooks wrote:
>
> > This patch looks suspiciously like it is sweeping the problem
> > `under the carpet`. Does bus_to_virt() return an `void __iomem *`?
> >
> > reply should really be an `void __iomem *`
>
> bus_to_virt returns void *.
>
> adpt_isr casts the return value to ulong though and adpt_i2o_to_scsi
> takes an ulong as its first argument and passes it to readl without a
> cast then.
>
> But I agree, the patch just silences the compiler warning. Maybe it
> would be a better solution to change the types of reply and msg in
> adpt_isr as well as the first argument to adpt_i2o_to_scsi to void
> __iomem *.
However, bus_to_virt() can only be used on addresses which correspond
with system RAM. readl and friends should not be used on such a memory
space. So, this driver is doing lots of bogus stuff.
The warnings are perfectly valid and should therefore stay.
If "reply" is actually referring to some system memory, the readl/writel
shouldn't be there, and we should be accessing system memory directly
via a bona-fide structure. If it isn't, then the bus_to_virt() is
completely bogus and unportable, and that's where we _must_ raise a
warning.
However, I do agree with changing "msg" to be void __iomem *, and
removing the (ulong) cast, since pHba->msg_addr_virt is already
void __iomem *.
(note: I'm just taking a passing interest in this thread, from the
point of view of an architecture maintainer who wants these types of
things to be Correct(tm)...)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core