Re: [PATCH v3 14/31] elx: libefc: FC node ELS and state handling

James Smart
2020-04-23T02:55:28Z

On 4/15/2020 11:47 PM, Hannes Reinecke wrote:
...
>> +    case EFC_EVT_FLOGI_RCVD: {
>> +        struct fc_frame_header *hdr = cbdata->header->dma.virt;
>> +        u32 d_id = ntoh24(hdr->fh_d_id);
>> +
>> +        /* sm: / save sparams, send FLOGI acc */
>> +        memcpy(node->sport->domain->flogi_service_params,
>> +               cbdata->payload->dma.virt,
>> +               sizeof(struct fc_els_flogi));
>> +
>
> Is the '->domain' pointer always present at this point?
> Shouldn't we rather test for it before accessing?

Always - if there's a node ans sport.

>
>> +        /* send FC LS_ACC response, override s_id */
>> +        efc_fabric_set_topology(node, EFC_SPORT_TOPOLOGY_P2P);
>> +        efc->tt.send_flogi_p2p_acc(efc, node,
>> +                be16_to_cpu(hdr->fh_ox_id), d_id);
>> +        if (efc_p2p_setup(node->sport)) {
>> +            node_printf(node,
>> +                    "p2p setup failed, shutting down node\n");
>> +            efc_node_post_event(node, EFC_EVT_SHUTDOWN, NULL);
>> +        } else {
>> +            efc_node_transition(node,
>> +                        __efc_p2p_wait_flogi_acc_cmpl,
>> +                        NULL);
>> +        }
>> +
>> +        break;
>> +    }
>> +
>> +    case EFC_EVT_LOGO_RCVD: {
>> +        struct fc_frame_header *hdr = cbdata->header->dma.virt;
>> +
>> +        if (!node->sport->domain->attached) {
>> +            /* most likely a frame left over from before a link
>> +             * down; drop and
>> +             * shut node down w/ "explicit logout" so pending
>> +             * frames are processed
>> +             */
>
> Same here; I find it slightly weird to have an 'attached' field in the
> domain structure; attached to what?
> Doesn't the existence of the ->domain pointer signal the same thing?
> If it doesn't, why don't we test for the ->domain pointer before
> accessing it?

Attached means something different here. Not only does the structure
exist, but there have been VFI/VPI resources registered with the
hardware for it.

Agree with your other comments and will address them.
And the same agreement for comments on patches 3, 9 & 10.

-- james