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

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

[Tomas Martisius]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

On Sun, 26 Oct 2003, Guennadi Liakhovetski posted patch for
AM53c974 driver. May be it is not perfect, but it is still not applied
to kernel source. I think it is better to have at least not broken
driver compared with existing.
I have Compaq deskpro XL 590 PC with integrated such controler, and this
driver affter applinig Guennadi Liakhovetski pach to 2.6.0-test11 source
works for me.

Best regards

Tomas Martisius
VMU network administrator
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE/2DJcAMslIG6CXk4RAtb9AJ98s5l+T7IqACGrp1l/Mv7PBxuFtQCg5g+0
rwpS2VetZGl8rS49hXE2aPM=
=EYze
-----END PGP SIGNATURE-----

[-- Attachment #2: AM53C974.diff.gz --]
[-- Type: application/x-tar, Size: 4888 bytes --]

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

[Christoph Hellwig]

On Thu, Dec 11, 2003 at 11:01:16AM +0200, Tomas Martisius wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hello,
> 
> On Sun, 26 Oct 2003, Guennadi Liakhovetski posted patch for
> AM53c974 driver. May be it is not perfect, but it is still not applied
> to kernel source. I think it is better to have at least not broken
> driver compared with existing.
> I have Compaq deskpro XL 590 PC with integrated such controler, and this
> driver affter applinig Guennadi Liakhovetski pach to 2.6.0-test11 source
> works for me.

The patch is broken but he has a better patch for the other driver for
that hardware, that will hopefully go into 2.6.1.

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

[Guennadi Liakhovetski]

On Thu, 11 Dec 2003, Christoph Hellwig wrote:

> On Thu, Dec 11, 2003 at 11:01:16AM +0200, Tomas Martisius wrote:
> > On Sun, 26 Oct 2003, Guennadi Liakhovetski posted patch for
> > AM53c974 driver. May be it is not perfect, but it is still not applied
> > to kernel source. I think it is better to have at least not broken
> > driver compared with existing.
> > I have Compaq deskpro XL 590 PC with integrated such controler, and this
> > driver affter applinig Guennadi Liakhovetski pach to 2.6.0-test11 source
> > works for me.
>
> The patch is broken but he has a better patch for the other driver for
> that hardware, that will hopefully go into 2.6.1.

Right, I think, you can safely use the patch for AM53C974 for now,
although it is broken, it should work for you (I have a slightly newer
Deskpro XL 5133), wait for 2.6.1 and just enable tmscsim:-)

Guennadi
---
Guennadi Liakhovetski

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

[Guennadi Liakhovetski]

> > > Any reason you fix this driver?  The tmcsim one for the same hardware
> > > looks like much better structured (though a bit obsufacted :))?

Ok, started looking at the tmscsim. A couple of questions:

1) After the "next" element has disappeared from the struct scsi_cmnd,
what is the "correct" / preferred way to queue scsi commands in drivers?
I saw aic7xxx (new) casting a part of struct scsi_pointer SCp in
scsi_cmnd, starting from Status to a list_head (or an anology thereof),
which doesn't seem very nice. Anyway, I didn't find any "standard" way for
doing this. Should host_scribble be used?

2) Actually, which scsi driver (or, better, several drivers) can be
considered well-written and can be taken as examples? I tried looking at
aic7xxx, as it is a pretty new one, but I am not sure if it is really a
good example to follow and it is pretty big too.

Thanks
Guennadi

P.S. Should this thread be taken to linux-scsi or is it better to continue
it on lkml?
---
Guennadi Liakhovetski

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

[Russell King]

