diff mbox series

[v3,06/16] s390-bios: Clean up cio.h

Message ID 1551466776-29123-7-git-send-email-jjherne@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390: vfio-ccw dasd ipl support | expand

Commit Message

Jason J. Herne March 1, 2019, 6:59 p.m. UTC
Add proper typedefs to all structs and modify all bit fields to use consistent
formatting.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Collin Walling <walling@linux.ibm.com>
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
---
 pc-bios/s390-ccw/cio.h      | 152 ++++++++++++++++++++++----------------------
 pc-bios/s390-ccw/s390-ccw.h |   8 ---
 2 files changed, 76 insertions(+), 84 deletions(-)

Comments

Cornelia Huck March 4, 2019, 5:23 p.m. UTC | #1
On Fri,  1 Mar 2019 13:59:26 -0500
"Jason J. Herne" <jjherne@linux.ibm.com> wrote:

> Add proper typedefs to all structs and modify all bit fields to use consistent
> formatting.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/cio.h      | 152 ++++++++++++++++++++++----------------------
>  pc-bios/s390-ccw/s390-ccw.h |   8 ---
>  2 files changed, 76 insertions(+), 84 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>
Thomas Huth March 5, 2019, 5:51 a.m. UTC | #2
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 <jjherne@linux.ibm.com>
> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  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 <thuth@redhat.com>
Jason J. Herne March 6, 2019, 6:42 p.m. UTC | #3
On 3/5/19 12:51 AM, Thomas Huth wrote:
> 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 <jjherne@linux.ibm.com>
>> Reviewed-by: Collin Walling <walling@linux.ibm.com>
>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   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?
> 

Perhaps, but this is not my code. I was just cleaning up the style. If you feel strongly 
that this patch is the place to give this a name, I can do that.
Cornelia Huck March 7, 2019, 8:08 a.m. UTC | #4
On Wed, 6 Mar 2019 13:42:17 -0500
"Jason J. Herne" <jjherne@linux.ibm.com> wrote:

> On 3/5/19 12:51 AM, Thomas Huth wrote:
> > 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 <jjherne@linux.ibm.com>
> >> Reviewed-by: Collin Walling <walling@linux.ibm.com>
> >> Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
> >> ---
> >>   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?
> >   
> 
> Perhaps, but this is not my code. I was just cleaning up the style. If you feel strongly 
> that this patch is the place to give this a name, I can do that.

FWIW, my personal preference for this is keeping it unnamed.
diff mbox series

Patch

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
@@ -17,35 +17,35 @@ 
  * path management control word
  */
 struct pmcw {
-    __u32 intparm;        /* interruption parameter */
-    __u32 qf      : 1;    /* qdio facility */
-    __u32 w       : 1;
-    __u32 isc     : 3;    /* interruption sublass */
-    __u32 res5    : 3;    /* reserved zeros */
-    __u32 ena     : 1;    /* enabled */
-    __u32 lm      : 2;    /* limit mode */
-    __u32 mme     : 2;    /* measurement-mode enable */
-    __u32 mp      : 1;    /* multipath mode */
-    __u32 tf      : 1;    /* timing facility */
-    __u32 dnv     : 1;    /* device number valid */
-    __u32 dev     : 16;   /* device number */
-    __u8  lpm;            /* logical path mask */
-    __u8  pnom;           /* path not operational mask */
-    __u8  lpum;           /* last path used mask */
-    __u8  pim;            /* path installed mask */
-    __u16 mbi;            /* measurement-block index */
-    __u8  pom;            /* path operational mask */
-    __u8  pam;            /* path available mask */
-    __u8  chpid[8];       /* CHPID 0-7 (if available) */
-    __u32 unused1 : 8;    /* reserved zeros */
-    __u32 st      : 3;    /* subchannel type */
-    __u32 unused2 : 18;   /* reserved zeros */
-    __u32 mbfc    : 1;    /* measurement block format control */
-    __u32 xmwme   : 1;    /* extended measurement word mode enable */
-    __u32 csense  : 1;    /* concurrent sense; can be enabled ...*/
-                /*  ... per MSCH, however, if facility */
-                /*  ... is not installed, this results */
-                /*  ... in an operand exception.       */
+    __u32 intparm;      /* interruption parameter */
+    __u32 qf:1;         /* qdio facility */
+    __u32 w:1;
+    __u32 isc:3;        /* interruption sublass */
+    __u32 res5:3;       /* reserved zeros */
+    __u32 ena:1;        /* enabled */
+    __u32 lm:2;         /* limit mode */
+    __u32 mme:2;        /* measurement-mode enable */
+    __u32 mp:1;         /* multipath mode */
+    __u32 tf:1;         /* timing facility */
+    __u32 dnv:1;        /* device number valid */
+    __u32 dev:16;       /* device number */
+    __u8  lpm;          /* logical path mask */
+    __u8  pnom;         /* path not operational mask */
+    __u8  lpum;         /* last path used mask */
+    __u8  pim;          /* path installed mask */
+    __u16 mbi;          /* measurement-block index */
+    __u8  pom;          /* path operational mask */
+    __u8  pam;          /* path available mask */
+    __u8  chpid[8];     /* CHPID 0-7 (if available) */
+    __u32 unused1:8;    /* reserved zeros */
+    __u32 st:3;         /* subchannel type */
+    __u32 unused2:18;   /* reserved zeros */
+    __u32 mbfc:1;       /* measurement block format control */
+    __u32 xmwme:1;      /* extended measurement word mode enable */
+    __u32 csense:1;     /* concurrent sense; can be enabled ...*/
+                        /*  ... per MSCH, however, if facility */
+                        /*  ... is not installed, this results */
+                        /*  ... in an operand exception.       */
 } __attribute__ ((packed));
 
 /* Target SCHIB configuration. */
@@ -77,28 +77,28 @@  struct scsw {
 /*
  * subchannel information block
  */
-struct schib {
+typedef struct schib {
     struct pmcw pmcw;     /* path management control word */
     struct scsw scsw;     /* subchannel status word */
     __u64 mba;            /* measurement block address */
     __u8 mda[4];          /* model dependent area */
-} __attribute__ ((packed,aligned(4)));
-
-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 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));
 
-struct chsc_area_sda {
+typedef struct chsc_area_sda {
     struct chsc_header request;
     __u8 reserved1:4;
     __u8 format:4;
@@ -111,29 +111,29 @@  struct chsc_area_sda {
     __u32 reserved5:4;
     __u32 format2:4;
     __u32 reserved6:24;
-} __attribute__((packed));
+} __attribute__((packed)) ChscAreaSda;
 
 /*
  * TPI info structure
  */
 struct tpi_info {
     struct subchannel_id schid;
-    __u32 intparm;         /* interruption parameter */
-    __u32 adapter_IO : 1;
-    __u32 reserved2  : 1;
-    __u32 isc        : 3;
-    __u32 reserved3  : 12;
-    __u32 int_type   : 3;
-    __u32 reserved4  : 12;
+    __u32 intparm;      /* interruption parameter */
+    __u32 adapter_IO:1;
+    __u32 reserved2:1;
+    __u32 isc:3;
+    __u32 reserved3:12;
+    __u32 int_type:3;
+    __u32 reserved4:12;
 } __attribute__ ((packed, aligned(4)));
 
 /* channel command word (type 1) */
-struct ccw1 {
+typedef struct ccw1 {
     __u8 cmd_code;
     __u8 flags;
     __u16 count;
     __u32 cda;
-} __attribute__ ((packed, aligned(8)));
+} __attribute__ ((packed, aligned(8))) Ccw1;
 
 #define CCW_FLAG_DC              0x80
 #define CCW_FLAG_CC              0x40
@@ -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;
 
 struct ciw {
     __u8 type;
@@ -193,7 +193,7 @@  struct ciw {
 /*
  * sense-id response buffer layout
  */
-struct senseid {
+typedef struct senseid {
     /* common part */
     __u8  reserved;   /* always 0x'FF' */
     __u16 cu_type;    /* control unit type */
@@ -203,15 +203,15 @@  struct senseid {
     __u8  unused;     /* padding byte */
     /* extended part */
     struct ciw ciw[62];
-}  __attribute__ ((packed, aligned(4)));
+}  __attribute__ ((packed, aligned(4))) SenseId;
 
 /* interruption response block */
-struct irb {
+typedef struct irb {
     struct scsw scsw;
     __u32 esw[5];
     __u32 ecw[8];
     __u32 emw[8];
-}  __attribute__ ((packed, aligned(4)));
+}  __attribute__ ((packed, aligned(4))) Irb;
 
 /*
  * Some S390 specific IO instructions as inline
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 9828aa2..241c6d0 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -49,14 +49,6 @@  typedef unsigned long long __u64;
 #include "cio.h"
 #include "iplb.h"
 
-typedef struct irb Irb;
-typedef struct ccw1 Ccw1;
-typedef struct cmd_orb CmdOrb;
-typedef struct schib Schib;
-typedef struct chsc_area_sda ChscAreaSda;
-typedef struct senseid SenseId;
-typedef struct subchannel_id SubChannelId;
-
 /* start.s */
 void disabled_wait(void);
 void consume_sclp_int(void);