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

[Christoph Hellwig <hch@infradead.org>]

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?