* [PATCH] scsi: 3w-9xxx: Fix endianness issues found by sparse
@ 2020-07-26 19:14 Samuel Holland
2020-07-27 19:18 ` Arnd Bergmann
0 siblings, 1 reply; 3+ messages in thread
From: Samuel Holland @ 2020-07-26 19:14 UTC (permalink / raw)
To: Adam Radford, James E.J. Bottomley, Martin K. Petersen
Cc: linux-scsi, linux-kernel, Samuel Holland
The main issue observed was at the call to scsi_set_resid, where the
byteswapped parameter would eventually trigger the alignment check at
drivers/scsi/sd.c:2009. At that point, the kernel would continuously
complain about an "Unaligned partial completion", and no further I/O
could occur.
This gets the controller working on big endian powerpc64.
Signed-off-by: Samuel Holland <samuel@sholland.org>
---
drivers/scsi/3w-9xxx.c | 35 +++++++++++++++++------------------
drivers/scsi/3w-9xxx.h | 6 +++++-
2 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index 3337b1e80412..95e25fda1f90 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -303,10 +303,10 @@ static int twa_aen_drain_queue(TW_Device_Extension *tw_dev, int no_check_reset)
/* Initialize sglist */
memset(&sglist, 0, sizeof(TW_SG_Entry));
- sglist[0].length = TW_SECTOR_SIZE;
- sglist[0].address = tw_dev->generic_buffer_phys[request_id];
+ sglist[0].length = cpu_to_le32(TW_SECTOR_SIZE);
+ sglist[0].address = TW_CPU_TO_SGL(tw_dev->generic_buffer_phys[request_id]);
- if (sglist[0].address & TW_ALIGNMENT_9000_SGL) {
+ if (tw_dev->generic_buffer_phys[request_id] & TW_ALIGNMENT_9000_SGL) {
TW_PRINTK(tw_dev->host, TW_DRIVER, 0x1, "Found unaligned address during AEN drain");
goto out;
}
@@ -440,8 +440,8 @@ static int twa_aen_read_queue(TW_Device_Extension *tw_dev, int request_id)
/* Initialize sglist */
memset(&sglist, 0, sizeof(TW_SG_Entry));
- sglist[0].length = TW_SECTOR_SIZE;
- sglist[0].address = tw_dev->generic_buffer_phys[request_id];
+ sglist[0].length = cpu_to_le32(TW_SECTOR_SIZE);
+ sglist[0].address = TW_CPU_TO_SGL(tw_dev->generic_buffer_phys[request_id]);
/* Mark internal command */
tw_dev->srb[request_id] = NULL;
@@ -501,7 +501,7 @@ static void twa_aen_sync_time(TW_Device_Extension *tw_dev, int request_id)
Sunday 12:00AM */
local_time = (ktime_get_real_seconds() - (sys_tz.tz_minuteswest * 60));
div_u64_rem(local_time - (3 * 86400), 604800, &schedulertime);
- schedulertime = cpu_to_le32(schedulertime % 604800);
+ cpu_to_le32p(&schedulertime);
memcpy(param->data, &schedulertime, sizeof(u32));
@@ -1004,7 +1004,7 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
full_command_packet->header.status_block.error,
error_str[0] == '\0' ?
twa_string_lookup(twa_error_table,
- full_command_packet->header.status_block.error) : error_str,
+ le16_to_cpu(full_command_packet->header.status_block.error)) : error_str,
full_command_packet->header.err_specific_desc);
else
printk(KERN_WARNING "3w-9xxx: ERROR: (0x%02X:0x%04X): %s:%s.\n",
@@ -1012,7 +1012,7 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
full_command_packet->header.status_block.error,
error_str[0] == '\0' ?
twa_string_lookup(twa_error_table,
- full_command_packet->header.status_block.error) : error_str,
+ le16_to_cpu(full_command_packet->header.status_block.error)) : error_str,
full_command_packet->header.err_specific_desc);
}
@@ -1129,12 +1129,11 @@ static int twa_initconnection(TW_Device_Extension *tw_dev, int message_credits,
tw_initconnect->opcode__reserved = TW_OPRES_IN(0, TW_OP_INIT_CONNECTION);
tw_initconnect->request_id = request_id;
tw_initconnect->message_credits = cpu_to_le16(message_credits);
- tw_initconnect->features = set_features;
/* Turn on 64-bit sgl support if we need to */
- tw_initconnect->features |= sizeof(dma_addr_t) > 4 ? 1 : 0;
+ set_features |= sizeof(dma_addr_t) > 4 ? 1 : 0;
- tw_initconnect->features = cpu_to_le32(tw_initconnect->features);
+ tw_initconnect->features = cpu_to_le32(set_features);
if (set_features & TW_EXTENDED_INIT_CONNECT) {
tw_initconnect->size = TW_INIT_COMMAND_PACKET_SIZE_EXTENDED;
@@ -1347,8 +1346,8 @@ static irqreturn_t twa_interrupt(int irq, void *dev_instance)
/* Report residual bytes for single sgl */
if ((scsi_sg_count(cmd) <= 1) && (full_command_packet->command.newcommand.status == 0)) {
- if (full_command_packet->command.newcommand.sg_list[0].length < scsi_bufflen(tw_dev->srb[request_id]))
- scsi_set_resid(cmd, scsi_bufflen(cmd) - full_command_packet->command.newcommand.sg_list[0].length);
+ if (le32_to_cpu(full_command_packet->command.newcommand.sg_list[0].length) < scsi_bufflen(tw_dev->srb[request_id]))
+ scsi_set_resid(cmd, scsi_bufflen(cmd) - le32_to_cpu(full_command_packet->command.newcommand.sg_list[0].length));
}
/* Now complete the io */
@@ -1390,13 +1389,13 @@ static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
if (TW_OP_OUT(full_command_packet->command.newcommand.opcode__reserved) == TW_OP_EXECUTE_SCSI) {
newcommand = &full_command_packet->command.newcommand;
newcommand->request_id__lunl =
- cpu_to_le16(TW_REQ_LUN_IN(TW_LUN_OUT(newcommand->request_id__lunl), request_id));
+ cpu_to_le16(TW_REQ_LUN_IN(TW_LUN_OUT(le16_to_cpu(newcommand->request_id__lunl)), request_id));
if (length) {
newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache) - 1);
newcommand->sg_list[0].length = cpu_to_le32(length);
}
newcommand->sgl_entries__lunh =
- cpu_to_le16(TW_REQ_LUN_IN(TW_LUN_OUT(newcommand->sgl_entries__lunh), length ? 1 : 0));
+ cpu_to_le16(TW_REQ_LUN_IN(TW_LUN_OUT(le16_to_cpu(newcommand->sgl_entries__lunh)), length ? 1 : 0));
} else {
oldcommand = &full_command_packet->command.oldcommand;
oldcommand->request_id = request_id;
@@ -1877,8 +1876,8 @@ static int twa_scsiop_execute_scsi(TW_Device_Extension *tw_dev, int request_id,
} else {
/* Internal cdb post */
for (i = 0; i < use_sg; i++) {
- command_packet->sg_list[i].address = TW_CPU_TO_SGL(sglistarg[i].address);
- command_packet->sg_list[i].length = cpu_to_le32(sglistarg[i].length);
+ command_packet->sg_list[i].address = sglistarg[i].address;
+ command_packet->sg_list[i].length = sglistarg[i].length;
if (command_packet->sg_list[i].address & TW_CPU_TO_SGL(TW_ALIGNMENT_9000_SGL)) {
TW_PRINTK(tw_dev->host, TW_DRIVER, 0x2f, "Found unaligned sgl address during internal post");
goto out;
@@ -2109,7 +2108,7 @@ static int twa_probe(struct pci_dev *pdev, const struct pci_device_id *dev_id)
TW_PARAM_FWVER, TW_PARAM_FWVER_LENGTH),
(char *)twa_get_param(tw_dev, 1, TW_VERSION_TABLE,
TW_PARAM_BIOSVER, TW_PARAM_BIOSVER_LENGTH),
- le32_to_cpu(*(int *)twa_get_param(tw_dev, 2, TW_INFORMATION_TABLE,
+ le32_to_cpu(*(__le32 *)twa_get_param(tw_dev, 2, TW_INFORMATION_TABLE,
TW_PARAM_PORTCOUNT, TW_PARAM_PORTCOUNT_LENGTH)));
/* Try to enable MSI */
diff --git a/drivers/scsi/3w-9xxx.h b/drivers/scsi/3w-9xxx.h
index d88cd3499bd5..1e0430eb3b40 100644
--- a/drivers/scsi/3w-9xxx.h
+++ b/drivers/scsi/3w-9xxx.h
@@ -469,7 +469,11 @@ printk(KERN_WARNING "3w-9xxx: ERROR: (0x%02X:0x%04X): %s.\n",a,b,c); \
#define TW_APACHE_MAX_SGL_LENGTH (sizeof(dma_addr_t) > 4 ? 72 : 109)
#define TW_ESCALADE_MAX_SGL_LENGTH (sizeof(dma_addr_t) > 4 ? 41 : 62)
#define TW_PADDING_LENGTH (sizeof(dma_addr_t) > 4 ? 8 : 0)
-#define TW_CPU_TO_SGL(x) (sizeof(dma_addr_t) > 4 ? cpu_to_le64(x) : cpu_to_le32(x))
+#if IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT)
+#define TW_CPU_TO_SGL(x) cpu_to_le64(x)
+#else
+#define TW_CPU_TO_SGL(x) cpu_to_le32(x)
+#endif
#pragma pack(1)
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] scsi: 3w-9xxx: Fix endianness issues found by sparse
2020-07-26 19:14 [PATCH] scsi: 3w-9xxx: Fix endianness issues found by sparse Samuel Holland
@ 2020-07-27 19:18 ` Arnd Bergmann
2020-07-29 4:50 ` Samuel Holland
0 siblings, 1 reply; 3+ messages in thread
From: Arnd Bergmann @ 2020-07-27 19:18 UTC (permalink / raw)
To: Samuel Holland
Cc: Adam Radford, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, linux-kernel
On Sun, Jul 26, 2020 at 9:15 PM Samuel Holland <samuel@sholland.org> wrote:
>
> The main issue observed was at the call to scsi_set_resid, where the
> byteswapped parameter would eventually trigger the alignment check at
> drivers/scsi/sd.c:2009. At that point, the kernel would continuously
> complain about an "Unaligned partial completion", and no further I/O
> could occur.
>
> This gets the controller working on big endian powerpc64.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> drivers/scsi/3w-9xxx.c | 35 +++++++++++++++++------------------
> drivers/scsi/3w-9xxx.h | 6 +++++-
> 2 files changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 3337b1e80412..95e25fda1f90 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -303,10 +303,10 @@ static int twa_aen_drain_queue(TW_Device_Extension *tw_dev, int no_check_reset)
>
> /* Initialize sglist */
> memset(&sglist, 0, sizeof(TW_SG_Entry));
> - sglist[0].length = TW_SECTOR_SIZE;
> - sglist[0].address = tw_dev->generic_buffer_phys[request_id];
> + sglist[0].length = cpu_to_le32(TW_SECTOR_SIZE);
> + sglist[0].address = TW_CPU_TO_SGL(tw_dev->generic_buffer_phys[request_id]);
This looks like it would add a sparse warning, not fix one, unless you also
change the types of the target structure.
> @@ -501,7 +501,7 @@ static void twa_aen_sync_time(TW_Device_Extension *tw_dev, int request_id)
> Sunday 12:00AM */
> local_time = (ktime_get_real_seconds() - (sys_tz.tz_minuteswest * 60));
> div_u64_rem(local_time - (3 * 86400), 604800, &schedulertime);
> - schedulertime = cpu_to_le32(schedulertime % 604800);
> + cpu_to_le32p(&schedulertime);
>
> memcpy(param->data, &schedulertime, sizeof(u32));
You dropped the '%' operation, and the result of the byteswap?
> @@ -1004,7 +1004,7 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
> full_command_packet->header.status_block.error,
> error_str[0] == '\0' ?
> twa_string_lookup(twa_error_table,
> - full_command_packet->header.status_block.error) : error_str,
> + le16_to_cpu(full_command_packet->header.status_block.error)) : error_str,
> full_command_packet->header.err_specific_desc);
> else
This looks correct, but the error value has already been copied into the local
'error' variable, which you could use for simplification. As 'status_block' is
defined as a native_endian structure, this also introduced a sparse warning.
> @@ -1012,7 +1012,7 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
> full_command_packet->header.status_block.error,
> error_str[0] == '\0' ?
> twa_string_lookup(twa_error_table,
> - full_command_packet->header.status_block.error) : error_str,
> + le16_to_cpu(full_command_packet->header.status_block.error)) : error_str,
Same here
Arnd
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] scsi: 3w-9xxx: Fix endianness issues found by sparse
2020-07-27 19:18 ` Arnd Bergmann
@ 2020-07-29 4:50 ` Samuel Holland
0 siblings, 0 replies; 3+ messages in thread
From: Samuel Holland @ 2020-07-29 4:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Adam Radford, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, linux-kernel
On 7/27/20 2:18 PM, Arnd Bergmann wrote:
> On Sun, Jul 26, 2020 at 9:15 PM Samuel Holland <samuel@sholland.org> wrote:
>>
>> The main issue observed was at the call to scsi_set_resid, where the
>> byteswapped parameter would eventually trigger the alignment check at
>> drivers/scsi/sd.c:2009. At that point, the kernel would continuously
>> complain about an "Unaligned partial completion", and no further I/O
>> could occur.
>>
>> This gets the controller working on big endian powerpc64.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>> drivers/scsi/3w-9xxx.c | 35 +++++++++++++++++------------------
>> drivers/scsi/3w-9xxx.h | 6 +++++-
>> 2 files changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
>> index 3337b1e80412..95e25fda1f90 100644
>> --- a/drivers/scsi/3w-9xxx.c
>> +++ b/drivers/scsi/3w-9xxx.c
>> @@ -303,10 +303,10 @@ static int twa_aen_drain_queue(TW_Device_Extension *tw_dev, int no_check_reset)
>>
>> /* Initialize sglist */
>> memset(&sglist, 0, sizeof(TW_SG_Entry));
>> - sglist[0].length = TW_SECTOR_SIZE;
>> - sglist[0].address = tw_dev->generic_buffer_phys[request_id];
>> + sglist[0].length = cpu_to_le32(TW_SECTOR_SIZE);
>> + sglist[0].address = TW_CPU_TO_SGL(tw_dev->generic_buffer_phys[request_id]);
>
> This looks like it would add a sparse warning, not fix one, unless you also
> change the types of the target structure.
Yes, I meant to change the structure types as well. All of the command
structures sent to the card are little-endian. I pulled this bugfix patch out of
a series of unrelated changes, and I missed the header changes. I'll send a v2.
>> @@ -501,7 +501,7 @@ static void twa_aen_sync_time(TW_Device_Extension *tw_dev, int request_id)
>> Sunday 12:00AM */
>> local_time = (ktime_get_real_seconds() - (sys_tz.tz_minuteswest * 60));
>> div_u64_rem(local_time - (3 * 86400), 604800, &schedulertime);
>> - schedulertime = cpu_to_le32(schedulertime % 604800);
>> + cpu_to_le32p(&schedulertime);
>>
>> memcpy(param->data, &schedulertime, sizeof(u32));
>
> You dropped the '%' operation, and the result of the byteswap?
schedulertime is the remainder from the previous line, so it is <604800 already.
You're right about that being the wrong function -- I meant to use cpu_to_le32s
to swap it in place, to avoid needing a second variable.
>> @@ -1004,7 +1004,7 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
>> full_command_packet->header.status_block.error,
>> error_str[0] == '\0' ?
>> twa_string_lookup(twa_error_table,
>> - full_command_packet->header.status_block.error) : error_str,
>> + le16_to_cpu(full_command_packet->header.status_block.error)) : error_str,
>> full_command_packet->header.err_specific_desc);
>> else
>
> This looks correct, but the error value has already been copied into the local
> 'error' variable, which you could use for simplification. As 'status_block' is
> defined as a native_endian structure, this also introduced a sparse warning.
I'll use 'error' in v2, thanks for the hint.
>> @@ -1012,7 +1012,7 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
>> full_command_packet->header.status_block.error,
>> error_str[0] == '\0' ?
>> twa_string_lookup(twa_error_table,
>> - full_command_packet->header.status_block.error) : error_str,
>> + le16_to_cpu(full_command_packet->header.status_block.error)) : error_str,
>
> Same here
>
> Arnd
>
Cheers,
Samuel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-29 4:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-26 19:14 [PATCH] scsi: 3w-9xxx: Fix endianness issues found by sparse Samuel Holland
2020-07-27 19:18 ` Arnd Bergmann
2020-07-29 4:50 ` Samuel Holland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).