On Fri, Oct 31, 2003 at 01:19:56PM +0100, Guennadi Liakhovetski wrote:
> > Oh.  What driver did you find this construct in?
> 
> grep... (maybe not all of them are real hits, but, most of them do
> certainly look very much like what I've done). BTW, I also saw arrays of
> cards in some drvers, which also doesn't correspond to your suggestions:
> 
> drivers/scsi/arm/scsi.h-41-		SCp->ptr = (char *)
> drivers/scsi/arm/scsi.h:42:			 (page_address(SCp->buffer->page) +
> drivers/scsi/arm/scsi.h-43-			  SCp->buffer->offset);
> --
> drivers/scsi/arm/scsi.h-79-		SCpnt->SCp.ptr = (char *)
> drivers/scsi/arm/scsi.h:80:			 (page_address(SCpnt->SCp.buffer->page) +
> drivers/scsi/arm/scsi.h-81-			  SCpnt->SCp.buffer->offset);

The drivers which use this will never be used on highmem-capable machines.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

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

[Guennadi Liakhovetski]

On Fri, 31 Oct 2003, Christoph Hellwig wrote:

> On Fri, Oct 31, 2003 at 12:25:08PM +0100, Guennadi Liakhovetski wrote:
> > > > +#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 :)

Grrrr... Commented out code...

> cli() disabled all intereupts in 2.4 which was safe, you only disable local

Auch, ok, forgot, that cli() was global in 2.4, thanks for reminding, I
don't work with SMPs due to the lack thereof:-(

> Oh.  What driver did you find this construct in?

grep... (maybe not all of them are real hits, but, most of them do
certainly look very much like what I've done). BTW, I also saw arrays of
cards in some drvers, which also doesn't correspond to your suggestions:

drivers/scsi/arm/scsi.h-41-		SCp->ptr = (char *)
drivers/scsi/arm/scsi.h:42:			 (page_address(SCp->buffer->page) +
drivers/scsi/arm/scsi.h-43-			  SCp->buffer->offset);
--
drivers/scsi/arm/scsi.h-79-		SCpnt->SCp.ptr = (char *)
drivers/scsi/arm/scsi.h:80:			 (page_address(SCpnt->SCp.buffer->page) +
drivers/scsi/arm/scsi.h-81-			  SCpnt->SCp.buffer->offset);
--
drivers/scsi/sg.c-1071-	return (sclp && sclp->page) ?
drivers/scsi/sg.c:1072:	    (unsigned char *) page_address(sclp->page) + sclp->offset : NULL;
--
drivers/scsi/st.c:3517:		res = copy_from_user(page_address(st_bp->frp[i].page) + offset, ubp, cnt);
--
drivers/scsi/st.c:3550:		res = copy_to_user(ubp, page_address(st_bp->frp[i].page) + offset, cnt);
--
drivers/scsi/st.c:3593:		memmove(page_address(st_bp->frp[dst_seg].page) + dst_offset,
drivers/scsi/st.c:3594:			page_address(st_bp->frp[src_seg].page) + src_offset, count);
--
drivers/scsi/scsi_debug.c:260:		return (unsigned char *)page_address(sclp->page) +
drivers/scsi/scsi_debug.c-261-		       sclp->offset;
--
drivers/scsi/in2000.c:377:		cmd->SCp.ptr = (char *) page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/in2000.c:769:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/ide-scsi.c:149:		buf = page_address(pc->sg->page) + pc->sg->offset;
--
drivers/scsi/ide-scsi.c:171:		buf = page_address(pc->sg->page) + pc->sg->offset;
--
drivers/scsi/NCR53C9x.c:924:				(char *) virt_to_phys((page_address(sp->SCp.buffer->page) + sp->SCp.buffer->offset));
--
drivers/scsi/NCR53C9x.c:1758:		sp->SCp.ptr = (char *) virt_to_phys((page_address(sp->SCp.buffer->page) + sp->SCp.buffer->offset));
--
drivers/scsi/imm.c:840:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/imm.c:981:	    cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/ips.c:215:#define IPS_SG_ADDRESS(sg)      (page_address((sg)->page) ? \
drivers/scsi/ips.c:216:                                     page_address((sg)->page)+(sg)->offset : 0)
--
drivers/scsi/ppa.c:752:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/ppa.c:903:	    cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/qlogicfas.c:426:				buf = page_address(sglist->page) + sglist->offset;
--
drivers/scsi/sun3x_esp.c:349:	    sg[sz].dvma_address = dvma_map((unsigned long)page_address(sg[sz].page) +
drivers/scsi/sun3x_esp.c-350-					   sg[sz].offset, sg[sz].length);
--
drivers/scsi/aha1542.c:72:	       page_address(sgpnt[badseg].page) + sgpnt[badseg].offset,
--
drivers/scsi/aha1542.c:719:					       (page_address(sgpnt[i].page) +
drivers/scsi/aha1542.c-720-						sgpnt[i].offset),
--
drivers/scsi/aha152x.c:584:#define SG_ADDRESS(buffer)	((char *) (page_address((buffer)->page)+(buffer)->offset))
--
drivers/scsi/oktagon_esp.c:559:        sp->SCp.ptr = page_address(sp->SCp.buffer->page)+
drivers/scsi/oktagon_esp.c-560-		      sp->SCp.buffer->offset;
--
drivers/scsi/oktagon_esp.c:573:	sp->SCp.ptr = page_address(sp->SCp.buffer->page)+
drivers/scsi/oktagon_esp.c-574-		      sp->SCp.buffer->offset;
--
drivers/scsi/megaraid.c:2040:					page_address((&sgl[0])->page) +
drivers/scsi/megaraid.c-2041-					(&sgl[0])->offset;
--
drivers/scsi/fd_mcs.c:1024:					current_SC->SCp.ptr = page_address(current_SC->SCp.buffer->page) + current_SC->SCp.buffer->offset;
--
drivers/scsi/fd_mcs.c:1057:				current_SC->SCp.ptr = page_address(current_SC->SCp.buffer->page) + current_SC->SCp.buffer->offset;
--
drivers/scsi/fd_mcs.c:1160:		current_SC->SCp.ptr = page_address(current_SC->SCp.buffer->page) + current_SC->SCp.buffer->offset;
--
drivers/scsi/dc395x.c:238:#define CPU_ADDR(sg)		(page_address((sg).page)+(sg).offset)
--
drivers/scsi/sun3_NCR5380.c:273:#define SGADDR(buffer) (void *)(((unsigned long)page_address((buffer)->page)) + \
drivers/scsi/sun3_NCR5380.c-274-			(buffer)->offset)
--
drivers/scsi/seagate.c:989:					     page_address(buffer[i].page) + buffer[i].offset,
--
drivers/scsi/seagate.c:996:			data = page_address(buffer->page) + buffer->offset;
--
drivers/scsi/seagate.c:1243:					data = page_address(buffer->page) + buffer->offset;
--
drivers/scsi/seagate.c:1417:					data = page_address(buffer->page) + buffer->offset;
--
drivers/scsi/fdomain.c:1294:	       current_SC->SCp.ptr = page_address(current_SC->SCp.buffer->page) + current_SC->SCp.buffer->offset;
--
drivers/scsi/fdomain.c:1327:	    current_SC->SCp.ptr = page_address(current_SC->SCp.buffer->page) + current_SC->SCp.buffer->offset;
--
drivers/scsi/fdomain.c:1413:      current_SC->SCp.ptr              = page_address(current_SC->SCp.buffer->page) + current_SC->SCp.buffer->offset;
--
drivers/scsi/osst.c:4280:		STp->buffer->aux = (os_aux_t *) (page_address(STp->buffer->sg[i].page) + OS_DATA_SIZE - b_size);
--
drivers/scsi/osst.c:5146:		res = copy_from_user(page_address(st_bp->sg[i].page) + offset, ubp, cnt);
--
drivers/scsi/osst.c:5179:		res = copy_to_user(ubp, page_address(st_bp->sg[i].page) + offset, cnt);
--
drivers/scsi/osst.c:5212:		memset(page_address(st_bp->sg[i].page) + offset, 0, cnt);
--
drivers/scsi/NCR53c406a.c:884:					NCR53c406a_pio_write(page_address(sglist->page) + sglist->offset, sglist->length);
--
drivers/scsi/NCR53c406a.c:911:					NCR53c406a_pio_read(page_address(sglist->page) + sglist->offset, sglist->length);
--
drivers/scsi/atari_NCR5380.c:472:	 virt_to_phys(page_address(cmd->SCp.buffer[1].page)+
drivers/scsi/atari_NCR5380.c-473-		      cmd->SCp.buffer[1].offset) == endaddr; ) {
--
drivers/scsi/atari_NCR5380.c:510:	cmd->SCp.ptr = (char *)page_address(cmd->SCp.buffer->page)+
drivers/scsi/atari_NCR5380.c-511-		       cmd->SCp.buffer->offset;
--
drivers/scsi/atari_NCR5380.c:2044:		    cmd->SCp.ptr = page_address(cmd->SCp.buffer->page)+
drivers/scsi/atari_NCR5380.c-2045-				   cmd->SCp.buffer->offset;
--
drivers/scsi/sym53c416.c:200:#define SG_ADDRESS(buffer)     ((char *) (page_address((buffer)->page)+(buffer)->offset))
--
drivers/scsi/53c7xx.c:3453:	    ? (u32)page_address(((struct scatterlist *)cmd->buffer)[i].page)+
drivers/scsi/53c7xx.c-3454-	      ((struct scatterlist *)cmd->buffer)[i].offset
--
drivers/scsi/53c7xx.c:5420:    	    	     buffers && !((found = ((ptr >= (char *)page_address(segment->page)+segment->offset) &&
drivers/scsi/53c7xx.c:5421:    	    	    	    (ptr < ((char *)page_address(segment->page)+segment->offset+segment->length)))));
--
drivers/scsi/53c7xx.c:5425:			cmd->device->host->host_no, saved, page_address(segment->page+segment->offset);
--
drivers/scsi/53c7xx.c:5429:    	    	    offset += ptr - ((char *)page_address(segment->page)+segment->offset);
--
drivers/scsi/eata_pio.c:184:			SCp->ptr = page_address(SCp->buffer->page) + SCp->buffer->offset;
--
drivers/scsi/eata_pio.c:417:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
--
drivers/scsi/NCR5380.c:337:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page)+
drivers/scsi/NCR5380.c-338-			       cmd->SCp.buffer->offset;
--
drivers/scsi/NCR5380.c:2338:					cmd->SCp.ptr = page_address(cmd->SCp.buffer->page)+
drivers/scsi/NCR5380.c-2339-						       cmd->SCp.buffer->offset;
--
drivers/scsi/wd33c93.c:388:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) +
drivers/scsi/wd33c93.c-389-		    cmd->SCp.buffer->offset;
--
drivers/scsi/wd33c93.c:721:		cmd->SCp.ptr = page_address(cmd->SCp.buffer->page) +
drivers/scsi/wd33c93.c-722-		    cmd->SCp.buffer->offset;
--
drivers/scsi/advansys.c:6728:		(unsigned char *)page_address(slp->page) + slp->offset));
--
drivers/scsi/advansys.c:6987:                   (unsigned char *)page_address(slp->page) + slp->offset));

