From: Finn Thain <fthain@telegraphics.com.au>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Daniel Wagner <dwagner@suse.de>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
"James E. J. Bottomley" <jejb@linux.vnet.ibm.com>,
linux-scsi@vger.kernel.org, Hannes Reinecke <hare@suse.de>,
Arun Easi <aeasi@marvell.com>,
Himanshu Madhani <himanshu.madhani@oracle.com>,
Nilesh Javali <njavali@marvell.com>,
Quinn Tran <qutran@marvell.com>, Martin Wilck <mwilck@suse.com>,
Roman Bolshakov <r.bolshakov@yadro.com>
Subject: Re: [PATCH v7 15/15] qla2xxx: Fix endianness annotations in source files
Date: Mon, 25 May 2020 09:45:47 +1000 (AEST) [thread overview]
Message-ID: <alpine.LNX.2.22.394.2005250920550.8@nippy.intranet> (raw)
In-Reply-To: <2737709e-1423-da29-4697-dc517ad7e413@acm.org>
On Sun, 24 May 2020, Bart Van Assche wrote:
> On 2020-05-23 21:28, Finn Thain wrote:
> > 2. The get_unaligned_le32() changes produce new pointer offsets in the
> > assembly code for qla82xx_get_table_desc() and qla82xx_get_data_desc().
> >
> > diff -ru /tmp/unpatched/qla_target.s /tmp/patched/qla_target.s
> > --- /tmp/unpatched/qla_target.s 2020-05-24 14:02:32.178019380 +1000
> > +++ /tmp/patched/qla_target.s 2020-05-24 14:01:43.487947966 +1000
> > @@ -12884,10 +12884,10 @@
> > .cfi_offset 6, -16
> > movq %rsp, %rbp
> > .cfi_def_cfa_register 6
> > - subq $32, %rsp
> > - movq %rdi, -24(%rbp)
> > - movq %rsi, -32(%rbp)
> > - movq -32(%rbp), %rax
> > + subq $64, %rsp
> > + movq %rdi, -56(%rbp)
> > + movq %rsi, -64(%rbp)
> > + movq -64(%rbp), %rax
> > movl 52(%rax), %eax
> > movl %eax, -8(%rbp)
> > movl $24, -12(%rbp)
> > @@ -12895,62 +12895,62 @@
> > cmpl %eax, -12(%rbp)
> > cmovbe -12(%rbp), %eax
> > movl %eax, %edx
> > - movq -32(%rbp), %rax
> > + movq -64(%rbp), %rax
> > movl %edx, 52(%rax)
> > - movq -24(%rbp), %rax
> > + movq -56(%rbp), %rax
> > (and so on.)
> >
> > Was this expected? I find it surprising...
>
> The functions qla82xx_get_table_desc() and qla82xx_get_data_desc() exist
> in qla_nx.c. Does the above diff perhaps refer to qla_nx.s instead of
> qla_target.s?
>
A similar effect can be seen in both qla_nx.s and qla_target.s.
> To me the change of "subq $32, %rsp" into "subq $64, %rsp" means that
> the compiler reserved more space on the stack for local variables.
>
> If I compare the assembler output for qla_nx.c then I see that
> qla82xx_get_table_desc() gets inlined with my patch applied but not
> without my patch applied. This is something that I had not expected but
> that explains the above diff IMHO.
>
Thanks for checking.
For completeness, here's the diff that produced the code generation
changes in qla_nx.s and qla_target.s. These changes aren't the same as
those in commit 7ffa5b939751 because these were intended to avoid
perturbing line numbering etc.
diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index ac54d2d1b02b..cff570b9c919 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -1582,7 +1582,7 @@ qla82xx_get_data_desc(struct qla_hw_data *ha,
u32 section, u32 idx_offset)
{
const u8 *unirom = ha->hablob->fw->data;
- int idx = cpu_to_le32(*((u32 *)&unirom[ha->file_prd_off] + idx_offset));
+ int idx = get_unaligned_le32((int *)&unirom[ha->file_prd_off] + idx_offset);
struct qla82xx_uri_table_desc *tab_desc = NULL;
uint32_t offset;
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index cb15654bab33..856140564408 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2841,8 +2841,8 @@ static void qlt_24xx_init_ctio_to_isp(struct ctio7_to_24xx *ctio,
ctio->u.status1.sense_length =
cpu_to_le16(prm->sense_buffer_len);
for (i = 0; i < prm->sense_buffer_len/4; i++)
- ((uint32_t *)ctio->u.status1.sense_data)[i] =
- cpu_to_be32(((uint32_t *)prm->sense_buffer)[i]);
+ put_unaligned_le32(get_unaligned_be32(&((uint32_t *)prm->sense_buffer)[i]),
+ &((uint32_t *)ctio->u.status1.sense_data)[i]);
qlt_print_dif_err(prm);
next prev parent reply other threads:[~2020-05-24 23:46 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-18 21:16 [PATCH v7 00/15] Fix qla2xxx endianness annotations Bart Van Assche
2020-05-18 21:16 ` [PATCH v7 01/15] qla2xxx: Fix spelling of a variable name Bart Van Assche
2020-05-18 21:16 ` [PATCH v7 02/15] qla2xxx: Suppress two recently introduced compiler warnings Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 03/15] qla2xxx: Simplify the functions for dumping firmware Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 04/15] qla2xxx: Sort BUILD_BUG_ON() statements alphabetically Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 05/15] qla2xxx: Add more BUILD_BUG_ON() statements Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 06/15] qla2xxx: Make a gap in struct qla2xxx_offld_chain explicit Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 07/15] qla2xxx: Increase the size of struct qla_fcp_prio_cfg to FCP_PRIO_CFG_SIZE Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 08/15] qla2xxx: Change two hardcoded constants into offsetof() / sizeof() expressions Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 09/15] qla2xxx: Use register names instead of register offsets Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 10/15] qla2xxx: Fix the code that reads from mailbox registers Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 11/15] qla2xxx: Change {RD,WRT}_REG_*() function names from upper case into lower case Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 12/15] qla2xxx: Cast explicitly to uint16_t / uint32_t Bart Van Assche
2020-05-19 15:29 ` Daniel Wagner
2020-05-18 21:17 ` [PATCH v7 13/15] qla2xxx: Use make_handle() instead of open-coding it Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 14/15] qla2xxx: Fix endianness annotations in header files Bart Van Assche
2020-05-18 21:17 ` [PATCH v7 15/15] qla2xxx: Fix endianness annotations in source files Bart Van Assche
2020-05-19 15:24 ` Daniel Wagner
2020-05-20 7:39 ` Finn Thain
2020-05-20 8:56 ` Daniel Wagner
2020-05-24 4:28 ` Finn Thain
2020-05-24 15:50 ` Bart Van Assche
2020-05-24 23:45 ` Finn Thain [this message]
2020-05-20 2:30 ` [PATCH v7 00/15] Fix qla2xxx endianness annotations Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LNX.2.22.394.2005250920550.8@nippy.intranet \
--to=fthain@telegraphics.com.au \
--cc=aeasi@marvell.com \
--cc=bvanassche@acm.org \
--cc=dwagner@suse.de \
--cc=hare@suse.de \
--cc=himanshu.madhani@oracle.com \
--cc=jejb@linux.vnet.ibm.com \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=mwilck@suse.com \
--cc=njavali@marvell.com \
--cc=qutran@marvell.com \
--cc=r.bolshakov@yadro.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).