Re: [qemu-s390x] [PATCH v3 06/16] s390-bios: Clean up cio.h

[Thomas Huth]

On 01/03/2019 19.59, Jason J. Herne wrote:
> Add proper typedefs to all structs and modify all bit fields to use consistent
> formatting.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> Reviewed-by: Collin Walling <address@hidden>
> Reviewed-by: Farhan Ali <address@hidden>
> ---
>  pc-bios/s390-ccw/cio.h      | 152 
> ++++++++++++++++++++++----------------------
>  pc-bios/s390-ccw/s390-ccw.h |   8 ---
>  2 files changed, 76 insertions(+), 84 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index 1a0795f..2f58256 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
[...]
> -struct subchannel_id {
> -        __u32 cssid  : 8;
> -        __u32        : 4;
> -        __u32 m      : 1;
> -        __u32 ssid   : 2;
> -        __u32 one    : 1;
> -        __u32 sch_no : 16;
> -} __attribute__ ((packed, aligned(4)));
> +} __attribute__ ((packed, aligned(4))) Schib;
> +
> +typedef struct subchannel_id {
> +        __u32 cssid:8;
> +        __u32:4;

__u32:4 looks a little bit funny. Not sure, but maybe this should be
given a name like "reserved" or so?

> +        __u32 m:1;
> +        __u32 ssid:2;
> +        __u32 one:1;
> +        __u32 sch_no:16;
> +} __attribute__ ((packed, aligned(4))) SubChannelId;
>  
>  struct chsc_header {
>      __u16 length;
>      __u16 code;
>  } __attribute__((packed));
[...]
> @@ -162,27 +162,27 @@ struct ccw1 {
>  /*
>   * Command-mode operation request block
>   */
> -struct cmd_orb {
> -    __u32 intparm;    /* interruption parameter */
> -    __u32 key:4;      /* flags, like key, suspend control, etc. */
> -    __u32 spnd:1;     /* suspend control */
> -    __u32 res1:1;     /* reserved */
> -    __u32 mod:1;      /* modification control */
> -    __u32 sync:1;     /* synchronize control */
> -    __u32 fmt:1;      /* format control */
> -    __u32 pfch:1;     /* prefetch control */
> -    __u32 isic:1;     /* initial-status-interruption control */
> -    __u32 alcc:1;     /* address-limit-checking control */
> -    __u32 ssic:1;     /* suppress-suspended-interr. control */
> -    __u32 res2:1;     /* reserved */
> -    __u32 c64:1;      /* IDAW/QDIO 64 bit control  */
> -    __u32 i2k:1;      /* IDAW 2/4kB block size control */
> -    __u32 lpm:8;      /* logical path mask */
> -    __u32 ils:1;      /* incorrect length */
> -    __u32 zero:6;     /* reserved zeros */
> -    __u32 orbx:1;     /* ORB extension control */
> -    __u32 cpa;    /* channel program address */
> -}  __attribute__ ((packed, aligned(4)));
> +typedef struct cmd_orb {
> +    __u32 intparm;  /* interruption parameter */
> +    __u32 key:4;    /* flags, like key, suspend control, etc. */
> +    __u32 spnd:1;   /* suspend control */
> +    __u32 res1:1;   /* reserved */
> +    __u32 mod:1;    /* modification control */
> +    __u32 sync:1;   /* synchronize control */
> +    __u32 fmt:1;    /* format control */
> +    __u32 pfch:1;   /* prefetch control */
> +    __u32 isic:1;   /* initial-status-interruption control */
> +    __u32 alcc:1;   /* address-limit-checking control */
> +    __u32 ssic:1;   /* suppress-suspended-interr. control */
> +    __u32 res2:1;   /* reserved */
> +    __u32 c64:1;    /* IDAW/QDIO 64 bit control  */
> +    __u32 i2k:1;    /* IDAW 2/4kB block size control */
> +    __u32 lpm:8;    /* logical path mask */
> +    __u32 ils:1;    /* incorrect length */
> +    __u32 zero:6;   /* reserved zeros */
> +    __u32 orbx:1;   /* ORB extension control */
> +    __u32 cpa;      /* channel program address */
> +}  __attribute__ ((packed, aligned(4))) CmdOrb;

The white space changes to this struct look like unnecessary code churn
to me ... I'd like to suggest to keep the comments at the old
indentation level.

With that fixed:

Reviewed-by: Thomas Huth <address@hidden>