Thanks
Guennadi
---------------------------------
Guennadi Liakhovetski, Ph.D.
DSA Daten- und Systemtechnik GmbH
Pascalstr. 28
D-52076 Aachen
Germany

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

[Christoph Hellwig]

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?

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

[Guennadi Liakhovetski]

(Replying fromo the web-interface, hope, the email will not be scrued up 
completely).

> 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

at merging it inthe kernel (also given the current freeze). But now, if Kurt
doesn't mind, I can try fixing the other driver, and, hopefully, with your
help, I'll try to do it properly.

> > +#include <linux/version.h>
> 
> What do you need version.h for?

Yep, I had my changes encapsulated in #if KERNEL_VERSION... first, but then 
I removed them, but forgot to remove this include.

> > +#undef AM53C974_MULTIPLE_CARD
> > +#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:-)

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

> >  static void AM53C974_print(struct Scsi_Host *instance)
> >  {
> >  	AM53C974_local_declare();
> > +#if 0 /* Called only from error-handling paths with sufficient
> protection? */
> >  	unsigned long flags;
> > +#endif
> 
> So don't if 0 it but completely kill it.

There's a "?" there:-) Which means, I wasn't quite sure, so, is my
assumption correct? I just saw, that other drivers do not implement any 
locking in these functions.

> >  /* Set up an interrupt handler if we aren't already sharing an IRQ with
> another board */
> > +#ifdef AM53C974_MULTIPLE_CARD
> >  	for (search = first_host;
> >  	     search && (((the_template != NULL) && (search->hostt !=
> the_template)) ||
> >  		 (search->irq != instance->irq) || (search == instance));
> >  	     search = search->next);
> >  	if (!search) {
> > +#endif
> 
> 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.

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

So, thanks a lot for commenting on the patch, and, if nobody minds, I'll try
to fix the tmcsim driver, will try to do it better this time:-)

Thanks
Guennadi
---
Guennadi Liakhovetski

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

[Christoph Hellwig]

On Fri, Oct 31, 2003 at 10:59:10AM +0100, Kurt Garloff wrote:
> On Fri, Oct 31, 2003 at 09:47:42AM +0000, Christoph Hellwig wrote:
> > Any reason why we'd keep both drivers?  Given that there's not much ressources
> > for fixing drivers for older hardware I'd rather see us not keeping multiple
> > drivers for the same hardware.
> 
> As long as there are people willing to do the work, there's no reason to
> drop any of them.

Well, yes.  If we had two properly maintained drivers there would be
no reason to drop one.  But we currently have two broken drivers, one of
them just resurrected to compiling and mostly working on UP state..

--
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

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

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

Christoph,

On Fri, Oct 31, 2003 at 09:47:42AM +0000, Christoph Hellwig wrote:
> Any reason why we'd keep both drivers?  Given that there's not much ressources
> for fixing drivers for older hardware I'd rather see us not keeping multiple
> drivers for the same hardware.

As long as there are people willing to do the work, there's no reason to
drop any of them.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Christoph Hellwig]

