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

2016-06-15 Thread Bryant G. Ly
On 6/14/16, 9:57 AM, "Christoph Hellwig"  wrote:

>On Tue, Jun 14, 2016 at 10:46:09AM +0200, Bart Van Assche wrote:
>> All what's needed is something like the (untested) patch below. As one
>> can see no new function pointers have been added to target_core_fabric_ops.
>> All that has been added are a few new data members. With the patch below
>> ibmvscsis_modify_std_inquiry() can be left out and the target core will
>> pick up the "IBM", "VOPTA", "3303 NVDISK" and "0001" strings through the
>> target_core_fabric_ops instance in the ibmvscsis template. I think this is
>> a much cleaner approach than keeping the ibmvscsis_modify_std_inquiry()
>> function ...
>
>If we really have to, yes.  But then again we manually allow overriding
>the values from configfs, and none of this seems to be a protocol
>requirement but just a workaround for a braindead AIX initiator.  So
>the right thing is documentation telling people how to modify their
>inquiry strings for AIX to work.
>

I can add documentation for modifying their inquiry strings, but the thing is
This is just to support older AIX clients who don’t want to update. We already
Have a working ODM patch to address this but we’d still like to support older
Customers who would like to use this driver with an AIX initiator(client).

Latest patch I have stripped everything out as per your request, which
Does work with a linux client.

-Bryant


--
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 v4] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

2016-06-15 Thread Bryant G. Ly
On 6/14/16, 3:46 AM, "Bart Van Assche"  wrote:

>On 06/14/2016 08:35 AM, Nicholas A. Bellinger wrote:
>> On Fri, 2016-06-10 at 12:05 -0700, Bart Van Assche wrote:
>>> On 06/09/2016 02:26 PM, Bryant G. Ly wrote:
 +/**
 + * ibmvscsis_modify_std_inquiry() - Modify STD Inquiry
 + *
 + * This function modifies the inquiry data prior to sending to initiator
 + * so that we can support current AIX. Internally we are going to
 + * add new ODM entries to support the emulation from LIO. This function
 + * is temporary until those changes are done.
 + */
 +static void ibmvscsis_modify_std_inquiry(struct se_cmd *se_cmd)
 +{
 +  struct se_device *dev = se_cmd->se_dev;
 +  u32 cmd_len = se_cmd->data_length;
 +  u32 repl_len;
 +  unsigned char *buf = NULL;
 +
 +  if (cmd_len <= 8)
 +  return;
 +
 +  buf = transport_kmap_data_sg(se_cmd);
 +  if (buf) {
 +  repl_len = 8;
 +  if (cmd_len - 8 < repl_len)
 +  repl_len = cmd_len - 8;
 +  memcpy(&buf[8], "IBM ", repl_len);
 +
 +  if (cmd_len > 16) {
 +  repl_len = 16;
 +  if (cmd_len - 16 < repl_len)
 +  repl_len = cmd_len - 16;
 +  if (dev->transport->get_device_type(dev) == TYPE_ROM)
 +  memcpy(&buf[16], "VOPTA   ", repl_len);
 +  else
 +  memcpy(&buf[16], "3303  NVDISK", repl_len);
 +  }
 +  if (cmd_len > 32) {
 +  repl_len = 4;
 +  if (cmd_len - 32 < repl_len)
 +  repl_len = cmd_len - 32;
 +  memcpy(&buf[32], "0001", repl_len);
 +  }
 +  transport_kunmap_data_sg(se_cmd);
 +  }
 +}
