From: Arnd Bergmann <arnd@arndb.de>
To: Samuel Holland <samuel@sholland.org>
Cc: Adam Radford <aradford@gmail.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
linux-scsi <linux-scsi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] scsi: 3w-9xxx: Fix endianness issues found by sparse
Date: Mon, 3 Aug 2020 16:02:44 +0200 [thread overview]
Message-ID: <CAK8P3a0xSyyaLHziuv4JKimUggF96frwLPKmjQ4G9VBWRW2EMg@mail.gmail.com> (raw)
In-Reply-To: <29ea8d0f-bcab-9ffd-0e2f-f022911f4bf2@sholland.org>
On Mon, Aug 3, 2020 at 5:42 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 7/31/20 2:29 AM, Arnd Bergmann wrote:
> > On Fri, Jul 31, 2020 at 12:07 AM 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>
> >> ---
> >>
> >> Changes since v1:
> >> - Include changes to use __le?? types in command structures
> >> - Use an object literal for the intermediate "schedulertime" value
> >> - Use local "error" variable to avoid repeated byte swapping
> >> - Create a local "length" variable to avoid very long lines
> >> - Move byte swapping to TW_REQ_LUN_IN/TW_LUN_OUT to avoid long lines
> >>
> >
> > Looks much better, thanks for the update. I see one more issue here
> >> /* Command Packet */
> >> typedef struct TW_Command {
> >> - unsigned char opcode__sgloffset;
> >> - unsigned char size;
> >> - unsigned char request_id;
> >> - unsigned char unit__hostid;
> >> + u8 opcode__sgloffset;
> >> + u8 size;
> >> + u8 request_id;
> >> + u8 unit__hostid;
> >> /* Second DWORD */
> >> - unsigned char status;
> >> - unsigned char flags;
> >> + u8 status;
> >> + u8 flags;
> >> union {
> >> - unsigned short block_count;
> >> - unsigned short parameter_count;
> >> + __le16 block_count;
> >> + __le16 parameter_count;
> >> } byte6_offset;
> >> union {
> >> struct {
> >> - u32 lba;
> >> - TW_SG_Entry sgl[TW_ESCALADE_MAX_SGL_LENGTH];
> >> - dma_addr_t padding;
> >> + __le32 lba;
> >> + TW_SG_Entry sgl[TW_ESCALADE_MAX_SGL_LENGTH];
> >> + dma_addr_t padding;
> >
> >
> > The use of dma_addr_t here seems odd, since this is neither endian-safe nor
> > fixed-length. I see you replaced the dma_addr_t in TW_SG_Entry with
> > a variable-length fixed-endian word. I guess there is a chance that this is
> > correct, but it is really confusing. On top of that, it seems that there is
> > implied padding in the structure when built with a 64-bit dma_addr_t
> > on most architectures but not on x86-32 (which uses 32-bit alignment for
> > 64-bit integers). I don't know what the hardware definition is for TW_Command,
> > but ideally this would be expressed using only fixed-endian fixed-length
> > members and explicit padding.
>
> All of the command structures are packed, due to the "#pragma pack(1)" earlier
> in the file. So alignment is not an issue. This dma_addr_t member _is_ the
> explicit padding to make sizeof(TW_Command) -
> sizeof(TW_Command.byte8_offset.{io,param}.sgl) equal TW_COMMAND_SIZE * 4. And
> indeed the structure is expected to be a different size depending on
> sizeof(dma_addr_t).
Ah, so only the first few members are accessed by hardware and the
last union is only accessed by the OS then? In that case I suppose it is
all fine, but I would also suggest removing the "#pragma packed"
to get somewhat more efficient access on systems that have problems
with misaligned accesses.
Arnd
next prev parent reply other threads:[~2020-08-03 14:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-30 22:07 [PATCH v2] scsi: 3w-9xxx: Fix endianness issues found by sparse Samuel Holland
2020-07-31 7:29 ` Arnd Bergmann
2020-08-03 3:43 ` Samuel Holland
2020-08-03 14:02 ` Arnd Bergmann [this message]
2020-08-05 1:45 ` Samuel Holland
2020-08-05 7:17 ` Arnd Bergmann
2020-08-09 0:16 ` Samuel Holland
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=CAK8P3a0xSyyaLHziuv4JKimUggF96frwLPKmjQ4G9VBWRW2EMg@mail.gmail.com \
--to=arnd@arndb.de \
--cc=aradford@gmail.com \
--cc=jejb@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=samuel@sholland.org \
/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).