On Fri, Oct 31, 2003 at 12:52:04AM +0100, Kurt Garloff wrote:
> Interested in taking tmscsim as well?
> I promised to port it, and I will, but it won't happen during the next 
> ten days :-(

Any reason why we'd keep both drivers?  Given that there's not much ressources
for fixing drivers for older hardware I'd rather see us not keeping multiple
drivers for the same hardware.

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

[Christoph Hellwig]

Any reason you fix this driver?  The tmcsim one for the same hardware
looks like much better structured (though a bit obsufacted :))?

> +#include <linux/version.h>

What do you need version.h for?

> +#undef AM53C974_MULTIPLE_CARD
> +#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 :)

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

>  static void AM53C974_print(struct Scsi_Host *instance)
>  {
>  	AM53C974_local_declare();
> +#if 0 /* Called only from error-handling paths with sufficient protection? */
>  	unsigned long flags;
> +#endif

So don't if 0 it but completely kill it.

>  /* Set up an interrupt handler if we aren't already sharing an IRQ with another board */
> +#ifdef AM53C974_MULTIPLE_CARD
>  	for (search = first_host;
>  	     search && (((the_template != NULL) && (search->hostt != the_template)) ||
>  		 (search->irq != instance->irq) || (search == instance));
>  	     search = search->next);
>  	if (!search) {
> +#endif

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

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

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

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

Guennadi,

Great!

On Thu, Oct 30, 2003 at 11:12:57PM +0100, Guennadi Liakhovetski wrote:
> Ok, I fixed it, well, at least, it works for me. What now? The fix is,
> probably, not perfect. E.g. it doesn't support multiple cards now, but it
> looks like the driver didn't support them even when it worked in its
> latest version (sorry, if I am wrong).
> 
> Patch attached. Comments / improvement suggestions mostly welcome.

I think it should just be merged; Christoph will certainly find places
to criticize ... but that's the way the stuff gets better.

Interested in taking tmscsim as well?
I promised to port it, and I will, but it won't happen during the next 
ten days :-(

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Guennadi Liakhovetski]

On Sun, 26 Oct 2003, Guennadi Liakhovetski wrote:

> > The above (AM53C974) driver depends on BROKEN in 2.6 and doesn't compile.
> > Is anybody working on / planning to fix it?

Ok, I fixed it, well, at least, it works for me. What now? The fix is,
probably, not perfect. E.g. it doesn't support multiple cards now, but it
looks like the driver didn't support them even when it worked in its
latest version (sorry, if I am wrong).

Patch attached. Comments / improvement suggestions mostly welcome.

Guennadi
---
Guennadi Liakhovetski

diff -ur linux-2.6.0-test7.arm/drivers/scsi/AM53C974.c linux-2.6.0-test7/drivers/scsi/AM53C974.c
--- linux-2.6.0-test7.arm/drivers/scsi/AM53C974.c	Wed Oct  8 23:26:43 2003
+++ linux-2.6.0-test7/drivers/scsi/AM53C974.c	Thu Oct 30 23:00:40 2003
@@ -1,6 +1,5 @@
-#error Please convert me to Documentation/DMA-mapping.txt
-
 #include <linux/module.h>
+#include <linux/version.h>
 #include <linux/delay.h>
 #include <linux/signal.h>
 #include <linux/sched.h>
@@ -10,6 +9,7 @@
 #include <linux/blkdev.h>
 #include <linux/init.h>
 #include <linux/spinlock.h>
+#include <linux/interrupt.h>

 #include <asm/io.h>
 #include <asm/system.h>
@@ -361,8 +361,8 @@
 static __inline__ void initialize_SCp(Scsi_Cmnd * cmd);
 static __inline__ void run_main(void);
 static void AM53C974_main(void);
-static void AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs);
-static void do_AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs);
+static irqreturn_t AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs);
+static irqreturn_t do_AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs);
 static void AM53C974_intr_disconnect(struct Scsi_Host *instance);
 static int AM53C974_sync_neg(struct Scsi_Host *instance, int target, unsigned char *msg);
 static __inline__ void AM53C974_set_async(struct Scsi_Host *instance, int target);
