[PATCH 1/5] Remove struct ata_sector and all references to it

[Pavel Roskin]

struct ata_sector was meant to facilitate taking upper and lower byte of
the sector.  However, the implementation is incorrect, as "struct" and
"union" are swapped in the definition.  What's worse, it's an overkill
for that simple task.  The code should be transparent without knowing
how struct ata_sector is defined.
---

drivers/ide.c |   10 +++-------
 drivers/ide.h |   14 --------------
 2 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/ide.c b/drivers/ide.c
index 1166f57..4d1e58c 100644
--- a/drivers/ide.c
+++ b/drivers/ide.c
@@ -764,7 +764,6 @@ ob_ide_read_ata_chs(struct ide_drive *drive, unsigned long long block,
    unsigned int sect = (block % drive->sect) + 1;
    unsigned int head = (track % drive->head);
    unsigned int cyl = (track / drive->head);
-	struct ata_sector ata_sector;

/*
     * fill in chs command to read from disk at given location
@@ -772,8 +771,7 @@ ob_ide_read_ata_chs(struct ide_drive *drive, unsigned long long block,
    cmd->buffer = buf;
    cmd->buflen = sectors * 512;

-	ata_sector.all = sectors;
-	cmd->nsector = ata_sector.low;
+	cmd->nsector = sectors & 0xff;
    cmd->sector = sect;
    cmd->lcyl = cyl;
    cmd->hcyl = cyl >> 8;
@@ -815,19 +813,17 @@ ob_ide_read_ata_lba48(struct ide_drive *drive, unsigned long long block,
                       unsigned char *buf, unsigned int sectors)
 {
    struct ata_command *cmd = &drive->channel->ata_cmd;
-	struct ata_sector ata_sector;

memset(cmd, 0, sizeof(*cmd));

cmd->buffer = buf;
    cmd->buflen = sectors * 512;
-	ata_sector.all = sectors;

/*
     * we are using tasklet addressing here
     */
-	cmd->task[2] = ata_sector.low;
-	cmd->task[3] = ata_sector.high;
+	cmd->task[2] = sectors & 0xff;
+	cmd->task[3] = (sectors >> 8) & 0xff;
    cmd->task[4] = block;
    cmd->task[5] = block >>  8;
    cmd->task[6] = block >> 16;
diff --git a/drivers/ide.h b/drivers/ide.h
index fec0e0e..da79a71 100644
--- a/drivers/ide.h
+++ b/drivers/ide.h
@@ -208,20 +208,6 @@ enum {
    atapi_ddir_write,
 };

-struct ata_sector {
-	u16 all;
-	union {
-#ifdef CONFIG_BIG_ENDIAN
-		u8 high;
-		u8 low;
-#endif
-#ifdef CONFIG_LITTLE_ENDIAN
-		u8 low;
-		u8 high;
-#endif
-	};
-};
-
 static int ob_ide_atapi_request_sense(struct ide_drive *drive);

#endif

[PATCH 2/5] Fix HFS on little-endian systems

[Pavel Roskin]

hfs_get_ushort() and hfs_get_uint() deal with big-endian data, so simply
casting to a native type won't work on little-endian machines.  Rewrite
those macros as inline functions so that their arguments are checked.

Don't dereference type-punned pointers.  gcc 4.4 warns about it.
---

fs/hfs_mdb.h |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/hfs_mdb.h b/fs/hfs_mdb.h
index 3df549a..169b999 100644
--- a/fs/hfs_mdb.h
+++ b/fs/hfs_mdb.h
@@ -19,8 +19,15 @@ typedef unsigned char hfs_char_t;
 typedef unsigned char hfs_ushort_t[2];
 typedef unsigned char hfs_uint_t[4];

-#define hfs_get_ushort(addr)    (*((unsigned short*)(addr)))
-#define hfs_get_uint(addr)      (*((unsigned int*)(addr)))
+static inline unsigned short hfs_get_ushort(hfs_ushort_t addr)
+{
+	return (addr[0] << 8) | addr[1];
+}
+
+static inline unsigned short hfs_get_uint(hfs_uint_t addr)
+{
+	return (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) | addr[3];
+}

/*
  * The HFS Master Directory Block (MDB).

[PATCH 2/5] Fix HFS on little-endian systems

[Laurent Vivier]

Le samedi 16 mai 2009 à 20:16 -0400, Pavel Roskin a écrit :
> hfs_get_ushort() and hfs_get_uint() deal with big-endian data, so simply
> casting to a native type won't work on little-endian machines.  Rewrite
> those macros as inline functions so that their arguments are checked.
> Don't dereference type-punned pointers.  gcc 4.4 warns about it.
>
> fs/hfs_mdb.h |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/hfs_mdb.h b/fs/hfs_mdb.h
> index 3df549a..169b999 100644
> --- a/fs/hfs_mdb.h
> +++ b/fs/hfs_mdb.h
> @@ -19,8 +19,15 @@ typedef unsigned char hfs_char_t;
>  typedef unsigned char hfs_ushort_t[2];
>  typedef unsigned char hfs_uint_t[4];
>
> -#define hfs_get_ushort(addr)    (*((unsigned short*)(addr)))
> -#define hfs_get_uint(addr)      (*((unsigned int*)(addr)))
> +static inline unsigned short hfs_get_ushort(hfs_ushort_t addr)
> +{
> return (addr[0] << 8) | addr[1];
>
> +}
>
> +static inline unsigned short hfs_get_uint(hfs_uint_t addr)
> +{
> return (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) | addr[3];
>
> +}

Could you use __be32_to_cpu() and __be16_to_cpu() instead ?

Laurent

[PATCH 3/5] Fix gcc 4.4 warnings about strict aliasing

[Pavel Roskin]

Rather than cast char arrays to other types, use correct types and cast
them to char pointers when needed.
---

drivers/fw_cfg.c   |   18 +++++++++---------
 modules/pc-parts.c |   18 +++++++++---------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/fw_cfg.c b/drivers/fw_cfg.c
index 0ca4a43..3c5e267 100644
--- a/drivers/fw_cfg.c
+++ b/drivers/fw_cfg.c
@@ -34,31 +34,31 @@ fw_cfg_read(uint16_t cmd, char *buf, unsigned int nbytes)
 uint64_t
 fw_cfg_read_i64(uint16_t cmd)
 {
-    char buf[sizeof(uint64_t)];
+    uint64_t buf;

-    fw_cfg_read(cmd, buf, sizeof(uint64_t));
+    fw_cfg_read(cmd, (char *)&buf, sizeof(uint64_t));

-    return __le64_to_cpu(*(uint64_t *)buf);
+    return __le64_to_cpu(buf);
 }

uint32_t
 fw_cfg_read_i32(uint16_t cmd)
 {
-    char buf[sizeof(uint32_t)];
+    uint32_t buf;

-    fw_cfg_read(cmd, buf, sizeof(uint32_t));
+    fw_cfg_read(cmd, (char *)&buf, sizeof(uint32_t));

-    return __le32_to_cpu(*(uint32_t *)buf);
+    return __le32_to_cpu(buf);
 }

uint16_t
 fw_cfg_read_i16(uint16_t cmd)
 {
-    char buf[sizeof(uint16_t)];
+    uint16_t buf;

-    fw_cfg_read(cmd, buf, sizeof(uint16_t));
+    fw_cfg_read(cmd, (char *)&buf, sizeof(uint16_t));

-    return __le16_to_cpu(*(uint16_t *)buf);
+    return __le16_to_cpu(buf);
 }

void
diff --git a/modules/pc-parts.c b/modules/pc-parts.c
index 3cececb..bc3109a 100644
--- a/modules/pc-parts.c
+++ b/modules/pc-parts.c
@@ -58,8 +58,8 @@ pcparts_open( pcparts_info_t *di )
    	unsigned char e_head;
    	unsigned char e_sector;
    	unsigned char e_cyl;
-		unsigned char start_sect[4]; /* unaligned little endian */
-		unsigned char nr_sects[4]; /* ditto */
+		u32 start_sect; /* unaligned little endian */
+		u32 nr_sects; /* ditto */
    } *p;
    unsigned char buf[512];

@@ -96,11 +96,11 @@ pcparts_open( pcparts_info_t *di )
    		printk("partition %d does not exist\n", parnum+1 );
    		RET( 0 );
    	}
-		di->offs = (llong)(__le32_to_cpu(*((u32 *)p->start_sect))) * bs;
-		di->size = (llong)(__le32_to_cpu(*((u32 *)p->nr_sects))) * bs;
+		di->offs = (llong)(__le32_to_cpu(p->start_sect)) * bs;
+		di->size = (llong)(__le32_to_cpu(p->nr_sects)) * bs;

/* printk("Primary partition at sector %x\n",
-				__le32_to_cpu(*((u32 *)p->start_sect))); */
+				__le32_to_cpu(p->start_sect)); */

RET( -1 );
    } else {
@@ -123,7 +123,7 @@ pcparts_open( pcparts_info_t *di )
    	printk("Extended partition at %d\n", i+1);

/* Visit each logical partition labels */
-		ext_start = __le32_to_cpu(*((u32 *)p[i].start_sect));
+		ext_start = __le32_to_cpu(p[i].start_sect);
    	cur_table = ext_start;
    	cur_part = 4;

@@ -147,8 +147,8 @@ pcparts_open( pcparts_info_t *di )
    				RET( 0 );
    			}
    			di->offs =
-					(llong)(cur_table+__le32_to_cpu(*((u32 *)p->start_sect))) * bs;
-				di->size = (llong)__le32_to_cpu(*((u32 *)p->nr_sects)) * bs;
+					(llong)(cur_table+__le32_to_cpu(p->start_sect)) * bs;
+				di->size = (llong)__le32_to_cpu(p->nr_sects) * bs;
    			RET ( -1 );
    		}

@@ -157,7 +157,7 @@ pcparts_open( pcparts_info_t *di )
    			printk("no link\n");
    			break;
    		}
-			cur_table = ext_start + __le32_to_cpu(*((u32 *)p[1].start_sect));
+			cur_table = ext_start + __le32_to_cpu(p[1].start_sect);

cur_part++;
    	}

[PATCH 4/5] Use -fno-strict-aliasing flag for all filesystems imported from GRUB.

[Pavel Roskin]

Otherwise, they won't compile with gcc 4.4.  Fixing the code would
require massive changes, which is not practical for the code taken from
another project.
---

fs/grubfs/build.xml |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/grubfs/build.xml b/fs/grubfs/build.xml
index 7469313..e5fb64a 100644
--- a/fs/grubfs/build.xml
+++ b/fs/grubfs/build.xml
@@ -1,17 +1,17 @@
 <build>
  <library name="fs" type="static" target="target">
   <object source="grubfs_fs.c"/>
-  <object source="fsys_ext2fs.c" condition="FSYS_EXT2FS" flags="-DFSYS_EXT2FS"/>
-  <object source="fsys_fat.c" condition="FSYS_FAT" flags="-DFSYS_FAT"/>
-  <object source="fsys_jfs.c" condition="FSYS_JFS" flags="-DFSYS_JFS"/>
-  <object source="fsys_minix.c" condition="FSYS_MINIX" flags="-DFSYS_MINIX"/>
-  <object source="fsys_reiserfs.c" condition="FSYS_REISERFS" flags="-DFSYS_REISERFS"/>
-  <object source="fsys_xfs.c" condition="FSYS_XFS" flags="-DFSYS_XFS"/>
-  <object source="fsys_ufs.c" condition="FSYS_UFS" flags="-DFSYS_UFS"/>
-  <object source="fsys_ffs.c" condition="FSYS_FFS" flags="-DFSYS_FFS"/>
-  <object source="fsys_vstafs.c" condition="FSYS_VSTAFS" flags="-DFSYS_VSTAFS"/>
-  <object source="fsys_iso9660.c" condition="FSYS_ISO9660" flags="-DFSYS_ISO9660"/>
-  <object source="fsys_ntfs.c" condition="FSYS_NTFS" flags="-DFSYS_NTFS"/>
-  <object source="fsys_affs.c" condition="FSYS_AFFS" flags="-DFSYS_AFFS"/>
+  <object source="fsys_ext2fs.c" condition="FSYS_EXT2FS" flags="-DFSYS_EXT2FS -fno-strict-aliasing"/>
+  <object source="fsys_fat.c" condition="FSYS_FAT" flags="-DFSYS_FAT -fno-strict-aliasing"/>
+  <object source="fsys_jfs.c" condition="FSYS_JFS" flags="-DFSYS_JFS -fno-strict-aliasing"/>
+  <object source="fsys_minix.c" condition="FSYS_MINIX" flags="-DFSYS_MINIX -fno-strict-aliasing"/>
+  <object source="fsys_reiserfs.c" condition="FSYS_REISERFS" flags="-DFSYS_REISERFS -fno-strict-aliasing"/>
+  <object source="fsys_xfs.c" condition="FSYS_XFS" flags="-DFSYS_XFS -fno-strict-aliasing"/>
+  <object source="fsys_ufs.c" condition="FSYS_UFS" flags="-DFSYS_UFS -fno-strict-aliasing"/>
+  <object source="fsys_ffs.c" condition="FSYS_FFS" flags="-DFSYS_FFS -fno-strict-aliasing"/>
+  <object source="fsys_vstafs.c" condition="FSYS_VSTAFS" flags="-DFSYS_VSTAFS -fno-strict-aliasing"/>
+  <object source="fsys_iso9660.c" condition="FSYS_ISO9660" flags="-DFSYS_ISO9660 -fno-strict-aliasing"/>
+  <object source="fsys_ntfs.c" condition="FSYS_NTFS" flags="-DFSYS_NTFS -fno-strict-aliasing"/>
+  <object source="fsys_affs.c" condition="FSYS_AFFS" flags="-DFSYS_AFFS -fno-strict-aliasing"/>
  </library>
 </build>

