From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755450AbcEXOOd (ORCPT ); Tue, 24 May 2016 10:14:33 -0400 Received: from smtprelay0008.hostedemail.com ([216.40.44.8]:46429 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752475AbcEXOOb (ORCPT ); Tue, 24 May 2016 10:14:31 -0400 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 50,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::::::::::::::::::::,RULES_HIT:41:355:379:541:599:800:960:967:968:973:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1534:1543:1593:1594:1711:1730:1747:1777:1792:2393:2525:2560:2563:2682:2685:2693:2828:2859:2933:2937:2939:2942:2945:2947:2951:2954:3022:3138:3139:3140:3141:3142:3355:3622:3865:3866:3867:3868:3871:3872:3873:3934:3936:3938:3941:3944:3947:3950:3953:3956:3959:4250:4321:5007:6117:6119:6120:6737:7576:7809:7901:7903:7904:9025:10004:10400:10848:11026:11232:11473:11658:11783:11914:12043:12048:12296:12438:12517:12519:12679:12740:13439:13894:14093:14097:14181:14659:14721:21080:21433:21434:30012:30054:30056:30064:30080:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:6,LUA_SUMMARY:none X-HE-Tag: edge15_26d02a8d4ee60 X-Filterd-Recvd-Size: 4428 Message-ID: <1464099264.1835.12.camel@perches.com> Subject: Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver From: Joe Perches To: "Bryant G. Ly" , JBottomley@odin.com, martin.petersen@oracle.com, tyreld@linux.vnet.ibm.com, akpm@linux-foundation.org, kvalo@codeaurora.org, davem@davemloft.net, gregkh@linuxfoundation.org, mchehab@osg.samsung.com, jslaby@suse.com, bp@suse.de Cc: linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, bgly Date: Tue, 24 May 2016 07:14:24 -0700 In-Reply-To: <1464097978-88457-1-git-send-email-bryantly@linux.vnet.ibm.com> References: <1464097978-88457-1-git-send-email-bryantly@linux.vnet.ibm.com> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.5.2-0ubuntu1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2016-05-24 at 08:52 -0500, Bryant G. Ly wrote: > From: bgly > > This initial commit contains WIP of the IBM VSCSI Target Fabric > Module. It currently supports read/writes, and I have tested > the ability to create a file backstore with the driver and install > RHEL VIA NIM and then boot up the partition via filio backstore > through the driver. Only trivial notes: Maybe try checkpatch with the --strict option and see if any of the additional messages are important to you. > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -5381,6 +5381,16 @@ S: Supported >  F: drivers/scsi/ibmvscsi/ibmvscsi* >  F: drivers/scsi/ibmvscsi/viosrp.h >   > +IBM Power Virtual SCSI Device Target Driver > +M: Bryant G. Ly > +L: linux-scsi@vger.kernel.org > +L: target-devel@vger.kernel.org > +S: Supported > +F: drivers/scsi/ibmvscsi/ibmvscsis.c > +F:      drivers/scsi/ibmvscsi/ibmvscsis.h > +F: drivers/scsi/libsrp.h > +F:      drivers/scsi/libsrp.c Please use a tab character consistently after the : > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig [] > @@ -847,6 +847,20 @@ config SCSI_IBMVSCSI >     To compile this driver as a module, choose M here: the >     module will be called ibmvscsi. >   > +config SCSI_IBMVSCSIS > +   tristate "IBM Virtual SCSI Server support" > +   depends on PPC_PSERIES && SCSI_SRP && TARGET_CORE > +   help > +     This is the IBM POWER Virtual SCSI Target Server > + > +          The userspace component needed to initialize the driver and > +     documentation can be found: here too. > + > +          https://github.com/powervm/ibmvscsis > + > +          To compile this driver as a module, choose M here: the > +     module will be called ibmvstgt. > + [] > diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile [] > @@ -127,7 +127,9 @@ obj-$(CONFIG_SCSI_LASI700) += 53c700.o lasi700.o >  obj-$(CONFIG_SCSI_SNI_53C710) += 53c700.o sni_53c710.o >  obj-$(CONFIG_SCSI_NSP32) += nsp32.o >  obj-$(CONFIG_SCSI_IPR) += ipr.o > +obj-$(CONFIG_SCSI_SRP)          += libsrp.o >  obj-$(CONFIG_SCSI_IBMVSCSI) += ibmvscsi/ > +obj-$(CONFIG_SCSI_IBMVSCSIS)    += ibmvscsi/ and here > diff --git a/drivers/scsi/ibmvscsi/ibmvscsis.c b/drivers/scsi/ibmvscsi/ibmvscsis.c [] > +static int ibmvscsis_probe(struct vio_dev *vdev, > +    const struct vio_device_id *id); [...] It might be nice to rearrange the code to avoid these forward function declarations. > +static ssize_t ibmvscsis_tpg_enable_store(struct config_item *item, > + const char *page, size_t count) > +{ [] > > + ret = kstrtoul(page, 0, &tmp); > + if (ret < 0) { > + pr_err("Unable to extract ibmvscsis_tpg_store_enable\n"); > + return -EINVAL; > + } It might be nicer to add: #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt before any #include to output all the logging messages with a standardized prefix.  Then all the other logging with an embedded prefix can have the embedded prefix removed too. > + > + if ((tmp != 0) && (tmp != 1)) { > + pr_err("Illegal value for ibmvscsis_tpg_store_enable: %lu\n", > + tmp); > + return -EINVAL; > + } > + > + if (tmp == 1) > + tport->enabled = true; > + else > + tport->enabled = false; tport->enabled = tmp;