@@ -382,7 +382,11 @@

 static struct Scsi_Host *first_instance;
 static Scsi_Host_Template *the_template;
+#undef AM53C974_MULTIPLE_CARD
+#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
 static volatile int main_running;
 static int commandline_current;
 override_t overrides[7] =
@@ -457,8 +461,7 @@

 	printk("AM53C974: coroutine is%s running.\n", main_running ? "" : "n't");

-	save_flags(flags);
-	cli();
+	local_irq_save(flags);

 	if (!hostdata->connected) {
 		printk("scsi%d: no currently connected command\n", instance->host_no);
@@ -489,7 +492,7 @@
 			print_Scsi_Cmnd(ptr);
 	}

-	restore_flags(flags);
+	local_irq_restore(flags);
 }

 #endif				/* AM53C974_DEBUG */
@@ -504,14 +507,17 @@
 static void AM53C974_print(struct Scsi_Host *instance)
 {
 	AM53C974_local_declare();
+#if 0 /* Called only from error-handling paths with sufficient protection? */
 	unsigned long flags;
+#endif
 	unsigned long ctcreg, dmastc, dmaspa, dmawbc, dmawac;
 	unsigned char cmdreg, statreg, isreg, cfireg, cntlreg[4], dmacmd,
 	 dmastatus;
 	AM53C974_setio(instance);

-	save_flags(flags);
-	cli();
+#if 0 /* Called only from error-handling paths with sufficient protection? */
+	local_irq_save(flags);
+#endif
 	ctcreg = AM53C974_read_8(CTCHREG) << 16;
 	ctcreg |= AM53C974_read_8(CTCMREG) << 8;
 	ctcreg |= AM53C974_read_8(CTCLREG);
@@ -529,7 +535,9 @@
 	dmawbc = AM53C974_read_32(DMAWBC);
 	dmawac = AM53C974_read_32(DMAWAC);
 	dmastatus = AM53C974_read_8(DMASTATUS);
-	restore_flags(flags);
+#if 0 /* Called only from error-handling paths with sufficient protection? */
+	local_irq_restore(flags);
+#endif

 	printk("AM53C974 register dump:\n");
 	printk("IO base: 0x%04lx; CTCREG: 0x%04lx; CMDREG: 0x%02x; STATREG: 0x%02x; ISREG: 0x%02x\n",
@@ -559,15 +567,14 @@
 		return;
 #endif

-	save_flags(flags);
-	cli();
+	local_irq_save(flags);
 	while ((inb_p(0x64) & 0x01) != 0x01);
 #ifdef AM53C974_DEBUG
 	key = inb(0x60);
 	if (key == 0x93)
 		deb_stop = 0;	/* don't stop if 'r' was pressed */
 #endif
-	restore_flags(flags);
+	local_irq_restore(flags);
 }

 #ifndef MODULE
@@ -667,7 +674,10 @@
 {
 	AM53C974_local_declare();
 	int i, j;
-	struct Scsi_Host *instance, *search;
+	struct Scsi_Host *instance;
+#ifdef AM53C974_MULTIPLE_CARD
+	struct Scsi_Host *search;
+#endif
 	struct AM53C974_hostdata *hostdata;

 #ifdef AM53C974_OPTION_DEBUG_PROBE_ONLY
@@ -739,21 +749,24 @@
 		return 0;
 	}
 /* Set up an interrupt handler if we aren't already sharing an IRQ with another board */
+#ifdef AM53C974_MULTIPLE_CARD
 	for (search = first_host;
 	     search && (((the_template != NULL) && (search->hostt != the_template)) ||
 		 (search->irq != instance->irq) || (search == instance));
 	     search = search->next);
 	if (!search) {
+#endif
 		if (request_irq(instance->irq, do_AM53C974_intr, SA_SHIRQ, "AM53C974", instance)) {
 			printk("scsi%d: IRQ%d not free, detaching\n", instance->host_no, instance->irq);
 			scsi_unregister(instance);
 			return 0;
 		}
+#ifdef AM53C974_MULTIPLE_CARD
 	} else {
 		printk("scsi%d: using interrupt handler previously installed for scsi%d\n",
 		       instance->host_no, search->host_no);
 	}
-
+#endif
 	if (!the_template) {
 		the_template = instance->hostt;
 		first_instance = instance;
@@ -832,7 +845,7 @@
 	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;
 		cmd->SCp.this_residual = cmd->SCp.buffer->length;
 	} else {
 		cmd->SCp.buffer = NULL;
@@ -858,15 +871,14 @@
 static __inline__ void run_main(void)
 {
 	unsigned long flags;
-	save_flags(flags);
-	cli();
+ 	local_irq_save(flags);
 	if (!main_running) {
 		/* main_running is cleared in AM53C974_main once it can't do
 		   more work, and AM53C974_main exits with interrupts disabled. */
 		main_running = 1;
 		AM53C974_main();
 	}
-	restore_flags(flags);
+	local_irq_restore(flags);
 }

 /**************************************************************************
@@ -891,8 +903,7 @@
 	struct AM53C974_hostdata *hostdata = (struct AM53C974_hostdata *) instance->hostdata;
 	Scsi_Cmnd *tmp;

-	save_flags(flags);
-	cli();
+	local_irq_save(flags);
 	DEB_QUEUE(printk(SEPARATOR_LINE));
 	DEB_QUEUE(printk("scsi%d: AM53C974_queue_command called\n", instance->host_no));
 	DEB_QUEUE(printk("cmd=%02x target=%02x lun=%02x bufflen=%d use_sg = %02x\n",
@@ -924,7 +935,7 @@

 /* Run the coroutine if it isn't already running. */
 	run_main();
-	restore_flags(flags);
+	local_irq_restore(flags);
 	return 0;
 }

@@ -952,12 +963,14 @@
  * the host adapters have anything that can be done, at which point
  * we set main_running to 0 and exit. */

-	save_flags(flags);
-	cli();		/* Freeze request queues */
+	local_irq_save(flags); /* Freeze request queues */
 	do {
 		done = 1;
+#ifdef AM53C974_MULTIPLE_CARD
 		for (instance = first_instance; instance && instance->hostt == the_template;
 		     instance = instance->next) {
+#endif
+			instance = first_instance;
 			hostdata = (struct AM53C974_hostdata *) instance->hostdata;
 			AM53C974_setio(instance);
 			/* start to select target if we are not connected and not in the
@@ -993,10 +1006,12 @@
 				DEB(printk("main: connected; cmd = 0x%lx, sel_cmd = 0x%lx\n",
 					   (long) hostdata->connected, (long) hostdata->sel_cmd));
 			}
+#ifdef AM53C974_MULTIPLE_CARD
 		}		/* for instance */
+#endif
 	} while (!done);
 	main_running = 0;
-	restore_flags(flags);
+	local_irq_restore(flags);
 }

 /************************************************************************
@@ -1008,14 +1023,16 @@
 *                                                                       *
 * Returns : nothing                                                     *
 ************************************************************************/
-static void do_AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs)
+static irqreturn_t do_AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs)
 {
 	unsigned long flags;
 	struct Scsi_Host *dev = dev_id;
-
+	irqreturn_t ret;
+
 	spin_lock_irqsave(dev->host_lock, flags);
-	AM53C974_intr(irq, dev_id, regs);
+	ret = AM53C974_intr(irq, dev_id, regs);
 	spin_unlock_irqrestore(dev->host_lock, flags);
+	return ret;
 }

 /************************************************************************
@@ -1027,7 +1044,7 @@
 *                                                                       *
 * Returns : nothing                                                     *
 ************************************************************************/