[PATCH 4/5] Use -fno-strict-aliasing flag for all filesystems imported from GRUB.

[Steven Noonan]

On Sat, May 16, 2009 at 5:16 PM, Pavel Roskin  wrote:
> Otherwise, they won't compile with gcc 4.4.  Fixing the code would
> require massive changes, which is not practical for the code taken from
> another project.

Besides, type-based aliasing is idiotic.

http://lwn.net/Articles/316126/

- Steven

[PATCH 5/5] Consolidate .sbss and .sbss.* sections and align them on PowerPC

[Pavel Roskin]

This is needed on Fedora 11 (GNU ld 2.19.51.0.2).  Otherwise, qemu
reports on statup:

invalid/unsupported opcode: 00 - 18 - 01 (00004070) 00000004 1
invalid/unsupported opcode: 00 - 04 - 17 (000095c8) 000095ec 0
---

arch/ppc/qemu/ldscript |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/ppc/qemu/ldscript b/arch/ppc/qemu/ldscript
index 66fcbcd..4a697af 100644
--- a/arch/ppc/qemu/ldscript
+++ b/arch/ppc/qemu/ldscript
@@ -52,6 +52,13 @@ SECTIONS
    _ebss = .;
     }

+    .sbss ALIGN(4096): {
+        _sbss = .;
+	*(.sbss)
+	*(.sbss.*)
+	_esbss = .;
+    }
+
     . = HRESET_ADDR;

.romentry : { *(.romentry) }

[PATCH 5/5] Consolidate .sbss and .sbss.* sections and align them on PowerPC

[Pavel Roskin]

On Sat, 2009-05-16 at 20:16 -0400, Pavel Roskin wrote:
> This is needed on Fedora 11 (GNU ld 2.19.51.0.2).  Otherwise, qemu
> reports on statup:
>
> invalid/unsupported opcode: 00 - 18 - 01 (00004070) 00000004 1
> invalid/unsupported opcode: 00 - 04 - 17 (000095c8) 000095ec 0

Actually, I don't feel strongly about this patch.  It's the first
version that worked.

I checked Linux sources (arch/powerpc/kernel/vmlinux.lds.S) and I see
that .sbss is put to the .bss section in front of .bss itself.  Perhaps
that's what we should be doing in OpenBIOS.

This patch works too:

--- a/arch/ppc/qemu/ldscript
+++ b/arch/ppc/qemu/ldscript
@@ -46,6 +46,8 @@ SECTIONS

