lkml.org 
[lkml]   [2011]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 5/5] tcm_loop: Add multi-fabric Linux/SCSI LLD fabric module
> --- /dev/null
> +++ b/drivers/target/tcm_loop/Makefile
> @@ -0,0 +1,11 @@
> +EXTRA_CFLAGS += -I$(srctree)/drivers/target/ -I$(srctree)/drivers/scsi/ -I$(srctree)/include/scsi/ -I$(srctree)/drivers/target/tcm_loop/

This should not be needed.

> +
> +tcm_loop-y := tcm_loop_fabric.o \
> + tcm_loop_fabric_scsi.o \
> + tcm_loop_configfs.o \
> +
> +obj-$(CONFIG_TCM_LOOP_FABRIC) += tcm_loop.o
> +
> +ifdef CONFIG_TCM_LOOP_CDB_DEBUG
> +EXTRA_CFLAGS += -DTCM_LOOP_CDB_DEBUG
> +endif

Please just test for CONFIG_TCM_LOOP_CDB_DEBUG directly in the code.

> +#include <linux/version.h>
> +#include <generated/utsrelease.h>
> +#include <linux/utsname.h>

These should not be needed.

> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/kthread.h>

You don't use kthreads in this file, so it shouldn't be needed either.

> +#include <tcm_loop_core.h>
> +#include <tcm_loop_configfs.h>
> +#include <tcm_loop_fabric.h>
> +#include <tcm_loop_fabric_scsi.h>

Theseshould be ""-style includes.

> + tl_nexus = kzalloc(sizeof(struct tcm_loop_nexus), GFP_KERNEL);
> + if (!(tl_nexus)) {

Please remove all these totally pointless braces inside negations.

> diff --git a/drivers/target/tcm_loop/tcm_loop_configfs.h b/drivers/target/tcm_loop/tcm_loop_configfs.h
> new file mode 100644
> index 0000000..f46aa4b
> --- /dev/null
> +++ b/drivers/target/tcm_loop/tcm_loop_configfs.h
> @@ -0,0 +1,2 @@
> +extern int tcm_loop_register_configfs(void);
> +extern void tcm_loop_deregister_configfs(void);

I think just having one header for this module instead of mini-headers
like this is a lot easier to read/maintain.

> +#include <linux/kthread.h>

Again, this should not be needed.

> +int tcm_loop_queue_data_in(struct se_cmd *se_cmd)
> +{
> + struct tcm_loop_cmd *tl_cmd = container_of(se_cmd,
> + struct tcm_loop_cmd, tl_se_cmd);
> + struct scsi_cmnd *sc = tl_cmd->sc;
> +
> + TL_CDB_DEBUG( "tcm_loop_queue_data_in() called for scsi_cmnd: %p"
> + " cdb: 0x%02x\n", sc, sc->cmnd[0]);
> +
> + sc->result = SAM_STAT_GOOD;
> + set_host_byte(sc, DID_OK);
> + (*sc->scsi_done)(sc);

This can be written simpler as sc->scsi_done(sc);

> +#include <linux/version.h>
> +#include <generated/utsrelease.h>
> +#include <linux/utsname.h>

not needed.

> +#include <scsi/libsas.h> /* For TASK_ATTR_* */

I thought that got fixed for the next merge window?

> + * Copied from drivers/scsi/libfc/fc_fcp.c:fc_change_queue_depth() and
> + * drivers/scsi/libiscsi.c:iscsi_change_queue_depth()
> + */

Sounds like we should move it to generic code instead of adding a third
duplicate.

> +static inline struct tcm_loop_hba *tcm_loop_get_hba(struct scsi_cmnd *sc)
> +{
> + return (struct tcm_loop_hba *)sc->device->host->hostdata[0];
> +}

You probably want to use shost_priv instead.

> + struct se_portal_group *se_tpg;
> + struct tcm_loop_hba *tl_hba;
> + struct tcm_loop_tpg *tl_tpg;
> +
> + TL_CDB_DEBUG("tcm_loop_queuecommand() %d:%d:%d:%d got CDB: 0x%02x"
> + " scsi_buf_len: %u\n", sc->device->host->host_no,
> + sc->device->id, sc->device->channel, sc->device->lun,
> + sc->cmnd[0], scsi_bufflen(sc));
> + /*
> + * Locate the tcm_loop_hba_t pointer
> + */
> + tl_hba = tcm_loop_get_hba(sc);
> + if (!(tl_hba)) {
> + printk(KERN_ERR "Unable to locate struct tcm_loop_hba from"
> + " struct scsi_cmnd\n");
> + set_host_byte(sc, DID_ERROR);
> + sc->scsi_done(sc);
> + return 0;
> + }

This can't ever be null.

> +static struct scsi_host_template tcm_loop_driver_template = {
> + .proc_info = tcm_loop_proc_info,
> + .proc_name = "tcm_loopback",
> + .name = "TCM_Loopback",
> + .info = NULL,
> + .slave_alloc = NULL,
> + .slave_configure = NULL,
> + .slave_destroy = NULL,
> + .ioctl = NULL,

There is no need to fill unused methods with NULLs.


\
 
 \ /
  Last update: 2011-02-28 09:11    [W:0.072 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site