On Mon, 2011-02-28 at 09:07 +0100, Christoph Hellwig wrote:
> > --- /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.
>
Ugh, forgot to fix up the Makefile bits..
Removed EXTRA_CFLAGS.
> > +
> > +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.
>
Fixed
> > +#include <linux/version.h>
> > +#include <generated/utsrelease.h>
> > +#include <linux/utsname.h>
>
> These should not be needed.
>
Removed from tcm_loop_fabric_scsi.c, but these are still required for
tcm_loop_configfs.c:tcm_loop_wwn_show_attr_version()
> > +#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.
>
<nod>, dropping unnecessary kthread.h includes..
> > +#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.
>
Fixed
> > + tl_nexus = kzalloc(sizeof(struct tcm_loop_nexus), GFP_KERNEL);
> > + if (!(tl_nexus)) {
>
> Please remove all these totally pointless braces inside negations.
>
Fixed in all code
> > 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.
>
Fair point. Merging tcm_loop_configfs.h, tcm_loop_fabric.h and
tcm_loop_fabric_scsi.h into tcm_loop_core.h
> > +#include <linux/kthread.h>
>
> Again, this should not be needed.
>
Removed
> > +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);
>
Changed here and in tcm_loop_queue_status()
> > +#include <linux/version.h>
> > +#include <generated/utsrelease.h>
> > +#include <linux/utsname.h>
>
> not needed.
>
<nod>, these includes are now only included for tcm_loop_configfs.c
> > +#include <scsi/libsas.h> /* For TASK_ATTR_* */
>
> I thought that got fixed for the next merge window?
>
The TMR definitions did get fixed, but the conversion to use scsi_tcq.h
for SAM Task Attributes has not happened yet..
> > + * 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.
>
Indeed, this type of generic SCSI code should be moved into scsi_lib.c
so we can drop this LLD duplicate code.. While this does not address
all drivers that need to preform LLD specific ops while changing the
struct scsi_device->queue_depth, a generic caller would work fine for
these cases..
> > +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.
>
Ok, converted tcm_loop_queuecommand() and tcm_loop_device_reset() to use
shost_priv(). Thanks for pointing this one out..
> > + 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.
>
Unnecessary check(s) removed..
> > +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.
Removed the NULL assignments from tcm_loop_driver_template..
Thanks for your review. The tcm_loop changes have been committed and
pushed into lio-4.1 here, and I will get patch #5 of the mainline series
updated shortly..
commit 07bc6603d7ff5f0abf5fc0e3d546b17d0d32cafe
Author: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
Date: Mon Feb 28 15:40:13 2011 -0800
tcm_loop: Cleanups for mainline
This patch contains the following changes to TCM_Loop from Christoph's
comments:
*) Remove EXTRA_FLAGS from Makefile
*) Change TCM_LOOP_CDB_DEBUG -> CONFIG_TCM_LOOP_CDB_DEBUG1
*) Remove unncessary kthread.h and version.h/utsrelease.h/utsname.h includes
*) Convert .c code to use "" style includes for local includes
*) Change 'if (!(foo)) {}' usage to 'if (!foo) {}' and (*sc->scsi_done)() ->
sc->scsi_done(sc)
*) Merge tcm_loop_configfs.h, tcm_loop_fabric.h, and tcm_loop_fabric_scsi.h
into tcm_loop_core.h
*) Convert tcm_loop_driver_probe() to setup sh->hostdata for the tl_hba
pointer, and update tcm_loop_queuecommand() + tcm_loop_device_reset()
to use include/scsi/scsi_host.h:shost_priv()
Reported-by: Christoph Hellwig <hch@xxxxxx>
Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
drivers/target/tcm_loop/Makefile | 6 --
drivers/target/tcm_loop/tcm_loop_configfs.c | 38 ++++++--------
drivers/target/tcm_loop/tcm_loop_configfs.h | 2 -
drivers/target/tcm_loop/tcm_loop_core.h | 66 +++++++++++++++++++++++-
drivers/target/tcm_loop/tcm_loop_fabric.c | 21 +++----
drivers/target/tcm_loop/tcm_loop_fabric.h | 40 --------------
drivers/target/tcm_loop/tcm_loop_fabric_scsi.c | 63 ++++++-----------------
drivers/target/tcm_loop/tcm_loop_fabric_scsi.h | 10 ----
8 files changed, 107 insertions(+), 139 deletions(-)
delete mode 100644 drivers/target/tcm_loop/tcm_loop_configfs.h
delete mode 100644 drivers/target/tcm_loop/tcm_loop_fabric.h
delete mode 100644 drivers/target/tcm_loop/tcm_loop_fabric_scsi.h
Thanks!
--nab
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
