On Mon, 20 Nov 2006, Matthew Wilcox wrote:
> On Mon, Nov 20, 2006 at 11:21:41AM -0800, Andrew Vasquez wrote:
> > Perhaps as an alternative, I'd propose the following change to qla2xxx
> > intialization semantics:
> >
> > - perform basic hardware configuration only (as usual)
> > - allocate resources
> > - load and execute firmware
> >
> > - defer link (transport) negotiations to the DPC thread
> > - again the code in qla2x00_initialize_adapter() to stall probe()
> > completion was needed for legacy-style scanning.
> > - DPC thread stalls until probe() complete.
> >
> > - before probe() completes, set DPC flags to perform loop-resync logic
> > (similar to what's done during cable-insertion/removal).
> >
> > Benefits: user does not have to wait 20+ seconds in case the FC cable
> > is unplugged during driver load, code consolidation (removal of
> > redundant link negotiation logic during initialize_adaoter()), and
> > finilly, the driver no longer needs to defer the fc_remote_port_add()
> > calls to hold off lun-scanning prior to returning from the probe()
> > function.
>
> This seems like a nice cleanup of some moderately complicated code.
> We still need scan_start and scan_finished methods so that the midlayer
> knows when the qla2xxx driver thinks it's found all the devices there
> are to find.
BTW: side-note, this is a bit tricky, as we are dealing with a fabric
environment where ports can fall on/off a topology at any given time
(ISL removed, port disconnected, etc. can all cause a loss in fcport
visibility). What this condition in scan_finished() is going to
catch:
+static int
+qla2xxx_scan_finished(struct Scsi_Host *shost, unsigned long time)
+{
+ scsi_qla_host_t *ha = (scsi_qla_host_t *)shost->hostdata;
+
+ if (!ha->host)
+ return 1;
+ if (time > ha->loop_reset_delay * HZ)
+ return 1;
+
+ return atomic_read(&ha->loop_state) == LOOP_READY;
+}
is the *first* instance where the firmware/driver has attained a
steady link state within the topology. The 'found all the devices
there are to find' case may or may not fall within this window...
> But the patch to add those looks like it could be
> significantly smaller and have less chance of being buggy.
>
> I think it should probably look something like this:
Ok, I've tweaked the code a bit and have been testing with the
following two patches, the first is a cleaned-up revision of my
'alternate' proposal (from above). The second, adds callbacks for
your scan_start/end() work.
...
> ... and then add the call to scsi_scan_host() right after the call
> to scsi_add_host(). Bear in mind that if async scanning is disabled,
> scsi_scan_host will wait for ->scan_finished() to return true, so
> everything (eg interrupts and the DPC thread) must be initialised
> before calling scsi_scan_host().
Yes, that will always be the case.
> But also remember that we must call
> scsi_scan_host() before the driver calls scsi_scan_target() for the
> first time, so anything that could trigger that happening has to be done
> in ->scan_start().
Yep. I'll continue testing with these two patches as an alternative
to your original work:
[SCSI] Convert qla2xxx to use scsi_scan_host
http://git.parisc-linux.org/?p=linux-2.6.git;a=commitdiff;h=114cf7c818ee1ba9104dbf0574c3b39e4f3ea5ef
-
To unsubscribe from this list: 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
