Re: [PATCH v2] scsi: 3w-9xxx: Fix endianness issues found by sparse

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux