Re: [PATCH v3 13/31] elx: libefc: Fabric node state machine interfaces

Hannes Reinecke
2020-04-16T06:38:53Z

On 4/12/20 5:32 AM, James Smart wrote:
> This patch continues the libefc library population.
>
> This patch adds library interface definitions for:
> - Fabric node initialization and logins.
> - Name/Directory Services node.
> - Fabric Controller node to process rscn events.
>
> These are all interactions with remote ports that correspond
> to well-known fabric entities
>
> Signed-off-by: Ram Vegesna <ram.vegesna@broadcom.com>
> Signed-off-by: James Smart <jsmart2021@gmail.com>
>
> ---
> v3:
> Replace efc_assert with WARN_ON
> Return defined return values
> ---
> drivers/scsi/elx/libefc/efc_fabric.c | 1759 ++++++++++++++++++++++++++++++++++
> drivers/scsi/elx/libefc/efc_fabric.h | 116 +++
> 2 files changed, 1875 insertions(+)
> create mode 100644 drivers/scsi/elx/libefc/efc_fabric.c
> create mode 100644 drivers/scsi/elx/libefc/efc_fabric.h
>
> diff --git a/drivers/scsi/elx/libefc/efc_fabric.c b/drivers/scsi/elx/libefc/efc_fabric.c
> new file mode 100644
> index 000000000000..251f8702dbc5
> --- /dev/null
> +++ b/drivers/scsi/elx/libefc/efc_fabric.c
> @@ -0,0 +1,1759 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Broadcom. All Rights Reserved. The term
> + * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
> + */
> +
> +/*
> + * This file implements remote node state machines for:
> + * - Fabric logins.
> + * - Fabric controller events.
> + * - Name/directory services interaction.
> + * - Point-to-point logins.
> + */
> +
> +/*
> + * fabric_sm Node State Machine: Fabric States
> + * ns_sm Node State Machine: Name/Directory Services States
> + * p2p_sm Node State Machine: Point-to-Point Node States
> + */
> +
> +#include "efc.h"
> +
> +static void
> +efc_fabric_initiate_shutdown(struct efc_node *node)
> +{
> + int rc;
> + struct efc *efc = node->efc;
> +
> + efc->tt.scsi_io_alloc_disable(efc, node);
> +
> + if (node->attached) {
> + /* issue hw node free; don't care if succeeds right away
> + * or sometime later, will check node->attached later in
> + * shutdown process
> + */
> + rc = efc->tt.hw_node_detach(efc, &node->rnode);
> + if (rc != EFC_HW_RTN_SUCCESS &&
> + rc != EFC_HW_RTN_SUCCESS_SYNC) {
> + node_printf(node, "Failed freeing HW node, rc=%d\n",
> + rc);
> + }
> + }
> + /*
> + * node has either been detached or is in the process of being detached,
> + * call common node's initiate cleanup function
> + */
> + efc_node_initiate_cleanup(node);
> +}
> +
> +static void *
> +__efc_fabric_common(const char *funcname, struct efc_sm_ctx *ctx,
> + enum efc_sm_event evt, void *arg)
> +{
> + struct efc_node *node = NULL;
> +
> + node = ctx->app;
> +
> + switch (evt) {
> + case EFC_EVT_DOMAIN_ATTACH_OK:
> + break;
> + case EFC_EVT_SHUTDOWN:
> + node->shutdown_reason = EFC_NODE_SHUTDOWN_DEFAULT;
> + efc_fabric_initiate_shutdown(node);
> + break;
> +
> + default:
> + /* call default event handler common to all nodes */
> + __efc_node_common(funcname, ctx, evt, arg);
> + break;
> + }
> + return NULL;
> +}
> +
> +void *
> +__efc_fabric_init(struct efc_sm_ctx *ctx, enum efc_sm_event evt,
> + void *arg)
> +{
> + struct efc_node *node = ctx->app;
> + struct efc *efc = node->efc;
> +
> + efc_node_evt_set(ctx, evt, __func__);
> +
> + node_sm_trace();
> +
> + switch (evt) {
> + case EFC_EVT_REENTER: /* not sure why we're getting these ... */
> + efc_log_debug(efc, ">>> reenter !!\n");
> + /* fall through */
> + case EFC_EVT_ENTER:
> + /* sm: / send FLOGI */
> + efc->tt.els_send(efc, node, ELS_FLOGI,
> + EFC_FC_FLOGI_TIMEOUT_SEC,
> + EFC_FC_ELS_DEFAULT_RETRIES);
> + efc_node_transition(node, __efc_fabric_flogi_wait_rsp, NULL);
> + break;
> +
> + default:
> + __efc_fabric_common(__func__, ctx, evt, arg);
> + break;
> + }
> +
> + return NULL;
> +}
> +
What is going on here?
Why do you declare all function as 'void *', but then continue to return
only NULL pointer?

Is this some weird API logic or are these functions being fleshed out in
later patches to return something else but NULL?

But as it stands I would recommend to move all of these functions to
simple 'void' functions, and updating the function prototypes once they
can return anything else than 'NULL'.

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer