2020-11-17 18:54:02

by Tyrel Datwyler

[permalink] [raw]
Subject: [PATCH 1/3] ibmvfc: byte swap login_buf.resp values in attribute show functions

Both ibmvfc_show_host_(capabilities|npiv_version) functions retrieve
values from vhost->login_buf.resp buffer. This is the MAD response
buffer from the VIOS and as such any multi-byte non-string values are in
big endian format.

Byte swap these values to host cpu endian format for better human
readability.

Signed-off-by: Tyrel Datwyler <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 070cf516b98f..01fe65de9086 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -3025,7 +3025,7 @@ static ssize_t ibmvfc_show_host_npiv_version(struct device *dev,
{
struct Scsi_Host *shost = class_to_shost(dev);
struct ibmvfc_host *vhost = shost_priv(shost);
- return snprintf(buf, PAGE_SIZE, "%d\n", vhost->login_buf->resp.version);
+ return snprintf(buf, PAGE_SIZE, "%d\n", be32_to_cpu(vhost->login_buf->resp.version));
}

static ssize_t ibmvfc_show_host_capabilities(struct device *dev,
@@ -3033,7 +3033,7 @@ static ssize_t ibmvfc_show_host_capabilities(struct device *dev,
{
struct Scsi_Host *shost = class_to_shost(dev);
struct ibmvfc_host *vhost = shost_priv(shost);
- return snprintf(buf, PAGE_SIZE, "%llx\n", vhost->login_buf->resp.capabilities);
+ return snprintf(buf, PAGE_SIZE, "%llx\n", be64_to_cpu(vhost->login_buf->resp.capabilities));
}

/**
--
2.27.0


2020-11-17 18:55:11

by Tyrel Datwyler

[permalink] [raw]
Subject: [PATCH 3/3] ibmvfc: use correlation token to tag commands

The vfcFrame correlation field is 64bit handle that is intended to trace
I/O operations through both the client stack and VIOS stack when the
underlying physical FC adapter supports tagging.

Tag vfcFrames with the associated ibmvfc_event pointer handle.

Signed-off-by: Tyrel Datwyler <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 0cab4b852b48..3922441a117d 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1693,6 +1693,8 @@ static int ibmvfc_queuecommand_lck(struct scsi_cmnd *cmnd,
vfc_cmd->iu.pri_task_attr = IBMVFC_SIMPLE_TASK;
}

+ vfc_cmd->correlation = cpu_to_be64(evt);
+
if (likely(!(rc = ibmvfc_map_sg_data(cmnd, evt, vfc_cmd, vhost->dev))))
return ibmvfc_send_event(evt, vhost, 0);

@@ -2370,6 +2372,8 @@ static int ibmvfc_abort_task_set(struct scsi_device *sdev)
tmf->iu.tmf_flags = IBMVFC_ABORT_TASK_SET;
evt->sync_iu = &rsp_iu;

+ tmf->correlation = cpu_to_be64(evt);
+
init_completion(&evt->comp);
rsp_rc = ibmvfc_send_event(evt, vhost, default_timeout);
}
--
2.27.0

2020-11-20 03:13:41

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH 1/3] ibmvfc: byte swap login_buf.resp values in attribute show functions


Tyrel,

> Both ibmvfc_show_host_(capabilities|npiv_version) functions retrieve
> values from vhost->login_buf.resp buffer. This is the MAD response
> buffer from the VIOS and as such any multi-byte non-string values are in
> big endian format.

Applied 1-3 to 5.11/scsi-staging, thanks!

--
Martin K. Petersen Oracle Linux Engineering

2020-11-24 04:01:52

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH 1/3] ibmvfc: byte swap login_buf.resp values in attribute show functions

On Tue, 17 Nov 2020 12:50:29 -0600, Tyrel Datwyler wrote:

> Both ibmvfc_show_host_(capabilities|npiv_version) functions retrieve
> values from vhost->login_buf.resp buffer. This is the MAD response
> buffer from the VIOS and as such any multi-byte non-string values are in
> big endian format.
>
> Byte swap these values to host cpu endian format for better human
> readability.

Applied to 5.11/scsi-queue, thanks!

[1/3] scsi: ibmvfc: Byte swap login_buf.resp values in attribute show functions
https://git.kernel.org/mkp/scsi/c/61bdb4eec8d1
[2/3] scsi: ibmvfc: Remove trailing semicolon
https://git.kernel.org/mkp/scsi/c/4e0716199ab6
[3/3] scsi: ibmvfc: Use correlation token to tag commands
https://git.kernel.org/mkp/scsi/c/2aa0102c6688

--
Martin K. Petersen Oracle Linux Engineering

2020-12-22 06:25:38

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 3/3] ibmvfc: use correlation token to tag commands

On Tue, Nov 17, 2020 at 12:50:31PM -0600, Tyrel Datwyler wrote:
> The vfcFrame correlation field is 64bit handle that is intended to trace
> I/O operations through both the client stack and VIOS stack when the
> underlying physical FC adapter supports tagging.
>
> Tag vfcFrames with the associated ibmvfc_event pointer handle.
>
> Signed-off-by: Tyrel Datwyler <[email protected]>
> ---
> drivers/scsi/ibmvscsi/ibmvfc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 0cab4b852b48..3922441a117d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -1693,6 +1693,8 @@ static int ibmvfc_queuecommand_lck(struct scsi_cmnd *cmnd,
> vfc_cmd->iu.pri_task_attr = IBMVFC_SIMPLE_TASK;
> }
>
> + vfc_cmd->correlation = cpu_to_be64(evt);
> +
> if (likely(!(rc = ibmvfc_map_sg_data(cmnd, evt, vfc_cmd, vhost->dev))))
> return ibmvfc_send_event(evt, vhost, 0);
>
> @@ -2370,6 +2372,8 @@ static int ibmvfc_abort_task_set(struct scsi_device *sdev)
> tmf->iu.tmf_flags = IBMVFC_ABORT_TASK_SET;
> evt->sync_iu = &rsp_iu;
>
> + tmf->correlation = cpu_to_be64(evt);
> +
> init_completion(&evt->comp);
> rsp_rc = ibmvfc_send_event(evt, vhost, default_timeout);
> }
> --
> 2.27.0
>

This patch introduces a clang warning, is this intentional behavior?

$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- LLVM=1 O=out distclean ppc64le_defconfig drivers/scsi/ibmvscsi/ibmvfc.o
Using ../arch/powerpc/configs/ppc64_defconfig as base
Merging ../arch/powerpc/configs/le.config
#
# merged configuration written to .config (needs make)
#
../drivers/scsi/ibmvscsi/ibmvfc.c:1747:25: warning: incompatible pointer to integer conversion passing 'struct ibmvfc_event *' to parameter of type '__u64' (aka 'unsigned long long') [-Wint-conversion]
vfc_cmd->correlation = cpu_to_be64(evt);
^~~~~~~~~~~~~~~~
../include/linux/byteorder/generic.h:92:21: note: expanded from macro 'cpu_to_be64'
#define cpu_to_be64 __cpu_to_be64
^
../include/uapi/linux/byteorder/little_endian.h:37:52: note: expanded from macro '__cpu_to_be64'
#define __cpu_to_be64(x) ((__force __be64)__swab64((x)))
~~~~~~~~~^~~~
../include/uapi/linux/swab.h:133:12: note: expanded from macro '__swab64'
__fswab64(x))
^
../include/uapi/linux/swab.h:66:57: note: passing argument to parameter 'val' here
static inline __attribute_const__ __u64 __fswab64(__u64 val)
^
../drivers/scsi/ibmvscsi/ibmvfc.c:2421:22: warning: incompatible pointer to integer conversion passing 'struct ibmvfc_event *' to parameter of type '__u64' (aka 'unsigned long long') [-Wint-conversion]
tmf->correlation = cpu_to_be64(evt);
^~~~~~~~~~~~~~~~
../include/linux/byteorder/generic.h:92:21: note: expanded from macro 'cpu_to_be64'
#define cpu_to_be64 __cpu_to_be64
^
../include/uapi/linux/byteorder/little_endian.h:37:52: note: expanded from macro '__cpu_to_be64'
#define __cpu_to_be64(x) ((__force __be64)__swab64((x)))
~~~~~~~~~^~~~
../include/uapi/linux/swab.h:133:12: note: expanded from macro '__swab64'
__fswab64(x))
^
../include/uapi/linux/swab.h:66:57: note: passing argument to parameter 'val' here
static inline __attribute_const__ __u64 __fswab64(__u64 val)
^
2 warnings generated.

Cheers,
Nathan

2020-12-22 17:41:20

by Tyrel Datwyler

[permalink] [raw]
Subject: Re: [PATCH 3/3] ibmvfc: use correlation token to tag commands

On 12/21/20 10:24 PM, Nathan Chancellor wrote:
> On Tue, Nov 17, 2020 at 12:50:31PM -0600, Tyrel Datwyler wrote:
>> The vfcFrame correlation field is 64bit handle that is intended to trace
>> I/O operations through both the client stack and VIOS stack when the
>> underlying physical FC adapter supports tagging.
>>
>> Tag vfcFrames with the associated ibmvfc_event pointer handle.
>>
>> Signed-off-by: Tyrel Datwyler <[email protected]>
>> ---
>> drivers/scsi/ibmvscsi/ibmvfc.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index 0cab4b852b48..3922441a117d 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -1693,6 +1693,8 @@ static int ibmvfc_queuecommand_lck(struct scsi_cmnd *cmnd,
>> vfc_cmd->iu.pri_task_attr = IBMVFC_SIMPLE_TASK;
>> }
>>
>> + vfc_cmd->correlation = cpu_to_be64(evt);
>> +
>> if (likely(!(rc = ibmvfc_map_sg_data(cmnd, evt, vfc_cmd, vhost->dev))))
>> return ibmvfc_send_event(evt, vhost, 0);
>>
>> @@ -2370,6 +2372,8 @@ static int ibmvfc_abort_task_set(struct scsi_device *sdev)
>> tmf->iu.tmf_flags = IBMVFC_ABORT_TASK_SET;
>> evt->sync_iu = &rsp_iu;
>>
>> + tmf->correlation = cpu_to_be64(evt);
>> +
>> init_completion(&evt->comp);
>> rsp_rc = ibmvfc_send_event(evt, vhost, default_timeout);
>> }
>> --
>> 2.27.0
>>
>
> This patch introduces a clang warning, is this intentional behavior?

Nope, I just missed the required cast. I've got a fixes patch queued up. I just
haven't sent it yet.

-Tyrel