On Mon, Jul 24, 2006 at 09:46:06AM -0400, James Smart wrote:
> - I disagree with the handing off of a function pointer, with no real
> bounds on it's "lifetime". There are windows with the current code
> that if the driver were to then fail attachment and tear down
> the shost, it would lead to bogus function calls, as well as
> threads, etc that are essentially orphaned.
Can't happen; we take a reference on the shost and only release it once
it's done:
data->shost = scsi_host_get(shost);
> - The jiffies thing for lpfc isn't needed, we'll deal with it in
> the driver.
OK, I can take that out again.
> - I'm a little nervous with the scsi_scan_target() vectoring into
> scsi_complete_async_scans(). The fc transport does this on a work
> queue, which means you are stalling it, mixing the fchost work
> queue with other driver code, etc (flushes always are a headache).
> Today, it's ok, but...
> >>Note: found another bug in the fc transport sysfs scan
> >> function. We're supposed to be holding the shost_lock when
> >> scan target is called - which would be bad news for your patch
> >> as is. Good news is, we have a bug and aren't holding the lock.
> >> I'll have to fix this bug.
We only wait for other scans to finish if this host isn't in the middle
of an async scan. So this isn't a problem either.
> - I'd prefer the callback function to be part of the shost template,
> and kicked in as of scsi_add_host(), and terminated as of
> scsi_remove_host(). This would also make the thread integrated
> with the host. Also, this "scan_ready" check feels like it should
> be more of a generic thing that is always executed in the scan
> code if the function is present. I'll try to send out a counter
> patch.
I'm open to this. I think what I'd prefer to see is the FC drivers
setting a SHT->scan_finished() method, then calling scsi_scan_host()
which would then do something completely different for drivers which
have a scan_finished method than it would for SPI drivers.
> - For lpfc, I need to do a different patch. The delay, based on
> where it was, occurred in other paths, such as when applications
> took the card online/offline. So, I'll need to deal with them.
> I don't know if the other vendors have the same issue.
> Also, I'll have to test it before ACK'ing it.
Yes, I got a bug report by private mail saying that the QLogic driver
still waits for LIPs to complete for 20 seconds per card. So more
iteration on this patch is certainly needed.
> - Lastly, be aware that this code only helps parallelize the discovery
> delay loops in each host (the goal :). It does nothing to aid
> consistent device naming. There are still no guarantees about which
> rport gets which target id, which is scanned first/later, etc.
> Udev is still a requirement.
Yes, indeed. It occurs to me that the async scanning code does actually
give us a chance to sort devices before they get added to sysfs (and
named). I don't understand the exact problem that FC has, but this
could give us back the sort order that you had when you were doing your
own discovery, right?
Clearly I haven't done a good enough job explaining how this patch
works. I'm going to do a design document now ...
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
