[Open-FCoE] [PATCH 1/1] fc class: add rogue rport support

James Smart James.Smart at Emulex.Com
Fri Sep 12 18:24:50 UTC 2008


michaelc at cs.wisc.edu 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



More information about the devel mailing list