Re: [PATCH 24/32] elx: efct: LIO backend interface routines

Bart Van Assche
2019-10-24T22:27:52Z

On 10/23/19 2:55 PM, James Smart wrote:
> diff --git a/drivers/scsi/elx/efct/efct_lio.c b/drivers/scsi/elx/efct/efct_lio.c
> new file mode 100644
> index 000000000000..c2661ab3e9c3
> --- /dev/null
> +++ b/drivers/scsi/elx/efct/efct_lio.c
> @@ -0,0 +1,2643 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Broadcom. All Rights Reserved. The term
> + * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
> + */
> +
> +#include "efct_driver.h"
> +
> +#include <scsi/scsi.h>
> +#include <scsi/scsi_host.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_tcq.h>
> +#include <target/target_core_base.h>
> +#include <target/target_core_fabric.h>

Please do not include SCSI initiator header files in a SCSI target
driver. See also commit ba929992522b ("target: Minimize SCSI header
#include directives").

> +#define FABRIC_NAME "efct"
> +#define FABRIC_NAME_NPIV "efct_npiv"

Some time ago Christoph Hellwig asked not to use the prefix "fabric" but
to use the prefix "target" instead.

> +#define FABRIC_SNPRINTF_LEN 32

"FABRIC_SNPRINTF_LEN" is a bad choice for the name for this constant.
Please change this into a name that refers to what the purpose of this
constant is (wwn string?) instead of how that string is generated.

> +#define FABRIC_SNPRINTF(str, len, pre, wwn) snprintf(str, len, \
> + "%s%02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x", pre, \
> + (u8)((wwn >> 56) & 0xff), (u8)((wwn >> 48) & 0xff), \
> + (u8)((wwn >> 40) & 0xff), (u8)((wwn >> 32) & 0xff), \
> + (u8)((wwn >> 24) & 0xff), (u8)((wwn >> 16) & 0xff), \
> + (u8)((wwn >> 8) & 0xff), (u8)((wwn & 0xff)))

Please convert this macro into a function and choose a better name, e.g.
efct_format_wwn().

> +#define ARRAY2WWN(w, a) (w = ((((u64)(a)[0]) << 56) | (((u64)(a)[1]) << 48) | \
> + (((u64)(a)[2]) << 40) | (((u64)(a)[3]) << 32) | \
> + (((u64)(a)[4]) << 24) | (((u64)(a)[5]) << 16) | \
> + (((u64)(a)[6]) << 8) | (((u64)(a)[7]))))

Is this perhaps an open-coded version of get_unaligned_be64()?

> +/* Per-target data for virtual targets */
> +struct efct_lio_vport_data_t {
> + struct list_head list_entry;
> + bool initiator_mode;
> + bool target_mode;
> + u64 phy_wwpn;
> + u64 phy_wwnn;
> + u64 vport_wwpn;
> + u64 vport_wwnn;
> + struct efct_lio_vport *lio_vport;
> +};
> +
> +/* Per-target data for virtual targets */
> +struct efct_lio_vport_list_t {
> + struct list_head list_entry;
> + struct efct_lio_vport *lio_vport;
> +};

Two times the same comment for two different data structures?
Additionally, what is a "virtual target"?

