Messages in this thread | | | Date | Fri, 31 Oct 2003 12:25:08 +0100 (MET) | From | "Guennadi Liakhovetski" <> | Subject | Re: [PATCH] Re: AMD 53c974 SCSI driver in 2.6 |
| |
(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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |