From: linas@austin.ibm.com (linas)
Subject: Re: [PATCH] PCI Error Recovery: Symbios SCSI device driver
Date: Wed, 18 Jan 2006 11:54:08 -0600
Message-ID: <20060118175408.GS26221@austin.ibm.com>
References: <20060118165345.GA31920@austin.ibm.com> <20060118170716.GA9569@parisc-linux.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20060118170716.GA9569@parisc-linux.org>
Sender: linux-scsi-owner@vger.kernel.org
To: Matthew Wilcox <matthew@wil.cx>
Cc: James.Bottomley@steeleye.com, brking@us.ibm.com, gregkh@suse.de, linux-scsi@vger.kernel.org

On Wed, Jan 18, 2006 at 10:07:16AM -0700, Matthew Wilcox was heard to remark:
> On Wed, Jan 18, 2006 at 10:53:45AM -0600, linas wrote:
> > Hi James,
> 
> Actually, I'm the sym2 maintainer.

Ah, sorry, then.

> > +static void sym_eeh_timeout(u_long p)
> 
> Please don't propagate more uses of this u_long crap.  I haven't got
> rid of all of them, but there's no need to add more.

I was trying to follow the coding style as currently found in 
that file; the other routines of this type use u_long.  Should
I use u_int instead? or unsigned long? 

> > @@ -1630,6 +1682,8 @@ static struct Scsi_Host * __devinit sym_
> >  	np->maxoffs	= dev->chip.offset_max;
> >  	np->maxburst	= dev->chip.burst_max;
> >  	np->myaddr	= dev->host_id;
> > +	np->s.io_state = pci_channel_io_normal;
> > +	np->s.io_reset_wait = NULL;
> 
> Is there a good reason to put them in the 's'?  It's really a dirty
> hack.

The struct sym_shcb is defined in a file "sym_glue.h" which seems to
contain linux-kernel specific code.  The comments before this struct 
state that its a "System specific host data structure."

By contrast, struct sym_hcb is defined in "sym_hipd.h", which appears
to have no Linux-specific structs in it. Thus, I assumed that it 
should go into the glue struct rather than the main struct.  I can
of course add it to sym_hcb if that is a better location.

> I really hate "if (constant == variable)".

Sorry, I'll fix it. I find it easier on the eyes to read that way. 
Old habit.  (It has the marginal benefit of flagging as a compiler 
error any accidental drop of an =, such as if (variable = constant) 
which is usually not what the coder intended).

I'll fix the other bits you mentioned as well.

--linas