-static void AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs)
+static irqreturn_t AM53C974_intr(int irq, void *dev_id, struct pt_regs *regs)
 {
 	AM53C974_local_declare();
 	struct Scsi_Host *instance;
@@ -1035,10 +1052,13 @@
 	unsigned char cmdreg, dmastatus, statreg, isreg, instreg, cfifo;

 /* find AM53C974 hostadapter responsible for this interrupt */
+#ifdef AM53C974_MULTIPLE_CARD
 	for (instance = first_instance; instance; instance = instance->next)
+#endif
+	instance = first_instance;
 		if ((instance->irq == irq) && (instance->hostt == the_template))
 			goto FOUND;
-	return;
+	return IRQ_NONE;

 /* found; now decode and process */
 	FOUND:
@@ -1065,8 +1085,7 @@
 		/* DMA transfer done */
 		unsigned long residual;
 		unsigned long flags;
-		save_flags(flags);
-		cli();
+		local_irq_save(flags);
 		if (!(AM53C974_read_8(DMACMD) & DMACMD_DIR)) {
 			do {
 				dmastatus = AM53C974_read_8(DMASTATUS);
@@ -1095,10 +1114,10 @@
 			AM53C974_information_transfer(instance, statreg, isreg, instreg, cfifo,
 						      dmastatus);
 		}
-		restore_flags(flags);
+		local_irq_restore(flags);
 	}
 	if (!(dmastatus & DMASTATUS_SCSIINT)) {
-		return;
+		return IRQ_HANDLED;
 	}
 /*** SCSI related interrupts ***/
 	cmdreg = AM53C974_read_8(CMDREG);
@@ -1138,8 +1157,7 @@
 #endif
 		DEB(printk("Bus reset interrupt received\n"));
 		AM53C974_intr_bus_reset(instance);
-		save_flags(flags);
-		cli();
+		local_irq_save(flags);
 		if (hostdata->connected) {
 			hostdata->connected->result = DID_RESET << 16;
 			hostdata->connected->scsi_done((Scsi_Cmnd *) hostdata->connected);
@@ -1151,11 +1169,11 @@
 				hostdata->sel_cmd = NULL;
 			}
 		}
-		restore_flags(flags);
+		local_irq_restore(flags);
 		if (hostdata->in_reset == 1)
 			goto EXIT;
 		else
