On Fri, Jul 31, 2020 at 12:07 AM Samuel Holland <samuel@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>
> 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.
Arnd
