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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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