> +/* local prototypes */
> +static char *efct_lio_get_npiv_fabric_wwn(struct se_portal_group *);
> +static char *efct_lio_get_fabric_wwn(struct se_portal_group *);
> +static u16 efct_lio_get_tag(struct se_portal_group *);
> +static u16 efct_lio_get_npiv_tag(struct se_portal_group *);
> +static int efct_lio_check_demo_mode(struct se_portal_group *);
> +static int efct_lio_check_demo_mode_cache(struct se_portal_group *);
> +static int efct_lio_check_demo_write_protect(struct se_portal_group *);
> +static int efct_lio_check_prod_write_protect(struct se_portal_group *);
> +static int efct_lio_npiv_check_demo_write_protect(struct se_portal_group *);
> +static int efct_lio_npiv_check_prod_write_protect(struct se_portal_group *);
> +static u32 efct_lio_tpg_get_inst_index(struct se_portal_group *);
> +static int efct_lio_check_stop_free(struct se_cmd *se_cmd);
> +static void efct_lio_aborted_task(struct se_cmd *se_cmd);
> +static void efct_lio_release_cmd(struct se_cmd *);
> +static void efct_lio_close_session(struct se_session *);
> +static u32 efct_lio_sess_get_index(struct se_session *);
> +static int efct_lio_write_pending(struct se_cmd *);
> +static void efct_lio_set_default_node_attrs(struct se_node_acl *);
> +static int efct_lio_get_cmd_state(struct se_cmd *);
> +static int efct_lio_queue_data_in(struct se_cmd *);
> +static int efct_lio_queue_status(struct se_cmd *);
> +static void efct_lio_queue_tm_rsp(struct se_cmd *);
> +static struct se_wwn *efct_lio_make_sport(struct target_fabric_configfs *,
> + struct config_group *, const char *);
> +static void efct_lio_drop_sport(struct se_wwn *);
> +static void efct_lio_npiv_drop_sport(struct se_wwn *);
> +static int efct_lio_parse_wwn(const char *, u64 *, u8 npiv);
> +static int efct_lio_parse_npiv_wwn(const char *name, size_t size,
> + u64 *wwpn, u64 *wwnn);
> +static struct se_portal_group *efct_lio_make_tpg(struct se_wwn *,
> + const char *);
> +static struct se_portal_group *efct_lio_npiv_make_tpg(struct se_wwn *,
> + const char *);
> +static void efct_lio_drop_tpg(struct se_portal_group *);
> +static struct se_wwn *efct_lio_npiv_make_sport(struct target_fabric_configfs *,
> + struct config_group *,
> + const char *);
> +static int
> +efct_lio_parse_npiv_wwn(const char *name, size_t size, u64 *wwpn, u64 *wwnn);
> +static void efct_lio_npiv_drop_tpg(struct se_portal_group *);
> +static int efct_lio_async_worker(struct efct_s *efct);
> +static void efct_lio_sg_unmap(struct efct_io_s *io);
> +static int efct_lio_abort_tgt_cb(struct efct_io_s *io,
> + enum efct_scsi_io_status_e scsi_status,
> + u32 flags, void *arg);
> +
> +static int efct_lio_init_nodeacl(struct se_node_acl *, const char *);
> +
> +static int efct_lio_check_demo_mode_login_only(struct se_portal_group *);
> +static int efct_lio_npiv_check_demo_mode_login_only(struct se_portal_group *);

Please reorder the code in this file such that most or all of these
function declarations disappear.

> +static ssize_t
> +efct_lio_wwn_version_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "Emulex EFCT fabric module version %s\n",
> + __stringify(EFCT_LIO_VERSION));
> +}

Version numbers are not useful in upstream code. Please remove this
attribute and also the EFCT_LIO_VERSION constant.

