[EMAIL PROTECTED] wrote:
> @@ -2470,59 +2462,181 @@ fc_rport_create(struct Scsi_Host *shost, int channel,
> INIT_WORK(&rport->stgt_delete_work, fc_starget_delete);
> INIT_WORK(&rport->rport_delete_work, fc_rport_final_delete);
>
> + dev = &rport->dev;
> + dev->parent = get_device(&shost->shost_gendev); /* parent reference */
> + dev->release = fc_rport_dev_release;
> + sprintf(dev->bus_id, "rport-%d:%d-%d",
> + shost->host_no, channel, rport->number);
> + device_initialize(dev); /* takes self reference */
> +
> + spin_lock_irqsave(shost->host_lock, flags);
> + rport->number = fc_host->next_rport_number++;
Delete the rport->number assignment. It's done in remote_port_add().
> + list_add_tail(&rport->peers, &fc_host->rogue_rports);
> + get_device(&shost->shost_gendev); /* for fc_host->*rports list */
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + return rport;
> +}
> +EXPORT_SYMBOL_GPL(fc_remote_port_alloc);
This should be EXPORT_SYMBOL(fc_remote_port_alloc)
> +
> +/**
> + * fc_remote_port_free - drop reference from allocation
> + * @rport: remote port
> + *
> + * This should be called if the LLD has not called
> + * add on the rport and wishes to free the resources for the rogue port.
> + * For example if discovery failed, then the LLD should call this
> + * function to free the rport that was allocated.
> + */
> +void fc_remote_port_free(struct fc_rport *rport)
> +{
> + struct Scsi_Host *shost = rport_to_shost(rport);
> + unsigned long flags;
> +
> + spin_lock_irqsave(shost->host_lock, flags);
> + if (!list_empty(&rport->peers))
> + list_del(&rport->peers);
> + /* for fc_host->rogue_rports list */
> + put_device(&rport->dev)
Shouldn't this be "put_device(&shost->shost_gendev)" ?
> + spin_unlock_irqrestore(shost->host_lock, flags);
> +
> + /* for self-reference */
> + put_device(&rport->dev);
> +}
> +EXPORT_SYMBOL_GPL(fc_remote_port_free);
This should be EXPORT_SYMBOL(fc_remote_port_free)
> +
> +static struct fc_rport *
> +rport_lookup(struct fc_host_attrs *fc_host, struct list_head *list, int
> channel,
> + enum fc_tgtid_binding_type tgtid_bind_type,
> + struct fc_rport_identifiers *ids)
oooh... I really dislike the overloading of the binding type on the
search type. Minimally, change the name "tgtid_bind_type" to "search_type".
> +{
> + struct fc_rport *rport;
> +
> + list_for_each_entry(rport, list, peers) {
> + if (rport->channel != channel)
> + continue;
> +
> + switch (tgtid_bind_type) {
> + case FC_TGTID_BIND_BY_WWPN:
> + case FC_TGTID_BIND_NONE:
> + if (rport->port_name != ids->port_name)
> + continue;
> + break;
> + case FC_TGTID_BIND_BY_WWNN:
> + if (rport->node_name != ids->node_name)
> + continue;
> + break;
> + case FC_TGTID_BIND_BY_ID:
> + if (rport->port_id != ids->port_id)
> + continue;
> + break;
> + default:
> + continue;
> + }
> + get_device(&rport->dev);
> + return rport;
> + }
> + return NULL;
> +}
> +
> +/**
> + * fc_remote_port_lookup - lookup a remote port by id
> + * @shost: scsi host
> + * @channel: channel
> + * @tgtid_bind_type: bind type
> + * @id: port id
> + *
> + * If a the port is found, it will be returned with a refcount on it.
> + * The caller must do a put_device on the rport when it is done.
> + * This function will check all rports lists, so the caller must
> + * check the rport port_state before processing.
> + *
> + * Caller must hold host lock.
> + */
> +static struct fc_rport *
> +__fc_remote_port_lookup(struct Scsi_Host *shost, int channel,
> + enum fc_tgtid_binding_type tgtid_bind_type,
> + struct fc_rport_identifiers *ids)
> +{
> + struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
> + struct fc_rport *rport;
> +
> + rport = rport_lookup(fc_host, &fc_host->rports, channel,
> + tgtid_bind_type, ids);
> + if (rport)
> + return rport;
> +
> + rport = rport_lookup(fc_host, &fc_host->rport_bindings, channel,
> + tgtid_bind_type, ids);
> + if (rport)
> + return rport;
> +
> + return rport_lookup(fc_host, &fc_host->rogue_rports, channel,
> + tgtid_bind_type, ids);
Why would you ever search "rogue" things that have no guarantee that
their values are valid ? remove the last line and just return NULL.
> +}
> +
> +struct fc_rport *
> +fc_remote_port_lookup(struct Scsi_Host *shost, int channel,
> + enum fc_tgtid_binding_type tgtid_bind_type,
> + struct fc_rport_identifiers *ids)
> +{
> + struct fc_rport *rport;
> + unsigned long flags;
> +
> + spin_lock_irqsave(shost->host_lock, flags);
> + rport = __fc_remote_port_lookup(shost, channel, tgtid_bind_type, ids);
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + return rport;
> +}
> +EXPORT_SYMBOL_GPL(fc_remote_port_lookup);
The whole notion of bindings is internal to the transport. We shouldn't
be hinting at them for LLDD interfaces. What I prefer here is a change
to an enum fc_search_type, with the enum having wwpn, port_id, or scsi
target id. But - the search has to be limited to the fc_host->rports
list so that it only returns real/valid entities.
> /**
> * fc_remote_port_add - notify fc transport of the existence of a remote FC
> port.
> - * @shost: scsi host the remote port is connected to.
> - * @channel: Channel on shost port connected to.
> - * @ids: The world wide names, fc address, and FC4 port
> - * roles for the remote port.
> + * @shost: scsi host the remote port is connected to.
> + * @rogue_rport: optional rogue rport.
> + * @channel: Channel on shost port connected to.
> + * @ids: The world wide names, fc address, and FC4 port
> + * roles for the remote port.
> *
> * The LLDD calls this routine to notify the transport of the existence
> * of a remote port. The LLDD provides the unique identifiers (wwpn,wwn)
> @@ -2552,157 +2666,102 @@ delete_rport:
> *
> * Should not be called from interrupt context.
> *
> + * The caller should have resolved rogue with active or deleted
> + * rports before calling.
> + *
> * Notes:
> * This routine assumes no locks are held on entry.
> */
> struct fc_rport *
> fc_remote_port_add(struct Scsi_Host *shost, int channel,
> - struct fc_rport_identifiers *ids)
> + struct fc_rport *rogue_rport,
> + struct fc_rport_identifiers *ids)
... rest of fc_remote_port_add changes.
I like how much code you cleaned up by replacing the per-list search
paths, but it's thrown me. Proceed with what's there, and I'll post a
patch if I see anything else.
-- james s
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
