"Justin T. Gibbs" <gibbs@freefall.freebsd.org> writes:

> 
> I've also
> started my revamp of the MI SCSI layer and have completely solved this
> problem with a queue based approach.

I'm looking at these changes, and I have several comments:

0) I was under the (perhaps mistaken) impression that we had a mailing
list to discuss these changes.

1) What is the point of passing down the scsi_queue to the *_get_ccb()
and *_free_ccb() routines?  What would they use it for?

2) Why isn't the scsi_queue allocated by the bus, rather than by the
controller (i.e. by scsi_alloc_bus(), in your configuration `model')?
(Presumably the function pointers would be passed down in the *_switch
structure.)

3) If you're going to do things like:

!                       scsi_xs_started(xs);
!                       scsi_done(xs);

I'd *much* rather have another function for it.  Although I'm fairly
certain that this is the wrong thing to be doing anyway.  (See below.)

4) Related to the previous, under precisely what conditions is the
*_scsi_cmd() routine *not* supposed to call scsi_xs_started()?  Are
there any?  (If not, why isn't that call done before even calling
*_scsi_cmd()?  If you need a lock on the target, it should be
separate, and should either be done explicitly from *_scsi_cmd()
*after the CDB is actually sent to the adapter* (or at least
reserved), or just automatically when the routine finishes.  This is
not currently the case.)

5) Passing around spl cookies as function arguments is just wrong.

6) Since you're already poking things into the xs, it seems rather
silly to pass a bunch of things to scsi_prepare_xs() that you could
just poke in.  Especially given that it just triples the overhead for
no reason.

7) I'm not sure where to even *begin* describing how much of an
abstraction violation this is:

+       if (xs->datalen && ((caddr_t) xs->data < (caddr_t) KERNBASE)) {
+ [...]

8) The use of `_scsi_flags' to deal with autoconfiguration is highly
questionable.  The old method of passing around the flags worked, and
didn't create a hidden interface.

9) It's unclear to me that actually keeping a pointer to the
controller-specific data in the xs is correct.  As I proposed this
change, the extra calldowns would merely reserve space on the
controller.  I believe this is better because it doesn't put *any*
knowledge of how the controller works internally into the xs.

(Lastly, I suppose it's nice that some of my changes have finally made
it into FreeBSD, but it makes looking at these diffs ... rather more
difficult.  And just whose idea was it that we were going to replace
our SCSI layer with yours?  I don't recall ever discussing that, and
given the current state of the world, I'm certainly not in favour of
it.)