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

From: Joe Perches
Date: Tue May 24 2016 - 10:14:35 EST


On Tue, 2016-05-24 at 08:52 -0500, Bryant G. Ly wrote:
> From: bgly <bgly@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxx>
> +L: linux-scsi@xxxxxxxxxxxxxxx
> +L: target-devel@xxxxxxxxxxxxxxx
> +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;