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

They've been discussed on FreeBSD-scsi to some extent, but there isn't
a common mailing list that I know of yet.

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

A single controller could be in charge of several different queues.  This
is geared toward multi-channel/RAID devices.  It is true that its not
used for anything yet.

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

Because some controllers share a queue across mutliple busses.  The 2742T
is one of these.

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

Having one function is fine by me.  But I don't think that you can remove
the need for scsi_xs_started.

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

scsi_xs_started should always be called as soon as the controller can
guarantee that the queuing order of the xs cannot be perterbed by an
interrupt or other event.

This is a case of not being able to have your cake and eat it too.  I
wanted to centralize the handling of errors to the scsi_done routine
but I couldn't prevent deadlock unless the lock was released by the
controller at the proper time since there are cases when scsi_done is
called before the *_scsi_cmd call completes.  I suppose I could record
the xs that holds the lock on the target, but this seemed a little excessive.
The current approach is actually more efficient anyway since the target is
only locked as long as it absolutely has to be.

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

In looking at this again, I'm not even sure why I did it this way.
I think the ordering is still maintained if the *start routines are
called without spl protection and do their own spl manipulation.

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

This was one of my initial complaints with scsi_scsi_cmd which was
why I separated it out into an *inline* function.  I kept it as
a function because of the type safety and assurance that all necesary
parameters are set.  There isn't any additional overhead.

>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)) {
>+ [...]

Did I say I was done? Its only included in the diff because the code
was moved.  I also believe there was an 'XXX' comment there. 8-)

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

This is because you don't know of my hidden agenda.  I'd like to see polling
go away entirely or at least only happen for dumps.  For all of the busses
I've looked at in depth, there isn't a reason that a device that has completed
its attach routine cannot have its interrupt enabled.  I'm sure that the arch
maintainers of NetBSD will quickly prove me wrong.... But none-the-less, I
see it as criminally broken to force all clients to decide for themselves if
they should poll or not.  For example, during a dump, all transactions had
better poll regardless of where they came from (even if they were queued
before the dump was started) or the dump may fail.  Moving this functionality
into _scsi_flags makes it so this works automagically during autoconfiguration
and dumps without forcing every client of the SCSI system to be aware of this
issue.

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

I won't even touch on who proposed what...  Anyway, the XS has no
knowlege of what the controller put in the cdb field.  Why complicate
the controller driver's job by forcing it to keep track of what is
reserved and what is not when this strategy is so simple and offers
a tight association of the resources.

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

I think that most of the changes from NetBSD up to that patch set were
type fixes/formating only.  I was looking at the quirk code last night though.

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

I said, "Once I am done, I'll port the code to Open/NetBSD."  Whether to
adopt it or not is for the Open/NetBSD camps to decide.  It certainly would
be nice if all of those device drivers could be shared.

--
Justin T. Gibbs
===========================================
  FreeBSD: Turning PCs into workstations
===========================================