.bss ALIGN(4096): {
         _bss = .;
+	*(.sbss)
+	*(.sbss.*)
    *(.bss)
    *(.bss.*)
    *(COMMON)

The problem is discussed at
https://bugzilla.redhat.com/show_bug.cgi?id=494075

[PATCH 5/5] Consolidate .sbss and .sbss.* sections and align them on PowerPC

[Laurent Vivier]

Le samedi 16 mai 2009 à 21:04 -0400, Pavel Roskin a écrit :
> On Sat, 2009-05-16 at 20:16 -0400, Pavel Roskin wrote:
> > This is needed on Fedora 11 (GNU ld 2.19.51.0.2).  Otherwise, qemu
> > reports on statup:
> >
> > invalid/unsupported opcode: 00 - 18 - 01 (00004070) 00000004 1
> > invalid/unsupported opcode: 00 - 04 - 17 (000095c8) 000095ec 0
>
> Actually, I don't feel strongly about this patch.  It's the first
> version that worked.
>
> I checked Linux sources (arch/powerpc/kernel/vmlinux.lds.S) and I see
> that .sbss is put to the .bss section in front of .bss itself.  Perhaps
> that's what we should be doing in OpenBIOS.
>
> This patch works too:
>
> --- a/arch/ppc/qemu/ldscript
> +++ b/arch/ppc/qemu/ldscript
> @@ -46,6 +46,8 @@ SECTIONS
>  .bss ALIGN(4096): {
>      _bss = .;
> *(.sbss)
> *(.sbss.*)
> *(.bss)
> *(.bss.*)
> *(COMMON)
>
> The problem is discussed at
> https://bugzilla.redhat.com/show_bug.cgi?id=494075

I've applied this one as commit r488.

Thanks,
Laurent

[PATCH 1/5] Remove struct ata_sector and all references to it

[Laurent Vivier]

This patch looks good, except one comment, see below.

Regards,
Laurent

Le samedi 16 mai 2009 à 20:16 -0400, Pavel Roskin a écrit :
> struct ata_sector was meant to facilitate taking upper and lower byte of
> the sector.  However, the implementation is incorrect, as "struct" and
> "union" are swapped in the definition.  What's worse, it's an overkill
> for that simple task.  The code should be transparent without knowing
> how struct ata_sector is defined.
>
> drivers/ide.c |   10 +++-------
>  drivers/ide.h |   14 --------------
>  2 files changed, 3 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/ide.c b/drivers/ide.c
> index 1166f57..4d1e58c 100644
> --- a/drivers/ide.c
> +++ b/drivers/ide.c
> @@ -764,7 +764,6 @@ ob_ide_read_ata_chs(struct ide_drive *drive, unsigned long long block,
>    unsigned int sect = (block % drive->sect) + 1;
>    unsigned int head = (track % drive->head);
>    unsigned int cyl = (track / drive->head);
>
> struct ata_sector ata_sector;
>
> /*
> fill in chs command to read from disk at given location
>
> @@ -772,8 +771,7 @@ ob_ide_read_ata_chs(struct ide_drive *drive, unsigned long long block,
>    cmd->buffer = buf;
>    cmd->buflen = sectors * 512;
> ata_sector.all = sectors;
> cmd->nsector = ata_sector.low;
> cmd->nsector = sectors & 0xff;
> cmd->sector = sect;
> cmd->lcyl = cyl;
> cmd->hcyl = cyl >> 8;
>
> @@ -815,19 +813,17 @@ ob_ide_read_ata_lba48(struct ide_drive *drive, unsigned long long block,
>                        unsigned char *buf, unsigned int sectors)
>  {
>    struct ata_command *cmd = &drive->channel->ata_cmd;
>
> struct ata_sector ata_sector;
>
> memset(cmd, 0, sizeof(*cmd));
>
> cmd->buffer = buf;
> cmd->buflen = sectors * 512;
>
> ata_sector.all = sectors;
>
> /*
> we are using tasklet addressing here
>
> */
>
> cmd->task[2] = ata_sector.low;
>
> cmd->task[3] = ata_sector.high;
> cmd->task[2] = sectors & 0xff;
> cmd->task[3] = (sectors >> 8) & 0xff;

This "& 0xff" is useless as task is "unsigned char", see below for
block:
> cmd->task[4] = block;
>    cmd->task[5] = block >>  8;
>    cmd->task[6] = block >> 16;
> diff --git a/drivers/ide.h b/drivers/ide.h
> index fec0e0e..da79a71 100644
> --- a/drivers/ide.h
> +++ b/drivers/ide.h
> @@ -208,20 +208,6 @@ enum {
>    atapi_ddir_write,
>  };
>
> -struct ata_sector {
> u16 all;
> union {
>
> -#ifdef CONFIG_BIG_ENDIAN
> u8 high;
> u8 low;
>
> -#endif
> -#ifdef CONFIG_LITTLE_ENDIAN
> u8 low;
> u8 high;
>
> -#endif
> };
>
> -};
>
> static int ob_ide_atapi_request_sense(struct ide_drive *drive);
>
> #endif

[PATCH 1/5] Remove struct ata_sector and all references to it

[Laurent Vivier]

Le samedi 16 mai 2009 à 20:16 -0400, Pavel Roskin a écrit :
> struct ata_sector was meant to facilitate taking upper and lower byte of
> the sector.  However, the implementation is incorrect, as "struct" and
> "union" are swapped in the definition.  What's worse, it's an overkill
> for that simple task.  The code should be transparent without knowing
> how struct ata_sector is defined.

Applied a slightly modified patch (remove useless "& 0xff") as r489

Thanks,
Laurent