From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753689Ab1B1XW6 (ORCPT ); Mon, 28 Feb 2011 18:22:58 -0500 Received: from mail.linux-iscsi.org ([67.23.28.174]:54920 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753488Ab1B1XW5 (ORCPT ); Mon, 28 Feb 2011 18:22:57 -0500 Subject: Re: [PATCH 5/5] tcm_loop: Add multi-fabric Linux/SCSI LLD fabric module From: "Nicholas A. Bellinger" To: Christoph Hellwig Cc: linux-scsi , linux-kernel , James Bottomley , Mike Christie , Hannes Reinecke , FUJITA Tomonori , Joel Becker , Douglas Gilbert In-Reply-To: <20110228080748.GA15238@lst.de> References: <1298874020-15628-1-git-send-email-nab@linux-iscsi.org> <1298874020-15628-6-git-send-email-nab@linux-iscsi.org> <20110228080748.GA15238@lst.de> Content-Type: text/plain Date: Mon, 28 Feb 2011 15:16:10 -0800 Message-Id: <1298934970.5401.224.camel@haakon2.linux-iscsi.org> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > +#include > > +#include > > 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 > > +#include > > +#include > > You don't use kthreads in this file, so it shouldn't be needed either. > , dropping unnecessary kthread.h includes.. > > +#include > > +#include > > +#include > > +#include > > 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 > > 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 > > +#include > > +#include > > not needed. > , these includes are now only included for tcm_loop_configfs.c > > +#include /* 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 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 Signed-off-by: Nicholas A. Bellinger 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