> +static struct efct_lio_tpg *
> +efct_get_vport_tpg(struct efc_node_s *node)
> +{
> + struct efct_s *efct;
> + u64 wwpn = node->sport->wwpn;
> + struct efct_lio_vport_list_t *vport, *next;
> + struct efct_lio_vport *lio_vport = NULL;
> + struct efct_lio_tpg *tpg = NULL;
> + unsigned long flags = 0;
> +
> + efct = node->efc->base;
> + spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
> + list_for_each_entry_safe(vport, next,
> + &efct->tgt_efct.vport_list, list_entry) {
> + lio_vport = vport->lio_vport;
> + if (wwpn && lio_vport &&
> + lio_vport->npiv_wwpn == wwpn) {
> + efc_log_test(efct, "found tpg on vport\n");
> + tpg = lio_vport->tpg;
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
> + return tpg;
> +}

The indentation in this function is wrong. list_for_each_entry() should
occur at the same level as spin_lock_irqsave().

> +/* local static data */

> +/* local static data */

Are these comments useful?

> +#define LIO_IOFMT "[%04x][i:%0*x t:%0*x h:%04x][c:%02x]"
> +#define LIO_TMFIOFMT "[%04x][i:%0*x t:%0*x h:%04x][f:%02x]"
> +#define LIO_IOFMT_ITT_SIZE(efct) 4
> +
> +#define efct_lio_io_printf(io, fmt, ...) \
> + efc_log_debug(io->efct, "[%s]" LIO_IOFMT " " fmt, \
> + io->node->display_name, io->instance_index, \
> + LIO_IOFMT_ITT_SIZE(io->efct), io->init_task_tag, \
> + LIO_IOFMT_ITT_SIZE(io->efct), io->tgt_task_tag, io->hw_tag,\
> + (io->tgt_io.cdb ? io->tgt_io.cdb[0] : 0xFF), ##__VA_ARGS__)
> +#define efct_lio_tmfio_printf(io, fmt, ...) \
> + efc_log_debug(io->efct, "[%s]" LIO_TMFIOFMT " " fmt,\
> + io->node->display_name, io->instance_index, \
> + LIO_IOFMT_ITT_SIZE(io->efct), io->init_task_tag, \
> + LIO_IOFMT_ITT_SIZE(io->efct), io->tgt_task_tag, io->hw_tag,\
> + io->tgt_io.tmf, ##__VA_ARGS__)

Please remove the LIO_IOFMT, LIO_TMFIOFMT and LIO_IOFMT_ITT_SIZE macros
and expand these macros where these are used. I think that will make the
above logging functions much more easy to read.

> +#define efct_lio_io_state_trace(io, value) (io->tgt_io.state |= value)

This macro has "trace" in its name but does not trace anything. Please
either remove this macro or choose a better name.

> +static int efct_lio_tgt_session_data(struct efct_s *efct, u64 wwpn,
> + char *buf, int size)
> +{
> + struct efc_sli_port_s *sport = NULL;
> + struct efc_node_s *node = NULL;
> + struct efc_lport *efc = efct->efcport;
> + u16 loop_id = 0;
> + int off = 0, rc = 0;
> +
> + if (!efc->domain) {
> + efc_log_err(efct, "failed to find efct/domain\n");
> + return -1;
> + }
> +
> + list_for_each_entry(sport, &efc->domain->sport_list, list_entry) {
> + if (sport->wwpn == wwpn) {
> + list_for_each_entry(node, &sport->node_list,
> + list_entry) {
> + /* Dump sessions only remote NPORT
> + * sessions
> + */
> + if (efct_lio_node_is_initiator(node)) {
> + rc = snprintf(buf + off,
> + size - off,
> + "0x%016llx,0x%08x,0x%04x\n",
> + be64_to_cpup((__force __be64 *)
> + node->wwpn),
> + node->rnode.fc_id,
> + loop_id);
> + if (rc < 0)
> + break;
> + off += rc;
> + }
> + }
> + }
> + }
> +
> + buf[size - 1] = '\0';
> + return 0;
> +}

Please use get_unaligned_be64() instead of using __force casts.

> +static const struct file_operations efct_debugfs_session_fops = {
> + .owner = THIS_MODULE,
> + .open = efct_debugfs_session_open,
> + .release = efct_debugfs_session_close,
> + .read = efct_debugfs_session_read,
> + .write = efct_debugfs_session_write,
> + .llseek = default_llseek,
> +};
> +
> +static const struct file_operations efct_npiv_debugfs_session_fops = {
> + .owner = THIS_MODULE,
> + .open = efct_npiv_debugfs_session_open,
> + .release = efct_debugfs_session_close,
> + .read = efct_debugfs_session_read,
> + .write = efct_debugfs_session_write,
> + .llseek = default_llseek,
> +};

Since the information that is exported through debugfs (logged in
initiators) is information that is also useful for other target drivers,
I think this functionality should be implemented in the target core
instead of in this target driver.

> +/* command has been aborted, cleanup here */
> +static void efct_lio_aborted_task(struct se_cmd *se_cmd)
> +{
> + int rc;
> + struct efct_scsi_tgt_io_s *ocp = container_of(se_cmd,
> + struct efct_scsi_tgt_io_s,
> + cmd);
> + struct efct_io_s *io = container_of(ocp, struct efct_io_s, tgt_io);
> +
> + efct_lio_io_trace(io, "%s\n", __func__);
> + efct_lio_io_state_trace(io, EFCT_LIO_STATE_TFO_ABORTED_TASK);
> +
> + if (!(se_cmd->transport_state & CMD_T_ABORTED) || ocp->rsp_sent)
> + return;
> +
> + /*
> + * if io is non-null, take a reference out on it so it isn't
> + * freed until the abort operation is complete.
> + */
> + if (kref_get_unless_zero(&io->ref) == 0) {
> + /* command no longer active */
> + struct efct_s *efct = io->efct;
> +
> + efc_log_test(efct,
> + "success: command no longer active (exists=%d)\n",
> + (io != NULL));
> + return;
> + }
> +
> + efct_lio_io_printf(io, "CMD_T_ABORTED set, aborting=%d\n",
> + ocp->aborting);
> + ocp->aborting = true;
> + /* set to non-success so data moves won't continue */
> + ocp->err = EFCT_SCSI_STATUS_ABORTED;
> +
> + /* wait until abort is complete; once we return, LIO will call
> + * queue_tm_rsp() which will send response to TMF
> + */
> + init_completion(&io->tgt_io.done);
> +
> + rc = efct_scsi_tgt_abort_io(io, efct_lio_abort_tgt_cb, NULL);
> + if (rc == 0) {
> + /* wait for abort to complete before returning */
> + rc = wait_for_completion_timeout(&io->tgt_io.done,
> + usecs_to_jiffies(10000000));
> +
> + /* done with reference on aborted IO */
> + kref_put(&io->ref, io->release);
> +
> + if (rc) {
> + efct_lio_io_printf(io,
> + "abort completed successfully\n");
> + /* check if TASK_ABORTED status should be sent
> + * for this IO
> + */
> + } else {
> + efct_lio_io_printf(io,
> + "timeout waiting for abort completed\n");
> + }
> + } else {
> + efct_lio_io_printf(io, "Failed to abort\n");
> + }
> +}

The .aborted_task() callback function must not wait until the aborted
command has finished but instead must free the resources owned by the
aborted command.

The comment "check if TASK_ABORTED status should be sent for this IO" is
wrong. .aborted_task() is only called if no response will be sent to the
initiator.

> +/**
> + * @brief Housekeeping for LIO SG mapping.
> + *
> + * @param io Pointer to IO context.
> + *
> + * @return count Count returned by pci_map_sg.
> + */

The above comment follows the Doxygen syntax. Kernel function headers
must use the kernel-doc syntax. See also
Documentation/process/kernel-docs.rst.

> +static struct se_wwn *
> +efct_lio_make_sport(struct target_fabric_configfs *tf,
> + struct config_group *group, const char *name)
> +{
> + struct efct_lio_sport *lio_sport;
> + struct efct_s *efct;
> + int efctidx, ret;
> + u64 wwpn;
> + char *sessions_name;
> +
> + ret = efct_lio_parse_wwn(name, &wwpn, 0);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + /* Now search for the HBA that has this WWPN */
> + for (efctidx = 0; efctidx < MAX_EFCT_DEVICES; efctidx++) {
> + u64 pwwn;
> + u8 pn[8];
> +
> + efct = efct_devices[efctidx];
> + if (!efct)
> + continue;
> + memcpy(pn, efct_hw_get_ptr(&efct->hw, EFCT_HW_WWN_PORT),
> + sizeof(pn));
> + ARRAY2WWN(pwwn, pn);
> + if (pwwn == wwpn)
> + break;
> + }
> + if (efctidx == MAX_EFCT_DEVICES) {
> + pr_err("cannot find EFCT for wwpn %s\n", name);
> + return ERR_PTR(-ENXIO);
> + }
> + efct = efct_devices[efctidx];
> + lio_sport = kzalloc(sizeof(*lio_sport), GFP_KERNEL);
> + if (!lio_sport)
> + return ERR_PTR(-ENOMEM);
> + lio_sport->efct = efct;
> + lio_sport->wwpn = wwpn;
> + FABRIC_SNPRINTF(lio_sport->wwpn_str, sizeof(lio_sport->wwpn_str),
> + "naa.", wwpn);
> + efct->tgt_efct.lio_sport = lio_sport;
> +
> + sessions_name = kasprintf(GFP_KERNEL, "efct-sessions-%d",
> + efct->instance_index);
> + if (sessions_name && efct->sess_debugfs_dir)
> + lio_sport->sessions = debugfs_create_file(sessions_name,
> + 0644,
> + efct->sess_debugfs_dir,
> + lio_sport,
> + &efct_debugfs_session_fops);
> + kfree(sessions_name);
> +
> + return &lio_sport->sport_wwn;
> +}
> +
> +static struct se_wwn *
> +efct_lio_npiv_make_sport(struct target_fabric_configfs *tf,
> + struct config_group *group, const char *name)
> +{
> + struct efct_lio_vport *lio_vport;
> + struct efct_s *efct;
> + int efctidx, ret = -1;
> + u64 p_wwpn, npiv_wwpn, npiv_wwnn;
> + char *p, tmp[128];
> + struct efct_lio_vport_list_t *vport_list;
> + char *sessions_name;
> + struct fc_vport *new_fc_vport;
> + struct fc_vport_identifiers vport_id;
> + unsigned long flags = 0;
> +
> + snprintf(tmp, 128, "%s", name);
> +
> + p = strchr(tmp, '@');
> +
> + if (!p) {
> + pr_err("Unable to find separator operator(@)\n");
> + return ERR_PTR(ret);
> + }
> + *p++ = '\0';
> +
> + ret = efct_lio_parse_wwn(tmp, &p_wwpn, 0);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + ret = efct_lio_parse_npiv_wwn(p, strlen(p) + 1, &npiv_wwpn, &npiv_wwnn);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + /* Now search for the HBA that has this WWPN */
> + for (efctidx = 0; efctidx < MAX_EFCT_DEVICES; efctidx++) {
> + u64 pwwn;
> + u8 pn[8];
> +
> + efct = efct_devices[efctidx];
> + if (!efct)
> + continue;
> + if (!efct->xport->req_wwpn) {
> + memcpy(pn, efct_hw_get_ptr(&efct->hw,
> + EFCT_HW_WWN_PORT), sizeof(pn));
> + ARRAY2WWN(pwwn, pn);
> + } else {
> + pwwn = efct->xport->req_wwpn;
> + }
> + if (pwwn == p_wwpn)
> + break;
> + }
> + if (efctidx == MAX_EFCT_DEVICES) {
> + pr_err("cannot find EFCT for base wwpn %s\n", name);
> + return ERR_PTR(-ENXIO);
> + }
> + efct = efct_devices[efctidx];
> + lio_vport = kzalloc(sizeof(*lio_vport), GFP_KERNEL);
> + if (!lio_vport)
> + return ERR_PTR(-ENOMEM);
> +
> + lio_vport->efct = efct;
> + lio_vport->wwpn = p_wwpn;
> + lio_vport->npiv_wwpn = npiv_wwpn;
> + lio_vport->npiv_wwnn = npiv_wwnn;
> +
> + FABRIC_SNPRINTF(lio_vport->wwpn_str, sizeof(lio_vport->wwpn_str),
> + "naa.", npiv_wwpn);
> +
> + vport_list = kmalloc(sizeof(*vport_list), GFP_KERNEL);
> + if (!vport_list) {
> + kfree(lio_vport);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + memset(vport_list, 0, sizeof(struct efct_lio_vport_list_t));
> + vport_list->lio_vport = lio_vport;
> + spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
> + INIT_LIST_HEAD(&vport_list->list_entry);
> + list_add_tail(&vport_list->list_entry, &efct->tgt_efct.vport_list);
> + spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
> +
> + sessions_name = kasprintf(GFP_KERNEL, "sessions-npiv-%d",
> + efct->instance_index);
> + if (sessions_name && efct->sess_debugfs_dir)
> + lio_vport->sessions = debugfs_create_file(sessions_name,
> + 0644,
> + efct->sess_debugfs_dir,
> + lio_vport,
> + &efct_npiv_debugfs_session_fops);
> + kfree(sessions_name);
> + memset(&vport_id, 0, sizeof(vport_id));
> + vport_id.port_name = npiv_wwpn;
> + vport_id.node_name = npiv_wwnn;
> + vport_id.roles = FC_PORT_ROLE_FCP_INITIATOR;
> + vport_id.vport_type = FC_PORTTYPE_NPIV;
> + vport_id.disable = false;
> +
> + new_fc_vport = fc_vport_create(efct->shost, 0, &vport_id);
> + if (!new_fc_vport) {
> + efc_log_err(efct, "fc_vport_create failed\n");
> + kfree(lio_vport);
> + kfree(vport_list);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + lio_vport->fc_vport = new_fc_vport;
> +
> + return &lio_vport->vport_wwn;
> +}

Please rework efct_lio_make_sport() and efct_lio_npiv_make_sport() such
that the amount of duplicate code is reduced significantly.

> +
> + /* Create kernel worker thread to service async requests
> + * (new/delete initiator, new cmd/tmf). Previously, a worker thread
> + * was needed to make upcalls into LIO because the HW completion
> + * context ran in an interrupt context (tasklet).
> + * This is no longer necessary now that HW completions run in a
> + * kernel thread context. However, performance is much better when
> + * these types of reqs have their own thread.
> + *
> + * Note: We've seen better performance when IO completion (non-async)
> + * upcalls into LIO are not given an additional kernel thread.
> + * Thus,make such upcalls directly from the HW completion kernel thread
> + */
> +
> + worker = &efct->tgt_efct.async_worker;
> + efct_mqueue_init(efct, &worker->wq);
> +
> + worker->thread = kthread_create((int(*)(void *)) efct_lio_async_worker,
> + efct, "efct_lio_async_worker");
> +
> + if (IS_ERR(worker->thread)) {
> + efc_log_err(efct, "kthread_create failed: %ld\n",
> + PTR_ERR(worker->thread));
> + worker->thread = NULL;
> + return -1;
> + }
> +
> + wake_up_process(worker->thread);

Please use the kernel workqueue infrastructure instead of duplicating it.

> +/**
> + * @brief Worker thread for LIO commands.
> + *
> + * @par Description
> + * This thread is used to make LIO upcalls associated with
> + * asynchronous requests (i.e. new commands received, register
> + * sessions, unregister sessions).
> + *
> + * @param mythread Pointer to the thread object.
> + *
> + * @return Always returns 0.
> + */
> +static int efct_lio_async_worker(struct efct_s *efct)
> +{
> + struct efct_lio_wq_data_s *wq_data;
> + struct efc_node_s *node;
> + struct se_session *se_sess;
> + int done = 0;
> + bool free_data = true;
> + struct efct_scsi_tgt_io_s *ocp;
> + int dir, rc = 0;
> + struct efct_io_s *io;
> + struct efct_io_s *tmfio;
> + struct efct_scsi_tgt_node_s *tgt_node = NULL;
> +
> + while (!done) {
> + /* Poll with a timeout, to keep the kernel from complaining
> + * of not periodically running
> + */
> + wq_data = efct_mqueue_get(&efct->tgt_efct.async_worker.wq,
> + 10000000);
> + if (kthread_should_stop())
> + break;
> +
> + if (!wq_data)
> + continue;
> +
> [ ... ]
> + }
> + if (free_data)
> + kfree(wq_data);
> + }
> +
> + complete(&efct->tgt_efct.async_worker.done);
> +
> + return 0;
> +}

Same comment here: please use the kernel workqueue infrastructure
instead of duplicating it.

> +#define scsi_pack_result(key, code, qualifier) (((key & 0xff) << 16) | \
> + ((code && 0xff) << 8) | (qualifier & 0xff))

Where is this macro used? I haven't found any uses of this macro in this
patch.

> +#define FABRIC_SNPRINTF_LEN 32

Please choose a better name for this constant. Or even better, leave out
this define entirely and use sizeof().

> +static inline int
> +efct_mqueue_init(struct efct_s *efct, struct efct_mqueue_s *q)
> +{
> + memset(q, 0, sizeof(*q));
> + q->efct = efct;
> + spin_lock_init(&q->lock);
> + init_completion(&q->prod);
> + INIT_LIST_HEAD(&q->queue);
> + return 0;
> +}

Functions that are not in the hot path should be defined in a .c file
instead of in a header file.

Thanks,

Bart.