[patch 13/17] drivers/scsi/initio.c: suppress compile warning

[patch 13/17] drivers/scsi/initio.c: suppress compile warning

[akpm]

From: Andrew Morton <akpm@linux-foundation.org>

powerpc:

drivers/scsi/initio.c: In function 'initio_build_scb':
drivers/scsi/initio.c:2585: warning: large integer implicitly truncated to unsigned type

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/scsi/initio.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/initio.c~drivers-scsi-initioc-suppress-compile-warning drivers/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 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;
 
_

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Grant Grundler]

On Fri, Mar 28, 2008 at 2:48 PM,  <akpm@linux-foundation.org> wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
>
>  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

>
>  Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>  Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>  Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>  ---
>
>   drivers/scsi/initio.c |    6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
>  diff -puN drivers/scsi/initio.c~drivers-scsi-initioc-suppress-compile-warning drivers/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 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;
>
>  _
>  --
>  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: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Andrew Morton]

On Fri, 28 Mar 2008 14:55:28 -0700
"Grant Grundler" <grundler@google.com> wrote:

> On Fri, Mar 28, 2008 at 2:48 PM,  <akpm@linux-foundation.org> wrote:
> > From: Andrew Morton <akpm@linux-foundation.org>
> >
> >  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.

Oh well, I droped this one then.

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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[James Bottomley]

On Fri, 2008-03-28 at 14:55 -0700, Grant Grundler wrote:
> On Fri, Mar 28, 2008 at 2:48 PM,  <akpm@linux-foundation.org> wrote:
> > From: Andrew Morton <akpm@linux-foundation.org>
> >
> >  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

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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Alan Cox]

> Alan, has this driver ever worked on a BE platform?

Not afaik.

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[James Bottomley]

On Fri, 2008-03-28 at 22:43 +0000, Alan Cox wrote:
> > 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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Grant Grundler]

On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
...
>  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?

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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[James Bottomley]

On Fri, 2008-03-28 at 17:49 -0700, Grant Grundler wrote:
> On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> ...
> >  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?
> 
> 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.

> 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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Grant Grundler]

On Fri, Mar 28, 2008 at 8:09 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2008-03-28 at 17:49 -0700, Grant Grundler wrote:
>  > On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
>  > <James.Bottomley@hansenpartnership.com> wrote:
>  > ...
>  > >  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?
>  >
>  > 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.

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

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

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


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

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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[James Bottomley]

On Sun, 2008-03-30 at 21:50 -0700, Grant Grundler wrote:
> On Fri, Mar 28, 2008 at 8:09 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Fri, 2008-03-28 at 17:49 -0700, Grant Grundler wrote:
> >  > On Fri, Mar 28, 2008 at 4:51 PM, James Bottomley
> >  > <James.Bottomley@hansenpartnership.com> wrote:
> >  > ...
> >  > >  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?
> >  >
> >  > 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.
> 
> >  > 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.

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.

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

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.

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

I'm not sure anyone can.  All of the initio users I know (well both of
them) have x86 boxes.

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

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

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

> 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().

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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Grant Grundler]

On Mon, Mar 31, 2008 at 7:56 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
...
>  > 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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Grant Grundler]

On Fri, Mar 28, 2008 at 8:09 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
...
>  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.

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

Re: [patch 13/17] drivers/scsi/initio.c: suppress compile warning

[Grant Grundler]

On Fri, Apr 04, 2008 at 12:05:01AM -0700, Grant Grundler wrote:
> On Fri, Mar 28, 2008 at 8:09 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> ...
> >  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.

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.

Signed-off-by: Grant Grundler <grundler@parisc-linux.org>

diff --git a/drivers/scsi/initio.c b/drivers/scsi/initio.c
index 0cc8868..dbae3fd 100644
--- a/drivers/scsi/initio.c
+++ b/drivers/scsi/initio.c
@@ -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;
 		}