-			return;
+			return IRQ_HANDLED;
 	}
 	if (instreg & INSTREG_ICMD) {
 		/* INVALID COMMAND INTERRUPT */
@@ -1172,20 +1190,18 @@
 		unsigned long flags;
 		/* DISCONNECT INTERRUPT */
 		DEB_INTR(printk("Disconnect interrupt received; "));
-		save_flags(flags);
-		cli();
+		local_irq_save(flags);
 		AM53C974_intr_disconnect(instance);
-		restore_flags(flags);
+		local_irq_restore(flags);
 		goto EXIT;
 	}
 	if (instreg & INSTREG_RESEL) {
 		unsigned long flags;
 		/* RESELECTION INTERRUPT */
 		DEB_INTR(printk("Reselection interrupt received\n"));
-		save_flags(flags);
-		cli();
+		local_irq_save(flags);
 		AM53C974_intr_reselect(instance, statreg);
-		restore_flags(flags);
+		local_irq_restore(flags);
 		goto EXIT;
 	}
 	if (instreg & INSTREG_SO) {
@@ -1193,15 +1209,14 @@
 		if (hostdata->selecting) {
 			unsigned long flags;
 			DEB_INTR(printk("DSR completed, starting select\n"));
-			save_flags(flags);
-			cli();
+			local_irq_save(flags);
 			AM53C974_select(instance, (Scsi_Cmnd *) hostdata->sel_cmd,
 			  (hostdata->sel_cmd->cmnd[0] == REQUEST_SENSE) ?
 					TAG_NONE : TAG_NEXT);
 			hostdata->selecting = 0;
 			AM53C974_set_sync(instance, hostdata->sel_cmd->device->id);
-			restore_flags(flags);
-			return;
+			local_irq_restore(flags);
+			return IRQ_HANDLED;
 		}
 		if (hostdata->sel_cmd != NULL) {
 			if (((isreg & ISREG_IS) != ISREG_OK_NO_STOP) &&
@@ -1209,22 +1224,20 @@
 				unsigned long flags;
 				/* UNSUCCESSFUL SELECTION */
 				DEB_INTR(printk("unsuccessful selection\n"));
-				save_flags(flags);
-				cli();
+				local_irq_save(flags);
 				hostdata->dma_busy = 0;
 				LIST(hostdata->sel_cmd, hostdata->issue_queue);
 				hostdata->sel_cmd->host_scribble = (unsigned char *) hostdata->issue_queue;
 				hostdata->issue_queue = hostdata->sel_cmd;
 				hostdata->sel_cmd = NULL;
 				hostdata->selecting = 0;
-				restore_flags(flags);
+				local_irq_restore(flags);
 				goto EXIT;
 			} else {
 				unsigned long flags;
 				/* SUCCESSFUL SELECTION */
 				DEB(printk("successful selection; cmd=0x%02lx\n", (long) hostdata->sel_cmd));
-				save_flags(flags);
-				cli();
+				local_irq_save(flags);
 				hostdata->dma_busy = 0;
 				hostdata->disconnecting = 0;
 				hostdata->connected = hostdata->sel_cmd;
@@ -1232,7 +1245,7 @@
 				hostdata->selecting = 0;
 #ifdef SCSI2
 				if (!hostdata->conneted->device->simple_tags)
-#else
+#endif
 					hostdata->busy[hostdata->connected->device->id] |= (1 << hostdata->connected->device->lun);
 				/* very strange -- use_sg is sometimes nonzero for request sense commands !! */
 				if ((hostdata->connected->cmnd[0] == REQUEST_SENSE) && hostdata->connected->use_sg) {
@@ -1243,16 +1256,15 @@
 				initialize_SCp((Scsi_Cmnd *) hostdata->connected);
 				hostdata->connected->SCp.phase = PHASE_CMDOUT;
 				AM53C974_information_transfer(instance, statreg, isreg, instreg, cfifo, dmastatus);
-				restore_flags(flags);
-				return;
+				local_irq_restore(flags);
+				return IRQ_HANDLED;
 			}
 		} else {
 			unsigned long flags;
-			save_flags(flags);
-			cli();
+			local_irq_save(flags);
 			AM53C974_information_transfer(instance, statreg, isreg, instreg, cfifo, dmastatus);
-			restore_flags(flags);
-			return;
+			local_irq_restore(flags);
+			return IRQ_HANDLED;
 		}
 	}
 	if (instreg & INSTREG_SR) {
@@ -1260,20 +1272,20 @@
 		if (hostdata->connected) {
 			unsigned long flags;
 			DEB_INTR(printk("calling information_transfer\n"));
-			save_flags(flags);
-			cli();
+			local_irq_save(flags);
 			AM53C974_information_transfer(instance, statreg, isreg, instreg, cfifo, dmastatus);
-			restore_flags(flags);
+			local_irq_restore(flags);
 		} else {
 			printk("scsi%d: weird: service request when no command connected\n", instance->host_no);
 			AM53C974_write_8(CMDREG, CMDREG_CFIFO);
 		}		/* clear FIFO */
-		return;
+		return IRQ_HANDLED;
 	}
 	EXIT:
 	    DEB_INTR(printk("intr: starting main\n"));
 	run_main();
 	DEB_INTR(printk("end of intr\n"));
+	return IRQ_HANDLED;
 }

 /**************************************************************************
@@ -1546,7 +1558,7 @@
 		if ((!cmd->SCp.this_residual) && cmd->SCp.buffers_residual) {
 			cmd->SCp.buffer++;
 			cmd->SCp.buffers_residual--;
-			cmd->SCp.ptr = (unsigned char *) cmd->SCp.buffer->address;
+			cmd->SCp.ptr = (unsigned char *) page_address(cmd->SCp.buffer->page) + cmd->SCp.buffer->offset;
 			cmd->SCp.this_residual = cmd->SCp.buffer->length;
 		}
 		if (cmd->SCp.this_residual) {
@@ -2268,7 +2280,6 @@
 static int AM53C974_abort(Scsi_Cmnd * cmd)
 {
 	AM53C974_local_declare();
-	unsigned long flags;
 	struct Scsi_Host *instance = cmd->device->host;
 	struct AM53C974_hostdata *hostdata = (struct AM53C974_hostdata *) instance->hostdata;
 	Scsi_Cmnd *tmp, **prev;
@@ -2276,8 +2287,6 @@
 #ifdef AM53C974_DEBUG
 	deb_stop = 1;
 #endif
-	save_flags(flags);
-	cli();
 	AM53C974_setio(instance);

 	DEB_ABORT(printk(SEPARATOR_LINE));
@@ -2292,7 +2301,6 @@
 		DEB_ABORT(printk("scsi%d: aborting connected command\n", instance->host_no));
 		hostdata->aborted = 1;
 		hostdata->msgout[0] = ABORT;
-		restore_flags(flags);
 		return (SCSI_ABORT_PENDING);
 	}
 /* Case 2 : If the command hasn't been issued yet,
@@ -2307,7 +2315,6 @@
 			(*prev) = (Scsi_Cmnd *) tmp->host_scribble;
 			tmp->host_scribble = NULL;
 			tmp->result = DID_ABORT << 16;
-			restore_flags(flags);
 			tmp->done(tmp);
 			return (SCSI_ABORT_SUCCESS);
 		}
@@ -2329,7 +2336,6 @@
  *          we fail. */
 	if (hostdata->connected || hostdata->sel_cmd) {
 		DEB_ABORT(printk("scsi%d : abort failed, other command connected.\n", instance->host_no));
-		restore_flags(flags);
 		return (SCSI_ABORT_NOT_RUNNING);
 	}
 /* Case 4: If the command is currently disconnected from the bus, and
@@ -2345,7 +2351,6 @@
 			hostdata->selecting = 1;
 			hostdata->sel_cmd = tmp;
 			AM53C974_write_8(CMDREG, CMDREG_DSR);
-			restore_flags(flags);
 			return (SCSI_ABORT_PENDING);
 		}
 	}
@@ -2358,7 +2363,6 @@
  * so we won't panic, but we will notify the user in case something really
  * broke. */
 	DEB_ABORT(printk("scsi%d : abort failed, command not found.\n", instance->host_no));