>>>
>>> Christoph Hellwig had asked you *not* to modify the INQUIRY response 
>>> generated by the target core. Had you noticed that comment?
>> 
>> Yeah, so I wanted to avoid having to add a new target_core_fabric_ops
>> API caller just for this special case, but there might not be another
>> choice..
>
>All what's needed is something like the (untested) patch below. As one
>can see no new function pointers have been added to target_core_fabric_ops.
>All that has been added are a few new data members. With the patch below
>ibmvscsis_modify_std_inquiry() can be left out and the target core will
>pick up the "IBM", "VOPTA", "3303 NVDISK" and "0001" strings through the
>target_core_fabric_ops instance in the ibmvscsis template. I think this is
>a much cleaner approach than keeping the ibmvscsis_modify_std_inquiry()
>function ...
>
>Bart.
>
>[PATCH] target: Make INQUIRY vendor, product ID and product revision 
>configurable
>
>---
> drivers/target/target_core_configfs.c |  4 
> drivers/target/target_core_spc.c  | 22 +-
> include/target/target_core_fabric.h   |  6 ++
> 3 files changed, 27 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/target/target_core_configfs.c 
>b/drivers/target/target_core_configfs.c
>index 2001005..32a74f7 100644
>--- a/drivers/target/target_core_configfs.c
>+++ b/drivers/target/target_core_configfs.c
>@@ -442,6 +442,10 @@ static int target_fabric_tf_ops_check(const struct 
>target_core_fabric_ops *tfo)
>   pr_err("Missing tfo->fabric_drop_tpg()\n");
>   return -EINVAL;
>   }
>+  if (tfo->prod_id_cnt && !tfo->prod_id) {
>+  pr_err("Missing tfo->prod_id\n");
>+  return -EINVAL;
>+  }
>
>   return 0;
> }
>diff --git a/drivers/target/target_core_spc.c 
>b/drivers/target/target_core_spc.c
>index 2a91ed3..32c8435 100644
>--- a/drivers/target/target_core_spc.c
>+++ b/drivers/target/target_core_spc.c
>@@ -66,6 +66,9 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
>*buf)
>   struct se_lun *lun = cmd->se_lun;
>   struct se_device *dev = cmd->se_dev;
>   struct se_session *sess = cmd->se_sess;
>+  struct se_portal_group *tpg = lun->lun_tpg;
>+  const struct target_core_fabric_ops *tfo = tpg->se_tpg_tfo;
>+  const char *vendor, *prod_id, *prod_rev;
>
>   /* Set RMB (removable media) for tape devices */
>   if (dev->transport->get_device_type(dev) == TYPE_TAPE)
>@@ -108,12 +111,21 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned 
>char *buf)
>
>   buf[7] = 0x2; /* CmdQue=1 */
>
>-  memcpy(&buf[8], "LIO-ORG ", 8);
>+  vendor = tfo->t10_vend_id ? : "LIO-ORG";
>+  memset(&buf[8], 0x20, 8);
>+  memcpy(&buf[8], vendor, min_t(int, strlen(vendor), 8));
>+  prod_id = dev->t10_wwn.model;
>+  if (tfo->prod_id) {
>+  int dev_type = dev->transport->get_device_type(dev);
>+
>+  if (dev_type < tfo->prod_id_cnt && tfo->prod_id[dev_type])
>+  prod_id = tfo->prod_id[dev_type];
>+  }
>   memset(&buf[16

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

2016-06-15 Thread Bryant G. Ly
On 6/14/16, 3:09 AM, "Bart Van Assche"  wrote:

>On 06/09/2016 11:26 PM, Bryant G. Ly wrote:
>> This driver is a pick up of the old IBM VIO scsi Target Driver
>> that was started by Nick and Fujita 2-4 years ago.
>> http://comments.gmane.org/gmane.linux.scsi/90119
>
>What kernel version is this patch based on? It doesn't apply cleanly on 
>top of kernel v4.7-rc1 nor on top of kernel v4.6.
>

It should be for 4.5, but latest version I based it off 4.6 stable. So you 
should look at that one.

-Bryant


--
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 v4] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

2016-06-14 Thread Christoph Hellwig
On Tue, Jun 14, 2016 at 10:46:09AM +0200, Bart Van Assche wrote:
> All what's needed is something like the (untested) patch below. As one
> can see no new function pointers have been added to target_core_fabric_ops.
> All that has been added are a few new data members. With the patch below
> ibmvscsis_modify_std_inquiry() can be left out and the target core will
> pick up the "IBM", "VOPTA", "3303 NVDISK" and "0001" strings through the
> target_core_fabric_ops instance in the ibmvscsis template. I think this is
> a much cleaner approach than keeping the ibmvscsis_modify_std_inquiry()
> function ...

If we really have to, yes.  But then again we manually allow overriding
the values from configfs, and none of this seems to be a protocol
requirement but just a workaround for a braindead AIX initiator.  So
the right thing is documentation telling people how to modify their
inquiry strings for AIX to work.
--
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 v4] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

2016-06-14 Thread Bart Van Assche
On 06/14/2016 08:35 AM, Nicholas A. Bellinger wrote:
> On Fri, 2016-06-10 at 12:05 -0700, Bart Van Assche wrote:
>> On 06/09/2016 02:26 PM, Bryant G. Ly wrote:
>>> +/**
>>> + * ibmvscsis_modify_std_inquiry() - Modify STD Inquiry
>>> + *
>>> + * This function modifies the inquiry data prior to sending to initiator
>>> + * so that we can support current AIX. Internally we are going to
>>> + * add new ODM entries to support the emulation from LIO. This function
>>> + * is temporary until those changes are done.
>>> + */
>>> +static void ibmvscsis_modify_std_inquiry(struct se_cmd *se_cmd)
>>> +{
>>> +   struct se_device *dev = se_cmd->se_dev;
>>> +   u32 cmd_len = se_cmd->data_length;
>>> +   u32 repl_len;
>>> +   unsigned char *buf = NULL;
>>> +
>>> +   if (cmd_len <= 8)
>>> +   return;
>>> +
>>> +   buf = transport_kmap_data_sg(se_cmd);
>>> +   if (buf) {
>>> +   repl_len = 8;
>>> +   if (cmd_len - 8 < repl_len)
>>> +   repl_len = cmd_len - 8;
>>> +   memcpy(&buf[8], "IBM ", repl_len);
>>> +
>>> +   if (cmd_len > 16) {
>>> +   repl_len = 16;
>>> +   if (cmd_len - 16 < repl_len)
>>> +   repl_len = cmd_len - 16;
>>> +   if (dev->transport->get_device_type(dev) == TYPE_ROM)
>>> +   memcpy(&buf[16], "VOPTA   ", repl_len);
>>> +   else
>>> +   memcpy(&buf[16], "3303  NVDISK", repl_len);
>>> +   }
>>> +   if (cmd_len > 32) {
>>> +   repl_len = 4;
>>> +   if (cmd_len - 32 < repl_len)
>>> +   repl_len = cmd_len - 32;
>>> +   memcpy(&buf[32], "0001", repl_len);
>>> +   }
>>> +   transport_kunmap_data_sg(se_cmd);
>>> +   }
>>> +}
>>
>> Christoph Hellwig had asked you *not* to modify the INQUIRY response 
>> generated by the target core. Had you noticed that comment?
> 
> Yeah, so I wanted to avoid having to add a new target_core_fabric_ops
> API caller just for this special case, but there might not be another
> choice..

All what's needed is something like the (untested) patch below. As one
can see no new function pointers have been added to target_core_fabric_ops.
All that has been added are a few new data members. With the patch below
ibmvscsis_modify_std_inquiry() can be left out and the target core will
pick up the "IBM", "VOPTA", "3303 NVDISK" and "0001" strings through the
target_core_fabric_ops instance in the ibmvscsis template. I think this is
a much cleaner approach than keeping the ibmvscsis_modify_std_inquiry()
function ...

Bart.

[PATCH] target: Make INQUIRY vendor, product ID and product revision 
configurable

---
 drivers/target/target_core_configfs.c |  4 
 drivers/target/target_core_spc.c  | 22 +-
 include/target/target_core_fabric.h   |  6 ++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 2001005..32a74f7 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -442,6 +442,10 @@ static int target_fabric_tf_ops_check(const struct 
target_core_fabric_ops *tfo)
pr_err("Missing tfo->fabric_drop_tpg()\n");
return -EINVAL;
}
+   if (tfo->prod_id_cnt && !tfo->prod_id) {
+   pr_err("Missing tfo->prod_id\n");
+   return -EINVAL;
+   }
 
return 0;
 }
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 2a91ed3..32c8435 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -66,6 +66,9 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
struct se_lun *lun = cmd->se_lun;
struct se_device *dev = cmd->se_dev;
struct se_session *sess = cmd->se_sess;
+   struct se_portal_group *tpg = lun->lun_tpg;
+   const struct target_core_fabric_ops *tfo = tpg->se_tpg_tfo;
+   const char *vendor, *prod_id, *prod_rev;
 
/* Set RMB (removable media) for tape devices */
if (dev->transport->get_device_type(dev) == TYPE_TAPE)
@@ -108,12 +111,21 @@ spc_emulate_inquiry_std(struct se_cmd *cmd, unsigned char 
*buf)
 
buf[7] = 0x2; /* CmdQue=1 */
 
-   memcpy(&buf[8], "LIO-ORG ", 8);
+   vendor = tfo->t10_vend_id ? : "LIO-ORG";
+   memset(&buf[8], 0x20, 8);
+   memcpy(&buf[8], vendor, min_t(int, strlen(vendor), 8));
+   prod_id = dev->t10_wwn.model;
+   if (tfo->prod_id) {
+   int dev_type = dev->transport->get_device_type(dev);
+
+   if (dev_type < tfo->prod_id_cnt && tfo->prod_id[dev_type])
+   prod_id = tfo->prod_id[dev_type];
+   }
memset(&buf[16], 0x20, 16);
-   memcpy(&buf[16], dev->t10_wwn.model,
-  min_t(size_t, strlen(dev->t10_wwn.mode

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

2016-06-14 Thread Bart Van Assche

On 06/09/2016 11:26 PM, Bryant G. Ly wrote:

This driver is a pick up of the old IBM VIO scsi Target Driver
that was started by Nick and Fujita 2-4 years ago.
http://comments.gmane.org/gmane.linux.scsi/90119


What kernel version is this patch based on? It doesn't apply cleanly on 
top of kernel v4.7-rc1 nor on top of kernel v4.6.


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 v4] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

2016-06-13 Thread Nicholas A. Bellinger
On Fri, 2016-06-10 at 12:05 -0700, Bart Van Assche wrote:
> On 06/09/2016 02:26 PM, Bryant G. Ly wrote:
> >  drivers/scsi/Kconfig |   27 +-
> >  drivers/scsi/Makefile|2 +-
> >  drivers/scsi/ibmvscsi/Makefile   |4 +
> >  drivers/scsi/ibmvscsi/ibmvscsi_tgt.c | 4184 
> > ++
> >  drivers/scsi/ibmvscsi/ibmvscsi_tgt.h |  347 +++
> >  drivers/scsi/ibmvscsi/libsrp.c   |  427 
> >  drivers/scsi/ibmvscsi/libsrp.h   |  123 +
> >  drivers/scsi/libsrp.c|  447 
> >  include/scsi/libsrp.h|   78 -
> 
> Sorry that I hadn't noticed this before but since no code is shared 
> between the IBM vSCSI initiator and target drivers please consider 
> moving the ibmvscsi_tgt.[ch] and libsrp.[ch] source files into a new 
> directory. This will keep initiator and target drivers are in separate 
> directories. This is the approach used for other upstream protocol 
> drivers for which no code is shared between initiator and target drivers 
> (e.g. the iSER, SRP initiator and target drivers).

Yeah, if no code is shared, and will ever be shared, we might as well
merge libsrc.[c,h] directly into ibmvscsi_tgt.[c,h].

> > +/**
> > + * ibmvscsis_modify_std_inquiry() - Modify STD Inquiry
> > + *
> > + * This function modifies the inquiry data prior to sending to initiator
> > + * so that we can support current AIX. Internally we are going to
> > + * add new ODM entries to support the emulation from LIO. This function
> > + * is temporary until those changes are done.
> > + */
> > +static void ibmvscsis_modify_std_inquiry(struct se_cmd *se_cmd)
> > +{
> > +   struct se_device *dev = se_cmd->se_dev;
> > +   u32 cmd_len = se_cmd->data_length;
> > +   u32 repl_len;
> > +   unsigned char *buf = NULL;
> > +
> > +   if (cmd_len <= 8)
> > +   return;
> > +
> > +   buf = transport_kmap_data_sg(se_cmd);
> > +   if (buf) {
> > +   repl_len = 8;
> > +   if (cmd_len - 8 < repl_len)
> > +   repl_len = cmd_len - 8;
> > +   memcpy(&buf[8], "IBM ", repl_len);
> > +
> > +   if (cmd_len > 16) {
> > +   repl_len = 16;
> > +   if (cmd_len - 16 < repl_len)
> > +   repl_len = cmd_len - 16;
> > +   if (dev->transport->get_device_type(dev) == TYPE_ROM)
> > +   memcpy(&buf[16], "VOPTA   ", repl_len);
> > +   else
> > +   memcpy(&buf[16], "3303  NVDISK", repl_len);
> > +   }
> > +   if (cmd_len > 32) {
> > +   repl_len = 4;
> > +   if (cmd_len - 32 < repl_len)
> > +   repl_len = cmd_len - 32;
> > +   memcpy(&buf[32], "0001", repl_len);
> > +   }
> > +   transport_kunmap_data_sg(se_cmd);
> > +   }
> > +}
> 
> Christoph Hellwig had asked you *not* to modify the INQUIRY response 
> generated by the target core. Had you noticed that comment?
> 

Yeah, so I wanted to avoid having to add a new target_core_fabric_ops
API caller just for this special case, but there might not be another
choice..

Previously, ibmvscsi-tgt was simply picking off INQUIRY and doing
completion, setting the special payload buffer contents without actually
submitting to target-core.

I'd probably still prefer the original method to avoid the extra
target_core_fabric_ops API caller that will never be used by anybody
else, unless there are issues with AIX ordered tasks wrt INQUIRY not
actually making it into target-core.

--
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 v4] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

2016-06-10 Thread Bart Van Assche

On 06/09/2016 02:26 PM, Bryant G. Ly wrote:

 drivers/scsi/Kconfig |   27 +-
 drivers/scsi/Makefile|2 +-
 drivers/scsi/ibmvscsi/Makefile   |4 +
 drivers/scsi/ibmvscsi/ibmvscsi_tgt.c | 4184 ++
 drivers/scsi/ibmvscsi/ibmvscsi_tgt.h |  347 +++
 drivers/scsi/ibmvscsi/libsrp.c   |  427 
 drivers/scsi/ibmvscsi/libsrp.h   |  123 +
 drivers/scsi/libsrp.c|  447 
 include/scsi/libsrp.h|   78 -


Sorry that I hadn't noticed this before but since no code is shared 
between the IBM vSCSI initiator and target drivers please consider 
moving the ibmvscsi_tgt.[ch] and libsrp.[ch] source files into a new 
directory. This will keep initiator and target drivers are in separate 
directories. This is the approach used for other upstream protocol 
drivers for which no code is shared between initiator and target drivers 
(e.g. the iSER, SRP initiator and target drivers).



+#include 


Does this driver create kernel threads? Is inclusion of this header file 
needed?



+#include 


We do not want #if LINUX_VERSION_CODE < KERNEL_VERSION(...) statements 
in upstream code so please leave this #include and the version tests out.



+/**
+ * ibmvscsis_modify_std_inquiry() - Modify STD Inquiry
+ *
+ * This function modifies the inquiry data prior to sending to initiator
+ * so that we can support current AIX. Internally we are going to
+ * add new ODM entries to support the emulation from LIO. This function
+ * is temporary until those changes are done.
+ */
+static void ibmvscsis_modify_std_inquiry(struct se_cmd *se_cmd)
+{
+   struct se_device *dev = se_cmd->se_dev;
+   u32 cmd_len = se_cmd->data_length;
+   u32 repl_len;
+   unsigned char *buf = NULL;
+
+   if (cmd_len <= 8)
+   return;
+
+   buf = transport_kmap_data_sg(se_cmd);
+   if (buf) {
+   repl_len = 8;
+   if (cmd_len - 8 < repl_len)
+   repl_len = cmd_len - 8;
+   memcpy(&buf[8], "IBM ", repl_len);
+
+   if (cmd_len > 16) {
+   repl_len = 16;
+   if (cmd_len - 16 < repl_len)
+   repl_len = cmd_len - 16;
+   if (dev->transport->get_device_type(dev) == TYPE_ROM)
+   memcpy(&buf[16], "VOPTA   ", repl_len);
+   else
+   memcpy(&buf[16], "3303  NVDISK", repl_len);
+   }
+   if (cmd_len > 32) {
+   repl_len = 4;
+   if (cmd_len - 32 < repl_len)
+   repl_len = cmd_len - 32;
+   memcpy(&buf[32], "0001", repl_len);
+   }
+   transport_kunmap_data_sg(se_cmd);
+   }
+}


Christoph Hellwig had asked you *not* to modify the INQUIRY response 
generated by the target core. Had you noticed that comment?


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