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;