-	restore_flags(flags);
 	return (SCSI_ABORT_NOT_RUNNING);
 }

@@ -2370,20 +2374,15 @@
 * Inputs : cmd -- which command within the command block was responsible for the reset
 *
 * Returns : status (SCSI_ABORT_SUCCESS)
-*
-* FIXME(eric) the reset_flags are ignored.
 **************************************************************************/
-static int AM53C974_reset(Scsi_Cmnd * cmd, unsigned int reset_flags)
+static int AM53C974_reset(Scsi_Cmnd * cmd)
 {
 	AM53C974_local_declare();
-	unsigned long flags;
 	int i;
 	struct Scsi_Host *instance = cmd->device->host;
 	struct AM53C974_hostdata *hostdata = (struct AM53C974_hostdata *) instance->hostdata;
 	AM53C974_setio(instance);

-	save_flags(flags);
-	cli();
 	DEB(printk("AM53C974_reset called; "));

 	printk("AM53C974_reset called\n");
@@ -2417,10 +2416,9 @@
 	udelay(40);
 	AM53C974_config_after_reset(instance);

-	restore_flags(flags);
 	cmd->result = DID_RESET << 16;
 	cmd->scsi_done(cmd);
-	return SCSI_ABORT_SUCCESS;
+	return SCSI_RESET_SUCCESS;
 }


@@ -2451,8 +2449,8 @@
 	.release        	= AM53C974_release,
 	.info			= AM53C974_info,
 	.queuecommand		= AM53C974_queue_command,
-	.abort			= AM53C974_abort,
-	.reset			= AM53C974_reset,
+	.eh_abort_handler	= AM53C974_abort,
+	.eh_bus_reset_handler	= AM53C974_reset,
 	.can_queue		= 12,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
diff -ur linux-2.6.0-test7.arm/drivers/scsi/AM53C974.h linux-2.6.0-test7/drivers/scsi/AM53C974.h
--- linux-2.6.0-test7.arm/drivers/scsi/AM53C974.h	Sat Aug  9 06:40:41 2003
+++ linux-2.6.0-test7/drivers/scsi/AM53C974.h	Thu Oct 30 23:05:41 2003
@@ -53,9 +53,7 @@
 static int AM53C974_pci_detect(Scsi_Host_Template * tpnt);
 static int AM53C974_release(struct Scsi_Host *shp);
 static const char *AM53C974_info(struct Scsi_Host *);
-static int AM53C974_command(Scsi_Cmnd * SCpnt);
 static int AM53C974_queue_command(Scsi_Cmnd * cmd, void (*done) (Scsi_Cmnd *));
 static int AM53C974_abort(Scsi_Cmnd * cmd);
-static int AM53C974_reset(Scsi_Cmnd * cmd, unsigned int);
-
+static int AM53C974_reset(Scsi_Cmnd * cmd);
 #endif				/* AM53C974_H */