Re: [PATCH] Re: AMD 53c974 SCSI driver in 2.6

[Christoph Hellwig <hch@infradead.org>]

On Fri, Oct 31, 2003 at 12:25:08PM +0100, Guennadi Liakhovetski wrote:
> (Replying fromo the web-interface, hope, the email will not be scrued up 
> completely).

Seems fine.

> > Any reason you fix this driver?  The tmcsim one for the same hardware
> > looks like much better structured (though a bit obsufacted :))?
> 
> This is the easiest to asnwer: Kurt Garloff wrote to me, saying that he 
> would fix the tmcsim driver, so, I didn't want to duplicate the work. Also, 
> I was considering this mostly as a fun-excersice, not really hoping / aiming

Ah, okay.

> > > +#ifdef AM53C974_MULTIPLE_CARD
> > > +#error "FIXME! Multiple card support is broken. Looks like it never
> > really worked. Might have to be fixed."
> > >  static struct Scsi_Host *first_host;	/* Head of list of AMD boards */
> > > +#endif
> > 
> > Why do you need the undef?  It looks like you need to kill a #define for
> > this symbol somewhere else :)
> 
> So that, when it is fixed, somebody can easily switch it on / off:-)

Then just comment ou the #define.  Rule of the thumb:  #undef always means
you screwed up elsewhere :)

> 
> > > -	save_flags(flags);
> > > -	cli();
> > > +	local_irq_save(flags);
> > 
> > That's not safe on SMP, you must mark the driver BROKEN_ON_SMP or better
> > fix this.
> 
> Yes. Again - the fix was pretty much mechanical. I didn't understand why it
> was considered SMP-safe to just disable local interrupts, so, just preferred
> to go the "minimal modifications" path.

cli() disabled all intereupts in 2.4 which was safe, you only disable local
interrupts which is ok on UP but doesn't work on SMP.  The right fix would
be to introduce spinlocks.  But given the driver design this might be
everything but easy - that's why I think killing the driver in favour of tmcsim
might be the better idea.

> > Not sure whether you're interested in fixing this, but the proper way
> > to fix that would be to call request_irq for each card, mark the irq's
> > sharable.  The irq handler then can use the void * argument to find the
> > right host and you can kill all this ugly host list walking.  That shold
> > get multiple host support working again in theory  (ok, except that the
> > driver has a totally broken single state machine..)
> 
> Sure - in irq-handler. But are these lists needed anywhere else, where a
> function is called in an "abstract" context without a dev-pointer? Probably,
> not.

A poper driver shouldn't do that, but we already know this one isn't..

> > >  	if (cmd->use_sg) {
> > >  		cmd->SCp.buffer = (struct scatterlist *) cmd->buffer;
> > >  		cmd->SCp.buffers_residual = cmd->use_sg - 1;
> > > -		cmd->SCp.ptr = (char *) cmd->SCp.buffer->address;
> > > +		cmd->SCp.ptr = (char *) page_address(cmd->SCp.buffer->page) +
> > cmd->SCp.buffer->offset;
> > 
> > This means you need a dma_mask < highmem to work.  I don't think we
> > want such crude hacks merged, could you please convert it to the proper
> > dma API?
> 
> Found in a "working" driver (which has to be fixed too, then). Will try to 
> find a proper fix (for the new driver).

Oh.  What driver did you find this construct in?