lkml.org 
[lkml]   [2003]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2005-03-22 13:58    [W:2.109 / U:0.580 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site