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

[Christoph Hellwig]

On Mon, Jun 13, 2016 at 11:23:52PM -0700, Nicholas A. Bellinger wrote:
> AFAICT, the problem with this improvement is that it's not stable
> material, which means that Canonical would have to carry this separately
> beyond linux-4.4.y, in order to support ibmvscsi-tgt.

So who cares?  If they have a business interest in backporting the
driver they'll need to figure out a way.  None of that should matter in
any way whatsover for mainline.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nicholas A. Bellinger]

On Fri, 2016-06-10 at 12:03 -0700, Bart Van Assche wrote:
> On 05/24/2016 01:00 PM, Bryant G Ly wrote:
> > Quoting Bart Van Assche :
> >> On 05/24/2016 06:52 AM, Bryant G. Ly wrote:
> >>> +static uint64_t ibmvscsis_unpack_lun(const uint8_t *lun, int len)
> >>> +{
> >>> +uint64_t res = NO_SUCH_LUN;
> >>> +int addressing_method;
> >>> +
> >>> +if (unlikely(len < 2)) {
> >>> +pr_err("Illegal LUN length %d, expected 2 bytes or more\n",
> >>> +len);
> >>> +goto out;
> >>> +}
> >>> +
> >>> +switch (len) {
> >>> +case 8:
> >>> +if ((*((__be64 *)lun) & cpu_to_be64(0xLL))
> >>> != 0)
> >>> +goto out_err;
> >>> +break;
> >>> +case 4:
> >>> +if (*((__be16 *)[2]) != 0)
> >>> +goto out_err;
> >>> +break;
> >>> +case 6:
> >>> +if (*((__be32 *)[2]) != 0)
> >>> +goto out_err;
> >>> +break;
> >>> +case 2:
> >>> +break;
> >>> +default:
> >>> +goto out_err;
> >>> +}
> >>> +
> >>> +addressing_method = (*lun) >> 6; /* highest two bits of byte 0 */
> >>> +switch (addressing_method) {
> >>> +case SCSI_LUN_ADDR_METHOD_PERIPHERAL:
> >>> +case SCSI_LUN_ADDR_METHOD_FLAT:
> >>> +case SCSI_LUN_ADDR_METHOD_LUN:
> >>> +res = *(lun + 1) | (((*lun) & 0x3f) << 8);
> >>> +break;
> >>> +
> >>> +case SCSI_LUN_ADDR_METHOD_EXTENDED_LUN:
> >>> +default:
> >>> +pr_err("Unimplemented LUN addressing method %u\n",
> >>> +addressing_method);
> >>> +break;
> >>> +}
> >>> +
> >>> +out:
> >>> +return res;
> >>> +out_err:
> >>> +pr_err("Support for multi-level LUNs has not yet been
> >>> implemented\n");
> >>> +goto out;
> >>> +}
> >>
> >> In the above function there is nothing that is specific to the VIO
> >> mechanism. Please consider to merge this function with
> >> scsilun_to_int(), e.g. by introducing a new function that accepts a
> >> SCSI LUN and the addressing method as arguments and by making
> >> scsilun_to_int() call that function with
> >> SCSI_LUN_ADDR_METHOD_PERIPHERAL as one of its arguments.
> >>
> >
> > We are going to keep this here atm, until we can make sure canonical can
> > pick up
> > these changes, since we have a deliverable for a 3rd party vendor who is
> > going to
> > fully utilize this driver. Once they can pick it up in 4.4 kernel I will
> > make the
> > change you recommended / move the scsi_lun_addr_method addressed below.
> > I will also
> > address the changes that ib_srpt needs to utilize this new function too.
> 
> A much better approach would be to move this function into the SCSI core 
> and to add a copy of that function to the backported driver that will be 
> provided to Canonical.

AFAICT, the problem with this improvement is that it's not stable
material, which means that Canonical would have to carry this separately
beyond linux-4.4.y, in order to support ibmvscsi-tgt.

As-is, it looks like we'll have two target-core fixes to support
ibmvscsi-tgt, both of which are considered stable material at this point
and will eventually make it to linux-4.4.y.

That said Bryant, I still don't have an objection for the initial commit
to use it's own internal ibmvscsis_unpack_lun(), but it would be nice in
the patch series containing the initial ibmvscsi-tgt commit to include
an incremental patch to make this common code, as Bart has highlighted.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On 05/24/2016 01:00 PM, Bryant G Ly wrote:

Quoting Bart Van Assche :

On 05/24/2016 06:52 AM, Bryant G. Ly wrote:

+static uint64_t ibmvscsis_unpack_lun(const uint8_t *lun, int len)
+{
+uint64_t res = NO_SUCH_LUN;
+int addressing_method;
+
+if (unlikely(len < 2)) {
+pr_err("Illegal LUN length %d, expected 2 bytes or more\n",
+len);
+goto out;
+}
+
+switch (len) {
+case 8:
+if ((*((__be64 *)lun) & cpu_to_be64(0xLL))
!= 0)
+goto out_err;
+break;
+case 4:
+if (*((__be16 *)[2]) != 0)
+goto out_err;
+break;
+case 6:
+if (*((__be32 *)[2]) != 0)
+goto out_err;
+break;
+case 2:
+break;
+default:
+goto out_err;
+}
+
+addressing_method = (*lun) >> 6; /* highest two bits of byte 0 */
+switch (addressing_method) {
+case SCSI_LUN_ADDR_METHOD_PERIPHERAL:
+case SCSI_LUN_ADDR_METHOD_FLAT:
+case SCSI_LUN_ADDR_METHOD_LUN:
+res = *(lun + 1) | (((*lun) & 0x3f) << 8);
+break;
+
+case SCSI_LUN_ADDR_METHOD_EXTENDED_LUN:
+default:
+pr_err("Unimplemented LUN addressing method %u\n",
+addressing_method);
+break;
+}
+
+out:
+return res;
+out_err:
+pr_err("Support for multi-level LUNs has not yet been
implemented\n");
+goto out;
+}


In the above function there is nothing that is specific to the VIO
mechanism. Please consider to merge this function with
scsilun_to_int(), e.g. by introducing a new function that accepts a
SCSI LUN and the addressing method as arguments and by making
scsilun_to_int() call that function with
SCSI_LUN_ADDR_METHOD_PERIPHERAL as one of its arguments.



We are going to keep this here atm, until we can make sure canonical can
pick up
these changes, since we have a deliverable for a 3rd party vendor who is
going to
fully utilize this driver. Once they can pick it up in 4.4 kernel I will
make the
change you recommended / move the scsi_lun_addr_method addressed below.
I will also
address the changes that ib_srpt needs to utilize this new function too.


A much better approach would be to move this function into the SCSI core 
and to add a copy of that function to the backported driver that will be 
provided to Canonical.


Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bryant G Ly]

Quoting Bart Van Assche :


On 05/24/2016 06:52 AM, Bryant G. Ly wrote:

+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:
+
+  https://github.com/powervm/ibmvscsis
+
+  To compile this driver as a module, choose M here: the
+ module will be called ibmvstgt.
+


This driver has confused Linux users before. Most people know SRP as  
a SCSI transport layer that is used for communication between  
servers. This driver however uses the SRP protocol for communication  
between guests and/or the host that run on the same server. Please  
add this information to the above help text, together with a  
reference to the VIO protocol documentation in the POWER  
architecture manual.




Done


+#include 


Every Linux user can query the kernel version by running the uname  
command. Is it really useful to print the kernel version  
(UTS_RELEASE) when this driver is loaded?




Done


+#include "ibmvscsi.h"


The above include directive indirectly includes a whole bunch of  
SCSI initiator driver header files. We do not want that initiator  
header files are included in the source code of a target driver. If  
it is necessary to share definitions of symbolic constants between  
the initiator and the target driver please move these into a new  
header file.




Done


+static inline long h_send_crq(struct ibmvscsis_adapter *adapter,
+   u64 word1, u64 word2)
+{
+   long rc;
+   struct vio_dev *vdev = adapter->dma_dev;
+
+   pr_debug("ibmvscsis: ibmvscsis_send_crq(0x%x, 0x%016llx, 0x%016llx)\n",
+   vdev->unit_address, word1, word2);
+


As Joe Perches already asked, please define pr_fmt() instead of  
including the kernel module name in every pr_debug() statement.




Done


+static char system_id[64] = "";
+static char partition_name[97] = "UNKNOWN";


Where do these magic constants come from and what is their meaning?  
Please consider to introduce symbolic names for these constants.




Done


+static ssize_t ibmvscsis_tpg_enable_show(struct config_item *item,
+   char *page)
+{
+   struct se_portal_group *se_tpg = to_tpg(item);
+   struct ibmvscsis_tport *tport = container_of(se_tpg,
+   struct ibmvscsis_tport, se_tpg);
+
+   return snprintf(page, PAGE_SIZE, "%d\n", (tport->enabled) ? 1 : 0);
+}


Have you considered to use MODULE_VERSION() instead of introducing a  
sysfs attribute to export the module version? An advantage of  
MODULE_VERSION() is that it allows to query the module version  
without having to load a kernel module.




Done


+static void ibmvscsis_modify_std_inquiry(struct se_cmd *se_cmd)
+{
+   struct se_device *dev = se_cmd->se_dev;
+   unsigned char *buf = NULL;
+   u32 cmd_len = se_cmd->data_length;
+
+   if (cmd_len <= INQ_DATA_OFFSET)
+   return;
+
+   buf = transport_kmap_data_sg(se_cmd);
+   if (buf) {
+   memcpy([8], "IBM", 8);
+   if (dev->transport->get_device_type(dev) == TYPE_ROM)
+   memcpy([16], "VOPTA   ", 16);
+   else
+   memcpy([16], "3303  NVDISK", 16);
+   memcpy([32], "0001", 4);
+   transport_kunmap_data_sg(se_cmd);
+   }
+}


Shouldn't a sense code be returned to the initiator if this function fails?



This function never fails? Also this is a temporary function that will go away
soon. It's to address AIX not recognizing target emulation of inquiry, but
we have already discussed internally to add these ODM entries into AIX.


+   default:
+   pr_err("ibmvscsis: unknown task mgmt func %d\n",
+   srp_tsk->tsk_mgmt_func);
+   cmd->se_cmd.se_tmr_req->response =
+   TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
+   rc = -1;
+   break;
+   }
+
+   if (!rc) {


Please consider changing the above "break" into "goto fail" such  
that the if (!rc) can be left out and such that the indentation of  
the code below if (!rc) can be reduced.




Done


+static int ibmvscsis_queuecommand(struct ibmvscsis_adapter *adapter,
+ struct iu_entry *iue)
+{
+   struct srp_cmd *cmd = iue->sbuf->buf;
+   struct scsi_cmnd *sc;
+   struct ibmvscsis_cmnd *vsc;
+   int ret;
+
+   pr_debug("ibmvscsis: ibmvscsis_queuecommand\n");
+
+   vsc = kzalloc(sizeof(*vsc), GFP_KERNEL);


Allocating memory in the command processing path in a SCSI driver  
usually causes suboptimal performance. Other LIO target drivers  

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

[Greg KH]

On Tue, May 24, 2016 at 09:44:43AM -0700, Bart Van Assche wrote:
> On 05/24/2016 09:34 AM, Greg KH wrote:
> > On Tue, May 24, 2016 at 09:25:05AM -0700, Bart Van Assche wrote:
> > > > +static inline long h_send_crq(struct ibmvscsis_adapter *adapter,
> > > > +   u64 word1, u64 word2)
> > > > +{
> > > > +   long rc;
> > > > +   struct vio_dev *vdev = adapter->dma_dev;
> > > > +
> > > > +   pr_debug("ibmvscsis: ibmvscsis_send_crq(0x%x, 0x%016llx, 
> > > > 0x%016llx)\n",
> > > > +   vdev->unit_address, word1, word2);
> > > > +
> > > 
> > > As Joe Perches already asked, please define pr_fmt() instead of including
> > > the kernel module name in every pr_debug() statement.
> > 
> > Even better, as this is a driver, it should be using dev_*() calls
> > instead of pr_*() calls to properly identify the device and driver that
> > is making the message.  No driver should be using pr_*() except in
> > _very_ limited usages.
> 
> Hi Greg,
> 
> The reason I recommended pr_debug() is because ibmvscsis is a LIO target
> driver and I don't think a struct device is associated with a LIO target
> driver. See e.g. target_register_template() in
> drivers/target/target_core_configfs.c.

That seems messed up, there should be a struct device somewhere to
use...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On 05/24/2016 09:34 AM, Greg KH wrote:

On Tue, May 24, 2016 at 09:25:05AM -0700, Bart Van Assche wrote:

+static inline long h_send_crq(struct ibmvscsis_adapter *adapter,
+   u64 word1, u64 word2)
+{
+   long rc;
+   struct vio_dev *vdev = adapter->dma_dev;
+
+   pr_debug("ibmvscsis: ibmvscsis_send_crq(0x%x, 0x%016llx, 0x%016llx)\n",
+   vdev->unit_address, word1, word2);
+


As Joe Perches already asked, please define pr_fmt() instead of including
the kernel module name in every pr_debug() statement.


Even better, as this is a driver, it should be using dev_*() calls
instead of pr_*() calls to properly identify the device and driver that
is making the message.  No driver should be using pr_*() except in
_very_ limited usages.


Hi Greg,

The reason I recommended pr_debug() is because ibmvscsis is a LIO target 
driver and I don't think a struct device is associated with a LIO target 
driver. See e.g. target_register_template() in 
drivers/target/target_core_configfs.c.


Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

On Tue, May 24, 2016 at 09:25:05AM -0700, Bart Van Assche wrote:
> > +static inline long h_send_crq(struct ibmvscsis_adapter *adapter,
> > +   u64 word1, u64 word2)
> > +{
> > +   long rc;
> > +   struct vio_dev *vdev = adapter->dma_dev;
> > +
> > +   pr_debug("ibmvscsis: ibmvscsis_send_crq(0x%x, 0x%016llx, 0x%016llx)\n",
> > +   vdev->unit_address, word1, word2);
> > +
> 
> As Joe Perches already asked, please define pr_fmt() instead of including
> the kernel module name in every pr_debug() statement.

Even better, as this is a driver, it should be using dev_*() calls
instead of pr_*() calls to properly identify the device and driver that
is making the message.  No driver should be using pr_*() except in
_very_ limited usages.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On 05/24/2016 06:52 AM, Bryant G. Ly wrote:

+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:
+
+  https://github.com/powervm/ibmvscsis
+
+  To compile this driver as a module, choose M here: the
+ module will be called ibmvstgt.
+


This driver has confused Linux users before. Most people know SRP as a 
SCSI transport layer that is used for communication between servers. 
This driver however uses the SRP protocol for communication between 
guests and/or the host that run on the same server. Please add this 
information to the above help text, together with a reference to the VIO 
protocol documentation in the POWER architecture manual.



+#include 


Every Linux user can query the kernel version by running the uname 
command. Is it really useful to print the kernel version (UTS_RELEASE) 
when this driver is loaded?



+#include "ibmvscsi.h"


The above include directive indirectly includes a whole bunch of SCSI 
initiator driver header files. We do not want that initiator header 
files are included in the source code of a target driver. If it is 
necessary to share definitions of symbolic constants between the 
initiator and the target driver please move these into a new header file.



+static inline long h_send_crq(struct ibmvscsis_adapter *adapter,
+   u64 word1, u64 word2)
+{
+   long rc;
+   struct vio_dev *vdev = adapter->dma_dev;
+
+   pr_debug("ibmvscsis: ibmvscsis_send_crq(0x%x, 0x%016llx, 0x%016llx)\n",
+   vdev->unit_address, word1, word2);
+


As Joe Perches already asked, please define pr_fmt() instead of 
including the kernel module name in every pr_debug() statement.



+static char system_id[64] = "";
+static char partition_name[97] = "UNKNOWN";


Where do these magic constants come from and what is their meaning? 
Please consider to introduce symbolic names for these constants.



+static ssize_t ibmvscsis_tpg_enable_show(struct config_item *item,
+   char *page)
+{
+   struct se_portal_group *se_tpg = to_tpg(item);
+   struct ibmvscsis_tport *tport = container_of(se_tpg,
+   struct ibmvscsis_tport, se_tpg);
+
+   return snprintf(page, PAGE_SIZE, "%d\n", (tport->enabled) ? 1 : 0);
+}


Have you considered to use MODULE_VERSION() instead of introducing a 
sysfs attribute to export the module version? An advantage of 
MODULE_VERSION() is that it allows to query the module version without 
having to load a kernel module.



+static void ibmvscsis_modify_std_inquiry(struct se_cmd *se_cmd)
+{
+   struct se_device *dev = se_cmd->se_dev;
+   unsigned char *buf = NULL;
+   u32 cmd_len = se_cmd->data_length;
+
+   if (cmd_len <= INQ_DATA_OFFSET)
+   return;
+
+   buf = transport_kmap_data_sg(se_cmd);
+   if (buf) {
+   memcpy([8], "IBM", 8);
+   if (dev->transport->get_device_type(dev) == TYPE_ROM)
+   memcpy([16], "VOPTA   ", 16);
+   else
+   memcpy([16], "3303  NVDISK", 16);
+   memcpy([32], "0001", 4);
+   transport_kunmap_data_sg(se_cmd);
+   }
+}


Shouldn't a sense code be returned to the initiator if this function fails?


+   default:
+   pr_err("ibmvscsis: unknown task mgmt func %d\n",
+   srp_tsk->tsk_mgmt_func);
+   cmd->se_cmd.se_tmr_req->response =
+   TMR_TASK_MGMT_FUNCTION_NOT_SUPPORTED;
+   rc = -1;
+   break;
+   }
+
+   if (!rc) {


Please consider changing the above "break" into "goto fail" such that 
the if (!rc) can be left out and such that the indentation of the code 
below if (!rc) can be reduced.



+static int ibmvscsis_queuecommand(struct ibmvscsis_adapter *adapter,
+ struct iu_entry *iue)
+{
+   struct srp_cmd *cmd = iue->sbuf->buf;
+   struct scsi_cmnd *sc;
+   struct ibmvscsis_cmnd *vsc;
+   int ret;
+
+   pr_debug("ibmvscsis: ibmvscsis_queuecommand\n");
+
+   vsc = kzalloc(sizeof(*vsc), GFP_KERNEL);


Allocating memory in the command processing path in a SCSI driver 
usually causes suboptimal performance. Other LIO target drivers 
preallocate such buffers before I/O starts.



+static uint64_t ibmvscsis_unpack_lun(const uint8_t *lun, int len)
+{
+   uint64_t res = NO_SUCH_LUN;
+   int addressing_method;
+
+   if (unlikely(len < 2)) {
+   pr_err("Illegal LUN length %d, expected 2 bytes or more\n",
+   len);
+   goto out;
+   }
+
+   

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

[Greg KH]

On Tue, May 24, 2016 at 08:52:58AM -0500, Bryant G. Ly wrote:
> From: bgly 

Really?  Please go get a proper review from the other internal IBM
developers to fix this, and the other obvious problems with your patch,
before you send it publically and force us to tell you these things...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Joe Perches]

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-de...@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, );
> + 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;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bryant G. Ly]

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.

Signed-off-by: bryantly 
---
 MAINTAINERS   |   10 +
 drivers/scsi/Kconfig  |   24 +
 drivers/scsi/Makefile |2 +
 drivers/scsi/ibmvscsi/Makefile|1 +
 drivers/scsi/ibmvscsi/ibmvscsis.c | 2033 +
 drivers/scsi/ibmvscsi/ibmvscsis.h |  160 +++
 drivers/scsi/libsrp.c |  387 +++
 include/scsi/libsrp.h |   95 ++
 8 files changed, 2712 insertions(+)
 create mode 100644 drivers/scsi/ibmvscsi/ibmvscsis.c
 create mode 100644 drivers/scsi/ibmvscsi/ibmvscsis.h
 create mode 100644 drivers/scsi/libsrp.c
 create mode 100644 include/scsi/libsrp.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6ee06ea..b520e6c 100644
--- 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-de...@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
+
 IBM Power Virtual FC Device Drivers
 M: Tyrel Datwyler 
 L: linux-scsi@vger.kernel.org
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index e2f31c9..6adf8f1 100644
--- 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:
+
+  https://github.com/powervm/ibmvscsis
+
+  To compile this driver as a module, choose M here: the
+ module will be called ibmvstgt.
+
 config SCSI_IBMVFC
tristate "IBM Virtual FC support"
depends on PPC_PSERIES && SCSI
@@ -1728,6 +1742,16 @@ config SCSI_PM8001
  This driver supports PMC-Sierra PCIE SAS/SATA 8x6G SPC 8001 chip
  based host adapters.
 
+config SCSI_SRP
+   tristate "SCSI RDMA Protocol helper library"
+   depends on SCSI && PCI
+   help
+ This scsi srp module is a library for ibmvscsi target driver.
+ If you wish to use SRP target drivers, say Y.
+
+ To compile this driver as a module, choose M here. The module will
+ be called libsrp.
+
 config SCSI_BFA_FC
tristate "Brocade BFA Fibre Channel Support"
depends on PCI && SCSI
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 862ab4e..8692dd4 100644
--- 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/
 obj-$(CONFIG_SCSI_IBMVFC)  += ibmvscsi/
 obj-$(CONFIG_SCSI_HPTIOP)  += hptiop.o
 obj-$(CONFIG_SCSI_STEX)+= stex.o
diff --git a/drivers/scsi/ibmvscsi/Makefile b/drivers/scsi/ibmvscsi/Makefile
index 3840c64..b241a567 100644
--- a/drivers/scsi/ibmvscsi/Makefile
+++ b/drivers/scsi/ibmvscsi/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SCSI_IBMVSCSI)+= ibmvscsi.o
+obj-$(CONFIG_SCSI_IBMVSCSIS)+= ibmvscsis.o
 obj-$(CONFIG_SCSI_IBMVFC)  += ibmvfc.o
diff --git a/drivers/scsi/ibmvscsi/ibmvscsis.c 
b/drivers/scsi/ibmvscsi/ibmvscsis.c
new file mode 100644
index 000..c7eb347
--- /dev/null
+++ b/drivers/scsi/ibmvscsi/ibmvscsis.c
@@ -0,0 +1,2033 @@
+/***
+ * IBM Virtual SCSI Target Driver
+ * Copyright (C) 2003-2005 Dave Boutcher (boutc...@us.ibm.com) IBM Corp.
+ *Santiago Leon (san...@us.ibm.com) IBM Corp.
+ *Linda Xie (l...@us.ibm.com) IBM Corp.
+ *
+ * Copyright (C) 2005-2011 FUJITA Tomonori 
+ * Copyright (C) 2010 Nicholas A. Bellinger 
+ * Copyright (C) 2016 Bryant G. Ly  IBM Corp.
+ *
+ * Authors: Bryant G. Ly 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms