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