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
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).
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
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++; }
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>
On Sat, May 16, 2009 at 5:16 PM, Pavel Roskin proski@gnu.org 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
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) }
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
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
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
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
-
Laurent Vivier -
Pavel Roskin -
Steven Noonan