On Sun, Jan 03, 2021 at 09:11:19AM -0800, James Smart wrote:
> This patch continues the libefc library population.
>
> This patch adds library interface definitions for:
> -Registrations for VFI, VPI and RPI.
>
> Co-developed-by: Ram Vegesna <ram.vegesna@xxxxxxxxxxxx>
> Signed-off-by: Ram Vegesna <ram.vegesna@xxxxxxxxxxxx>
> Signed-off-by: James Smart <jsmart2021@xxxxxxxxx>
>
> ---
> v5:
> EFC_EVT_XXX name changes.
> ---
> drivers/scsi/elx/libefc/efc_cmds.c | 877 +++++++++++++++++++++++++++++
> drivers/scsi/elx/libefc/efc_cmds.h | 35 ++
> 2 files changed, 912 insertions(+)
> create mode 100644 drivers/scsi/elx/libefc/efc_cmds.c
> create mode 100644 drivers/scsi/elx/libefc/efc_cmds.h
>
> diff --git a/drivers/scsi/elx/libefc/efc_cmds.c b/drivers/scsi/elx/libefc/efc_cmds.c
> new file mode 100644
> index 000000000000..a1cca840cf63
> --- /dev/null
> +++ b/drivers/scsi/elx/libefc/efc_cmds.c
> @@ -0,0 +1,877 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Broadcom. All Rights Reserved. The term
> + * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
> + */
> +
> +#include "efclib.h"
> +#include "../libefc_sli/sli4.h"
> +#include "efc_cmds.h"
> +#include "efc_sm.h"
> +
> +static void
> +efc_nport_free_resources(struct efc_nport *nport, int evt, void *data)
> +{
> + struct efc *efc = nport->efc;
> +
> + /* Clear the nport attached flag */
> + nport->attached = false;
> +
> + /* Free the service parameters buffer */
> + if (nport->dma.virt) {
> + dma_free_coherent(&efc->pci->dev, nport->dma.size,
> + nport->dma.virt, nport->dma.phys);
> + memset(&nport->dma, 0, sizeof(struct efc_dma));
> + }
> +
> + /* Free the SLI resources */
> + sli_resource_free(efc->sli, SLI4_RSRC_VPI, nport->indicator);
> +
> + efc_nport_cb(efc, evt, nport);
> +}
> +
> +static int
> +efc_nport_get_mbox_status(struct efc_nport *nport, u8 *mqe, int status)
> +{
> + struct efc *efc = nport->efc;
> + struct sli4_mbox_command_header *hdr =
> + (struct sli4_mbox_command_header *)mqe;
> + int rc = 0;
Is there a reason not to use the EFC_SUCCESS/FAIL defines? Anyway, this
local variable could be avoided by just using return directly.
> +
> + if (status || le16_to_cpu(hdr->status)) {
> + efc_log_debug(efc, "bad status vpi=%#x st=%x hdr=%x\n",
> + nport->indicator, status, le16_to_cpu(hdr->status));
> + rc = -1;
return EFC_FAIL/-1;
> + }
> +
> + return rc;
return EFC_SUCCESS/0;
Anyway, this pattern with 'rc = foo(); if (rc) {}' is different to the
patches I've seen so far. Usually, the local rc variable is avoided and
the function is directly called in the if condition
statement. Personally, I find the style here simpler to follow but for
consistency sake I rather have one style through out the driver.
> +static int
> +efc_nport_free_unreg_vpi_cb(struct efc *efc, int status, u8 *mqe, void *arg)
> +{
> + struct efc_nport *nport = arg;
> + int evt = EFC_EVT_NPORT_FREE_OK;
> + int rc = 0;
Again the question why not EFC_SUCCESS?
Rest looks good. As the above comments are just nitpicking:
Reviewed-by: Daniel Wagner <dwagner@xxxxxxx>
