drivers/scsi/initio.c: suppress compile warning
PATCH 13/17 drivers/scsi/initio.c: suppress compile warning
From: Andrew Morton <[email protected]>
powerpc:
drivers/scsi/initio.c: In function 'initio_build_scb':drivers/scsi/initio.c:2585: warning: large integer implicitly truncated to unsigned type
---
drivers/scsi/initio.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/scsi/initio.c renamed from drivers/scsi/initio.c~drivers-scsi-initioc-suppress-compile-warning +5 -1
@@ -2582,7 +2582,11 @@ static void initio_build_scb(struct init dma_addr =dma_map_single (&host->pci_dev->dev, cmnd->sense_buffer,SENSE_SIZE ,DMA_FROM_DEVICE ); cblk->senseptr =cpu_to_le32 ((u32 )dma_addr); - cblk->senselen =cpu_to_le32 (SENSE_SIZE ); +/* +* The below needs casting to avoid a "large integer implicitly +* truncated to unsigned type" warning on powerpc +*/ + cblk->senselen = (u8 )cpu_to_le32 (SENSE_SIZE ); cmnd->SCp.ptr = (char *)(unsigned long )dma_addr; cblk->cdblen = cmnd->cmd_len;
_
Quoted 6 lines · On Fri, Mar 28, 2008 at 2:48 PM, <[email protected]> wrote:
From: Andrew Morton <[email protected]>
powerpc:
drivers/scsi/initio.c: In function 'initio_build_scb':
drivers/scsi/initio.c:2585: warning: large integer implicitly truncated to unsigned type
I posted a fix for this yesterday and Alan Cox ACKed it.
It's here:
http://marc.info/?l=linux-scsi&m=120668352622659&w=2
hth,
grant
Quoted 31 lines
Cc James Bottomley <[email protected]>Cc FUJITA Tomonori <[email protected]>Signed-off-by Andrew Morton <[email protected]>---
drivers/scsi/initio.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)diff -puN
drivers/scsi/initio.c~drivers-scsi-initioc-suppress-compile-warningdrivers/scsi/initio.c--- a/drivers/scsi/initio.c~drivers-scsi-initioc-suppress-compile-warning
+++a/drivers/scsi/initio.c
@@ -2582,7 +2582,11 @@ static void initio_build_scb(struct initdma_addr = dma_map_single(&host->pci_dev->dev, cmnd->sense_buffer, SENSE_SIZE, DMA_FROM_DEVICE); cblk->senseptr = cpu_to_le32((u32)dma_addr); - cblk->senselen = cpu_to_le32(SENSE_SIZE); + /* + * The below needs casting to avoid a "large integer implicitly + * truncated to unsigned type" warning on powerpc + */ + cblk->senselen = (u8)cpu_to_le32(SENSE_SIZE); cmnd->SCp.ptr = (char *)(unsigned long)dma_addr; cblk->cdblen = cmnd->cmd_len;_
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · On Fri, 28 Mar 2008 14:55:28 -0700 "Grant Grundler" <[email protected]> wrote:
Quoted 6 lines · On Fri, Mar 28, 2008 at 2:48 PM, <[email protected]> wrote:
From: Andrew Morton <[email protected]>
powerpc:
drivers/scsi/initio.c: In function 'initio_build_scb':
drivers/scsi/initio.c:2585: warning: large integer implicitly truncated to unsigned typeI posted a fix for this yesterday and Alan Cox ACKed it.
Oh well, I droped this one then.
Quoted 2 lines
It's here:
http://marc.info/?l=linux-scsi&m=120668352622659&w=2
And there it will probably stay. Please send me a copy if you want to delegate the send-it-until-it-sticks slog.
James Bottomley
maintainerQuoted 5 lines · Grant Grundler
Quoted 4 lines · On Fri, Mar 28, 2008 at 2:48 PM, <[email protected]> wrote:
From: Andrew Morton <[email protected]>
powerpc:
Quoted 7 lines
Quoted 2 lines
drivers/scsi/initio.c: In function 'initio_build_scb':
drivers/scsi/initio.c:2585: warning: large integer implicitly truncated to unsigned typeI posted a fix for this yesterday and Alan Cox ACKed it.
It's here:
http://marc.info/?l=linux-scsi&m=120668352622659&w=2
Actually, I have to say that neither of these looks to be correct. Andrew's is obviously wrong because (u8)cpu_to_le32(xxx) always returns zero on a BE platform if xxx is an 8 bit quantity.
However, the driver clearly does a cblk->bufflen (a __le32 quantity) = cblk->senselen, which looks obviously wrong on a BE platform as well. Plus there's another hunk around here:
scsi_for_each_sg(cmnd, sglist, cblk->sglen, i) {
sg->data = cpu_to_le32((u32)sg_dma_address(sglist));
total_len += sg->len = cpu_to_le32((u32)sg_dma_len(sglist));
++sg;
}here total_len is a le32 quantity
cblk->buflen = (scsi_bufflen(cmnd) > total_len) ?
here we compare against a CPU native quantity
total_len : scsi_bufflen(cmnd);
And here we set either to a le32 or cpu native quantity depending on the result of the comparison.
Alan, has this driver ever worked on a BE platform?
James
Quoted 1 line
Alan, has this driver ever worked on a BE platform?
Not afaik.
James Bottomley
maintainerQuoted 3 lines · Alan Cox
Quoted 1 line
Alan, has this driver ever worked on a BE platform?
Not afaik.
It looks to me like the SCSI piece is entirely driven by the state model in tulip_scsi, which seems to drive the phases by I/O instructions which are endian safe, so the only issue I can see is the actual SG entries themselves, which begin as an outl of the pointer to the SG table, but which must be pulled into the card by DMA.
So basically, most of the cpu_to_le32 in this driver look wrong. If I can fix it (or persuade someone else to fix it) can anyone test it on a BE platform?
James
On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
<[email protected]> wrote:
...
Quoted 3 lines
So basically, most of the
cpu_to_le32in this driver look wrong. If I can fix it (or persuade someone else to fix it) can anyone test it on a BE platform?
But the code I submitted the patch is also broken for LE platforms. (As you pointed out earlier and was my original incentive for submitting the patch).
If most of the usage is wrong anyway, perhaps it's better to not pretend the driver can work on a BE platform and just rip all the cpu_to_le32() usage out...including the one I submitted the patch for. Either way, that change should go in. Right?
hth,
grant
James Bottomley
maintainerQuoted 10 lines · Grant Grundler
On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
<[email protected]> wrote:
...Quoted 3 lines
So basically, most of the
cpu_to_le32in this driver look wrong. If I can fix it (or persuade someone else to fix it) can anyone test it on a BE platform?But the code I submitted the patch is also broken for LE platforms. (As you pointed out earlier and was my original incentive for submitting the patch).
No ... it's correct on a LE platform .. the warning is superfluous we promote a u8 to a u32 and then complain when it's truncated to a u8 again.
Quoted 4 lines
If most of the usage is wrong anyway, perhaps it's better to not pretend the driver can work on a BE platform and just rip all the
cpu_to_le32()usage out...including the one I submitted the patch for. Either way, that change should go in. Right?
Well, not really; the problem is it's not complete ... it only covers up the real problem by silencing the warning.
If the actual BE pieces of the driver worked, you could make it correct either by making senselen a u32 and leaving the cpu_to_le32 or adding it to the point at which we assign it to bufflen.
If you can verify my analysis of the way the driver works, then the complete fix should be pretty simple: just remove the cpu_to_le32 from everywhere except the sg list construction.
James
Quoted 8 lines · James Bottomley
Quoted 7 lines · Grant Grundler
On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
<[email protected]> wrote:
...Quoted 3 lines
So basically, most of the
cpu_to_le32in this driver look wrong. If I can fix it (or persuade someone else to fix it) can anyone test it on a BE platform?
Quoted 7 lines
Quoted 3 lines
But the code I submitted the patch is also broken for LE platforms. (As you pointed out earlier and was my original incentive for submitting the patch).
No ... it's correct on a LE platform .. the warning is superfluous we promote a u8 to a u32 and then complain when it's truncated to a u8 again.
Yeah, you are right. It would produce correct code on LE platform. Not sure why now I thought it wouldn't.
Quoted 7 lines
Quoted 4 lines
If most of the usage is wrong anyway, perhaps it's better to not pretend the driver can work on a BE platform and just rip all the
cpu_to_le32()usage out...including the one I submitted the patch for. Either way, that change should go in. Right?Well, not really; the problem is it's not complete ... it only covers up the real problem by silencing the warning.
After looking at the code for a bit, I think I would prefer to disable the driver entirely for BE platforms since it's pretty clear it could never work correctly. I'm not willing to own it for BE platforms.
Quoted 3 lines
If the actual BE pieces of the driver worked, you could make it correct either by making senselen a u32 and leaving the
cpu_to_le32or adding it to the point at which we assign it to bufflen.
Making senselen u32 would also require fixing this bit in tulip_main():
if (!(scb->mode & SCM_RSENS)) { /* not
in auto req. sense mode */
...
if (scb->flags & SCF_SENSE) {
u8 len;
len = scb->senselen;
...
scb->cdb[3] = 0;
scb->cdb[4] = len;
scb->cdb[5] = 0;
initio_push_pend_scb(host, scb);
break;...
Quoted 3 lines
If you can verify my analysis of the way the driver works, then the complete fix should be pretty simple: just remove the
cpu_to_le32from everywhere except the sg list construction.
I think your analysis is correct. Seems the key bits are in initio_xfer_data_in/out() routines and around the SCF_SG handling.
But I am not interested in actually testing it. The more I look at this code, the less I want to do with it.
Another similar issue with bufptr usage in tulip_main():
scb->bufptr = scb->senseptr;
is pushing a LE32 into native byte pointer used in initio_state_5():
scb->bufptr += ((u32) (i - scb->sgidx) << 3);
and
scb->bufptr += (u32) xcnt;
bufptr will be converted to "Bus Endianess" when finally written to the controller:
outl(scb->bufptr, host->addr + TUL_XAddH);
I also expected bugs with initio_host->semaph. I initially thought there would be memory ordering issues and needed to be declared "volatile". But seems to be properly protected by a spinlock.
Nit: i91u_intr() could use spin_lock() instead of spinlock_irqsave().
hth,
grant
James Bottomley
maintainerQuoted 26 lines · Grant Grundler
Quoted 8 lines · James Bottomley
Quoted 7 lines · Grant Grundler
On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
<[email protected]> wrote:
...Quoted 3 lines
So basically, most of the
cpu_to_le32in this driver look wrong. If I can fix it (or persuade someone else to fix it) can anyone test it on a BE platform?Quoted 7 lines
Quoted 3 lines
But the code I submitted the patch is also broken for LE platforms. (As you pointed out earlier and was my original incentive for submitting the patch).
No ... it's correct on a LE platform .. the warning is superfluous we promote a u8 to a u32 and then complain when it's truncated to a u8 again.
Yeah, you are right. It would produce correct code on LE platform. Not sure why now I thought it wouldn't.
Quoted 5 lines
Quoted 4 lines
If most of the usage is wrong anyway, perhaps it's better to not pretend the driver can work on a BE platform and just rip all the
cpu_to_le32()usage out...including the one I submitted the patch for. Either way, that change should go in. Right?
Quoted 6 lines
Quoted 2 lines
Well, not really; the problem is it's not complete ... it only covers up the real problem by silencing the warning.
After looking at the code for a bit, I think I would prefer to disable the driver entirely for BE platforms since it's pretty clear it could never work correctly. I'm not willing to own it for BE platforms.
Unfortunately, there's no way to do that which won't have the random configuration people after you with a big stick ... we don't have a config option for BE, only a runtime option ... make the compile break and they'll find it and bug report it.
Don't worry, I won't make you own the BE part of this. A reasonable fix that looks like it has a chance of working will be fine.
Quoted 17 lines
Quoted 3 lines
If the actual BE pieces of the driver worked, you could make it correct either by making senselen a u32 and leaving the
cpu_to_le32or adding it to the point at which we assign it to bufflen.Making senselen u32 would also require fixing this bit in
tulip_main():if (!(scb->mode & SCM_RSENS)) { /* not in auto req. sense mode */ ... if (scb->flags & SCF_SENSE) { u8 len; len = scb->senselen; ... scb->cdb[3] = 0; scb->cdb[4] = len; scb->cdb[5] = 0; initio_push_pend_scb(host, scb); break;
Quoted 1 line
...
Sure ... but the u32 fix doesn't work because the driver isn't BE ready. The correct fix is to trip the wrong cpu_to_leX out of it.
Quoted 9 lines
Quoted 4 lines
If you can verify my analysis of the way the driver works, then the complete fix should be pretty simple: just remove the
cpu_to_le32from everywhere except the sg list construction.I think your analysis is correct. Seems the key bits are in initio_xfer_data_in/out() routines and around the
SCF_SGhandling.But I am not interested in actually testing it.
I'm not sure anyone can. All of the initio users I know (well both of them) have x86 boxes.
Quoted 1 line
The more I look at this code, the less I want to do with it.
OK ... just fix what we currently know is broken and I'll find some other willing victim^Wvolunteer if someone actually finds that it still doesn't work.
Quoted 5 lines
Another similar issue with bufptr usage in
tulip_main():scb->bufptr = scb->senseptr;is pushing a LE32 into native byte pointer used in
initio_state_5():
No ... bufptr is transmitted directly via outl, so it should be a CPU native variable. The only bus native variables we have in the entire driver are the SG list elements.
Quoted 8 lines
scb->bufptr += ((u32) (i - scb->sgidx) << 3);and
scb->bufptr += (u32) xcnt;bufptr will be converted to "Bus Endianess" when finally written to the controller:
outl(scb->bufptr, host->addr + TUL_XAddH);
Right, so we strip the spurious cpu_to_leX and keep everything CPU native (apart from the SG list entries).
Quoted 5 lines
I also expected bugs with
initio_host->semaph. I initially thought there would be memory ordering issues and needed to be declared "volatile". But seems to be properly protected by a spinlock.Nit:
i91u_intr()could usespin_lock()instead ofspinlock_irqsave().
The call chains for that one are pretty deep ... are you sure? I agree it looks like it should be re-entrant against other interrupts, but I wouldn't bet the farm on it ... and if we're wrong the x86 users will see corruption.
James
On Mon, Mar 31, 2008 at 7:56 AM, James Bottomley
<[email protected]> wrote:
...
Quoted 11 lines
Quoted 3 lines
After looking at the code for a bit, I think I would prefer to disable the driver entirely for BE platforms since it's pretty clear it could never work correctly. I'm not willing to own it for BE platforms.
Unfortunately, there's no way to do that which won't have the random configuration people after you with a big stick ... we don't have a config option for BE, only a runtime option ... make the compile break and they'll find it and bug report it.
Don't worry, I won't make you own the BE part of this. A reasonable fix that looks like it has a chance of working will be fine.
Ok - I'll cook up a new patch that fixes the additional issues in the next day or two. This means senselen will remain u8 and only the sg list handling will use cpu_to_le32.
thanks,
grant
On Fri, Mar 28, 2008 at 8:09 PM, James Bottomley
<[email protected]> wrote:
...
Quoted 3 lines
If you can verify my analysis of the way the driver works, then the complete fix should be pretty simple: just remove the
cpu_to_le32from everywhere except the sg list construction.
When I started striping out cpu_to_le32, I found one more nit:
if (cmnd->SCp.ptr) {
dma_unmap_single(&pci_dev->dev,
(dma_addr_t)((unsigned long)cmnd->SCp.ptr),
SENSE_SIZE, DMA_FROM_DEVICE);
cmnd->SCp.ptr = NULL;
}cmnd->SCp.ptr can be assigned a NULL value that is a perfectly valid DMA address (possibly from an IOMMU). I'm not going to attempt to fix this since it's obviously working....just want to point it out.
grant
Quoted 6 lines · Grant Grundler
On Fri, Mar 28, 2008 at 8:09 PM, James Bottomley
<[email protected]> wrote:
...Quoted 3 lines
If you can verify my analysis of the way the driver works, then the complete fix should be pretty simple: just remove the
cpu_to_le32from everywhere except the sg list construction.
Ok...here's a patch that builds clean on parisc (Big endian) and I *think* addresses the issues we found.
thanks,
grant
---
Most of the cpu_to_le32() usage was wrong in one way or another. Compiler warning on BE builds was just the tip of the iceberg. This patch attempts to make this driver work on BE though I don't have the HW to test it.
drivers/scsi/initio.c +5 -4
@@ -2581,8 +2581,8 @@ static void initio_build_scb(struct initio_host * host, struct scsi_ctrl_blk * c/* Map the sense buffer into bus memory */ dma_addr =dma_map_single (&host->pci_dev->dev, cmnd->sense_buffer,SENSE_SIZE ,DMA_FROM_DEVICE ); - cblk->senseptr =cpu_to_le32 ((u32 )dma_addr); - cblk->senselen =cpu_to_le32 (SENSE_SIZE ); + cblk->senseptr = (u32 )dma_addr; + cblk->senselen =SENSE_SIZE ; cmnd->SCp.ptr = (char *)(unsigned long )dma_addr; cblk->cdblen = cmnd->cmd_len;
@@ -2606,7 +2606,7 @@ static void initio_build_scb(struct initio_host * host, struct scsi_ctrl_blk * c dma_addr =dma_map_single (&host->pci_dev->dev, &cblk->sglist[0 ],sizeof (struct sg_entry ) *TOTAL_SG_ENTRY ,DMA_BIDIRECTIONAL ); - cblk->bufptr =cpu_to_le32 ((u32 )dma_addr); + cblk->bufptr = (u32 )dma_addr; cmnd->SCp.dma_handle = dma_addr; cblk->sglen = nseg;
@@ -2616,7 +2616,8 @@ static void initio_build_scb(struct initio_host * host, struct scsi_ctrl_blk * c sg = &cblk->sglist[0 ];scsi_for_each_sg (cmnd, sglist, cblk->sglen, i) { sg->data =cpu_to_le32 ((u32 )sg_dma_address (sglist)); - total_len += sg->len =cpu_to_le32 ((u32 )sg_dma_len (sglist)); + sg->len =cpu_to_le32 ((u32 )sg_dma_len (sglist)); + total_len +=sg_dma_len (sglist); ++sg; }