linux-kernel - Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1464099264.1835.12.camel@perches.com>
Date:	Tue, 24 May 2016 07:14:24 -0700
From:	Joe Perches <joe@...ches.com>
To:	"Bryant G. Ly" <bryantly@...ux.vnet.ibm.com>, JBottomley@...n.com,
	martin.petersen@...cle.com, tyreld@...ux.vnet.ibm.com,
	akpm@...ux-foundation.org, kvalo@...eaurora.org,
	davem@...emloft.net, gregkh@...uxfoundation.org,
	mchehab@....samsung.com, jslaby@...e.com, bp@...e.de
Cc:	linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
	target-devel@...r.kernel.org, bgly <bgly@...ibm.com>
Subject: Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

On Tue, 2016-05-24 at 08:52 -0500, Bryant G. Ly wrote:
> From: bgly <bgly@...ibm.com>
> 
> 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 <bryantly@...ux.vnet.ibm.com>
> +L:	linux-scsi@...r.kernel.org
> +L:	target-devel@...r.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;