* [PATCH 1/1] build modular usb isd200 with modular ide
@ 2004-10-24 0:55 Doug Maxey
2004-10-24 10:03 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Doug Maxey @ 2004-10-24 0:55 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Paul Mackerras, Greg Kroah-Hartman
Cc: linuxppc64-dev, Linux IDE Mailing List, Linux SCSI Mailing List,
Jacob Moilanen, Olof Johansson, dwm
Name: inline ide_fix_driveid()
Rationale:
This is a fix for bugme.osdl 3819.
With any of the 2.6.9 release flavors (vanilla, mm1, ac3), one
cannot build the usb isd200 module due to the dependency on
ide_fix_driveid() being exported from ide-iops.
Description:
When building IDE modular, the current ide_fix_driveid() is
exported from ide-iops.c. This patch makes the function an inline.
Status: compile tested on ppc64. Other issues prevent run test.
Signed-off-by: Doug Maxey <dwm@austin.ibm.com>
ChangeLog:
++doug
drivers/ide/ide-iops.c | 98 -------------------------------------------------
include/linux/ide.h | 48 +++++++++++++++++++++++-
2 files changed, 47 insertions(+), 99 deletions(-)
diff -Nwupa lk-2.6.9-mm1/drivers/ide/ide-iops.c lk-2.6.9-mm1.edit/drivers/ide/ide-iops.c
--- lk-2.6.9-mm1/drivers/ide/ide-iops.c 2004-10-22 15:10:30.465342832 -0500
+++ lk-2.6.9-mm1.edit/drivers/ide/ide-iops.c 2004-10-23 00:22:16.901355000 -0500
@@ -352,104 +352,6 @@ EXPORT_SYMBOL(atapi_output_bytes);
/*
* Beginning of Taskfile OPCODE Library and feature sets.
*/
-void ide_fix_driveid (struct hd_driveid *id)
-{
-#ifndef __LITTLE_ENDIAN
-# ifdef __BIG_ENDIAN
- int i;
- u16 *stringcast;
-
- id->config = __le16_to_cpu(id->config);
- id->cyls = __le16_to_cpu(id->cyls);
- id->reserved2 = __le16_to_cpu(id->reserved2);
- id->heads = __le16_to_cpu(id->heads);
- id->track_bytes = __le16_to_cpu(id->track_bytes);
- id->sector_bytes = __le16_to_cpu(id->sector_bytes);
- id->sectors = __le16_to_cpu(id->sectors);
- id->vendor0 = __le16_to_cpu(id->vendor0);
- id->vendor1 = __le16_to_cpu(id->vendor1);
- id->vendor2 = __le16_to_cpu(id->vendor2);
- stringcast = (u16 *)&id->serial_no[0];
- for (i = 0; i < (20/2); i++)
- stringcast[i] = __le16_to_cpu(stringcast[i]);
- id->buf_type = __le16_to_cpu(id->buf_type);
- id->buf_size = __le16_to_cpu(id->buf_size);
- id->ecc_bytes = __le16_to_cpu(id->ecc_bytes);
- stringcast = (u16 *)&id->fw_rev[0];
- for (i = 0; i < (8/2); i++)
- stringcast[i] = __le16_to_cpu(stringcast[i]);
- stringcast = (u16 *)&id->model[0];
- for (i = 0; i < (40/2); i++)
- stringcast[i] = __le16_to_cpu(stringcast[i]);
- id->dword_io = __le16_to_cpu(id->dword_io);
- id->reserved50 = __le16_to_cpu(id->reserved50);
- id->field_valid = __le16_to_cpu(id->field_valid);
- id->cur_cyls = __le16_to_cpu(id->cur_cyls);
- id->cur_heads = __le16_to_cpu(id->cur_heads);
- id->cur_sectors = __le16_to_cpu(id->cur_sectors);
- id->cur_capacity0 = __le16_to_cpu(id->cur_capacity0);
- id->cur_capacity1 = __le16_to_cpu(id->cur_capacity1);
- id->lba_capacity = __le32_to_cpu(id->lba_capacity);
- id->dma_1word = __le16_to_cpu(id->dma_1word);
- id->dma_mword = __le16_to_cpu(id->dma_mword);
- id->eide_pio_modes = __le16_to_cpu(id->eide_pio_modes);
- id->eide_dma_min = __le16_to_cpu(id->eide_dma_min);
- id->eide_dma_time = __le16_to_cpu(id->eide_dma_time);
- id->eide_pio = __le16_to_cpu(id->eide_pio);
- id->eide_pio_iordy = __le16_to_cpu(id->eide_pio_iordy);
- for (i = 0; i < 2; ++i)
- id->words69_70[i] = __le16_to_cpu(id->words69_70[i]);
- for (i = 0; i < 4; ++i)
- id->words71_74[i] = __le16_to_cpu(id->words71_74[i]);
- id->queue_depth = __le16_to_cpu(id->queue_depth);
- for (i = 0; i < 4; ++i)
- id->words76_79[i] = __le16_to_cpu(id->words76_79[i]);
- id->major_rev_num = __le16_to_cpu(id->major_rev_num);
- id->minor_rev_num = __le16_to_cpu(id->minor_rev_num);
- id->command_set_1 = __le16_to_cpu(id->command_set_1);
- id->command_set_2 = __le16_to_cpu(id->command_set_2);
- id->cfsse = __le16_to_cpu(id->cfsse);
- id->cfs_enable_1 = __le16_to_cpu(id->cfs_enable_1);
- id->cfs_enable_2 = __le16_to_cpu(id->cfs_enable_2);
- id->csf_default = __le16_to_cpu(id->csf_default);
- id->dma_ultra = __le16_to_cpu(id->dma_ultra);
- id->trseuc = __le16_to_cpu(id->trseuc);
- id->trsEuc = __le16_to_cpu(id->trsEuc);
- id->CurAPMvalues = __le16_to_cpu(id->CurAPMvalues);
- id->mprc = __le16_to_cpu(id->mprc);
- id->hw_config = __le16_to_cpu(id->hw_config);
- id->acoustic = __le16_to_cpu(id->acoustic);
- id->msrqs = __le16_to_cpu(id->msrqs);
- id->sxfert = __le16_to_cpu(id->sxfert);
- id->sal = __le16_to_cpu(id->sal);
- id->spg = __le32_to_cpu(id->spg);
- id->lba_capacity_2 = __le64_to_cpu(id->lba_capacity_2);
- for (i = 0; i < 22; i++)
- id->words104_125[i] = __le16_to_cpu(id->words104_125[i]);
- id->last_lun = __le16_to_cpu(id->last_lun);
- id->word127 = __le16_to_cpu(id->word127);
- id->dlf = __le16_to_cpu(id->dlf);
- id->csfo = __le16_to_cpu(id->csfo);
- for (i = 0; i < 26; i++)
- id->words130_155[i] = __le16_to_cpu(id->words130_155[i]);
- id->word156 = __le16_to_cpu(id->word156);
- for (i = 0; i < 3; i++)
- id->words157_159[i] = __le16_to_cpu(id->words157_159[i]);
- id->cfa_power = __le16_to_cpu(id->cfa_power);
- for (i = 0; i < 14; i++)
- id->words161_175[i] = __le16_to_cpu(id->words161_175[i]);
- for (i = 0; i < 31; i++)
- id->words176_205[i] = __le16_to_cpu(id->words176_205[i]);
- for (i = 0; i < 48; i++)
- id->words206_254[i] = __le16_to_cpu(id->words206_254[i]);
- id->integrity_word = __le16_to_cpu(id->integrity_word);
-# else
-# error "Please fix <asm/byteorder.h>"
-# endif
-#endif
-}
-
-EXPORT_SYMBOL(ide_fix_driveid);
void ide_fixstring (u8 *s, const int bytecount, const int byteswap)
{
diff -Nwupa lk-2.6.9-mm1/include/linux/ide.h lk-2.6.9-mm1.edit/include/linux/ide.h
--- lk-2.6.9-mm1/include/linux/ide.h 2004-10-22 15:10:36.748318728 -0500
+++ lk-2.6.9-mm1.edit/include/linux/ide.h 2004-10-23 15:28:52.635380680 -0500
@@ -1204,7 +1204,53 @@ extern ide_startstop_t ide_abort(ide_dri
*/
extern void ide_cmd(ide_drive_t *, u8, u8, ide_handler_t *);
-extern void ide_fix_driveid(struct hd_driveid *);
+/*
+ * ide_fix_driveid - fix IDENTIFY DEVICE data for big endian machines.
+ * @id - pointer to data from drive.
+ *
+ * Could be a one liner except for the 3 x 32 bit and 2 x 64 bit
+ * fields. Offsets are from d1532v1r4.
+ */
+static inline void ide_fix_driveid (struct hd_driveid *id)
+{
+#ifndef __LITTLE_ENDIAN
+# ifdef __BIG_ENDIAN
+ u16 *sp = (u16*)id;
+
+ for (; sp < ((u16*)id) + 61; sp++) *sp = __le16_to_cpu(*sp);
+
+ /* lba_capacity */
+ *((u32*)sp) = __le32_to_cpu(*((u32*)sp));
+ sp += 2;
+
+ for (; sp < ((u16*)id) + 98; sp++)
+ *sp = __le16_to_cpu(*sp);
+
+ /* Streaming Perfomance Granularity. words 98-99 */
+ *((u32*)sp) = __le32_to_cpu(*((u32*)sp));
+ sp += 2; /* word 100 */
+
+ /* lba_capacity2. words 100-103 */
+ *((u64*)sp) = __le64_to_cpu(*((u64*)sp));
+ sp += 4; /* word 104 */
+
+ for (; sp < ((u16*)id) + 117; sp++)
+ *sp = __le16_to_cpu(*sp);
+
+
+ /* Words per Logical Sector. words 117-118 */
+ *((u32*)sp) = __le32_to_cpu(*((u32*)sp));
+ sp += 2; /* word 119 */
+
+ for (; sp < ((u16*)id) + 256; sp++)
+ *sp = __le16_to_cpu(*sp);
+
+# else
+# error "Please fix <asm/byteorder.h>"
+# endif
+#endif
+}
+
/*
* ide_fixstring() cleans up and (optionally) byte-swaps a text string,
* removing leading/trailing blanks and compressing internal blanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-24 0:55 [PATCH 1/1] build modular usb isd200 with modular ide Doug Maxey
@ 2004-10-24 10:03 ` Christoph Hellwig
2004-10-24 12:45 ` Bartlomiej Zolnierkiewicz
2004-10-24 23:11 ` Doug Maxey
0 siblings, 2 replies; 10+ messages in thread
From: Christoph Hellwig @ 2004-10-24 10:03 UTC (permalink / raw)
To: Doug Maxey
Cc: Bartlomiej Zolnierkiewicz, Paul Mackerras, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Sat, Oct 23, 2004 at 07:55:36PM -0500, Doug Maxey wrote:
>
> Name: inline ide_fix_driveid()
>
> Rationale:
> This is a fix for bugme.osdl 3819.
bugme.osdl.org doesn't know of a bug #3819.
> With any of the 2.6.9 release flavors (vanilla, mm1, ac3), one
> cannot build the usb isd200 module due to the dependency on
> ide_fix_driveid() being exported from ide-iops.
>
> Description:
> When building IDE modular, the current ide_fix_driveid() is
> exported from ide-iops.c. This patch makes the function an inline.
Still doesn't make any sense. ide_fix_driveid is properly exported from
ide-iops.c, so you use it from other modules. The only case that
doesn't work is modular ide and builtin usb-storage, and the BLK_DEV_IDE
depency should fix that one.
If you think that depency is ugly (I do) just copy the routine to
isd200.c, it's a) too large to inline but b) just a trivial byteswap
that should need much changes over time.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-24 10:03 ` Christoph Hellwig
@ 2004-10-24 12:45 ` Bartlomiej Zolnierkiewicz
2004-10-25 22:55 ` Doug Maxey
2004-10-24 23:11 ` Doug Maxey
1 sibling, 1 reply; 10+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2004-10-24 12:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Doug Maxey, Paul Mackerras, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Sun, 24 Oct 2004 12:03:19 +0200, Christoph Hellwig <hch@lst.de> wrote:
> On Sat, Oct 23, 2004 at 07:55:36PM -0500, Doug Maxey wrote:
> >
> > Name: inline ide_fix_driveid()
> >
> > Rationale:
> > This is a fix for bugme.osdl 3819.
>
> bugme.osdl.org doesn't know of a bug #3819.
>
> > With any of the 2.6.9 release flavors (vanilla, mm1, ac3), one
> > cannot build the usb isd200 module due to the dependency on
> > ide_fix_driveid() being exported from ide-iops.
> >
> > Description:
> > When building IDE modular, the current ide_fix_driveid() is
> > exported from ide-iops.c. This patch makes the function an inline.
>
> Still doesn't make any sense. ide_fix_driveid is properly exported from
> ide-iops.c, so you use it from other modules. The only case that
> doesn't work is modular ide and builtin usb-storage, and the BLK_DEV_IDE
> depency should fix that one.
The new ide_fix_driveid function seems buggy,
ie. it byte-swaps id->max_multsect with id->vendor3.
> If you think that depency is ugly (I do) just copy the routine to
> isd200.c, it's a) too large to inline but b) just a trivial byteswap
> that should need much changes over time.
The dependency is a bug, <linux/ide.h> is for IDE driver only.
Doug, if you kill debugging code in isd200.c then only:
id->command_set_1
id->model
id->fw_rev
id->capability
id->lba_capacity
id->heads
id->cyls
id->sectors
id->command_set_2
need to be byte-swapped.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-24 10:03 ` Christoph Hellwig
2004-10-24 12:45 ` Bartlomiej Zolnierkiewicz
@ 2004-10-24 23:11 ` Doug Maxey
1 sibling, 0 replies; 10+ messages in thread
From: Doug Maxey @ 2004-10-24 23:11 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bartlomiej Zolnierkiewicz, Paul Mackerras, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Sun, 24 Oct 2004 12:03:19 +0200, Christoph Hellwig wrote:
>On Sat, Oct 23, 2004 at 07:55:36PM -0500, Doug Maxey wrote:
>>
>> Name: inline ide_fix_driveid()
>>
>> Rationale:
>> This is a fix for bugme.osdl 3819.
>
>bugme.osdl.org doesn't know of a bug #3819.
Uh Oh. Should be 3618. Have no idea where 3819 came from.
>
>> With any of the 2.6.9 release flavors (vanilla, mm1, ac3), one
>> cannot build the usb isd200 module due to the dependency on
>> ide_fix_driveid() being exported from ide-iops.
>>
>> Description:
>> When building IDE modular, the current ide_fix_driveid() is
>> exported from ide-iops.c. This patch makes the function an inline.
>
>Still doesn't make any sense. ide_fix_driveid is properly exported from
>ide-iops.c, so you use it from other modules. The only case that
>doesn't work is modular ide and builtin usb-storage, and the BLK_DEV_IDE
>depency should fix that one.
>
>If you think that depency is ugly (I do) just copy the routine to
What happened to common code that may have more uses than originally
intended? Do it right once in one place, and make it available.
>isd200.c, it's a) too large to inline but b) just a trivial byteswap
>that should need much changes over time.
Except for those few 32 and 64 bit quantities that need word (16 bit) swaps,
I agree completely.
The points I was trying to make were that
1) This is called in only a few places.
2) it is never on a fast path.
2) The sequence of the named elements was a little bit much. Meaning of
the words change, and quite a few of the fields no longer have the
original meaning or definition.
3) Having a singular (even if somewhat large) inline handles all current (and
future) uses.
++doug
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-24 12:45 ` Bartlomiej Zolnierkiewicz
@ 2004-10-25 22:55 ` Doug Maxey
2004-10-25 23:12 ` Paul Mackerras
2004-10-25 23:20 ` Bartlomiej Zolnierkiewicz
0 siblings, 2 replies; 10+ messages in thread
From: Doug Maxey @ 2004-10-25 22:55 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Christoph Hellwig, Paul Mackerras, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Sun, 24 Oct 2004 14:45:53 +0200, Bartlomiej Zolnierkiewicz wrote:
...
>
>The new ide_fix_driveid function seems buggy,
>ie. it byte-swaps id->max_multsect with id->vendor3.
Ok, lets look at those vars. Both are defined in hdreg.h as bytes.
No fields in the data from the device are bytes, but are 16 bit. On big
endian, the relative positions for an LE u16 are swapped. If the swap is
not done on those, then one replaces the other when read. Probably not
what was intended. It appears that another bug is being fixed here.
Do you not agree that all reads when doing IDENTIFY xxx DEVICE are
retrieved as u16? If not, then the current ide_fix_driveid() code is
wrong also.
Backup data, taken from the raw bits on the wire via datatransit:
$ hexdump -C eio/ata/041025-2.6.9-rc3-wcd-4-dwm.data.bin
00000000 40 00 ff 3f 37 c8 10 00 00 00 00 00 3f 00 00 00 |@..?7.......?...|
00000010 00 00 00 00 20 20 20 20 20 20 20 20 20 20 36 20 |.... 6 |
00000020 54 34 30 33 32 30 41 32 00 00 00 00 30 00 42 50 |T40320A2....0.BP|
00000030 30 31 45 33 20 20 4f 54 48 53 42 49 20 41 4b 4d |01E3 OTHSBI AKM|
00000040 30 34 36 32 41 47 42 58 20 20 20 20 20 20 20 20 |0462AGBX |
00000050 20 20 20 20 20 20 20 20 20 20 20 20 20 20 10 80 | ..|
00000060 00 00 00 2f 00 40 00 02 00 00 07 00 ff 3f 10 00 |.../.@.......?..|
00000070 3f 00 10 fc fb 00 10 01 00 53 a8 04 07 00 07 00 |?........S......|
00000080 03 00 78 00 78 00 78 00 78 00 00 00 00 00 00 00 |..x.x.x.x.......|
00000090 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
000000a0 7e 00 00 00 6b 7c 08 59 03 40 49 7c 08 18 03 40 |~...k|.Y.@I|...@|
000000b0 3f 20 0f 00 00 00 80 00 fe ff 4b 60 00 00 00 00 |? ........K`....|
000000c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
00000100 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
00000110 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
*
000001f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a5 89 |................|
00000200
Note that bytes 5E-5F are '10 80'. Per d1532, table 15, in the version I am
looking at:
47 M F 15-8 80h
F 7-0 00h = Reserved
F 01h-FFh = Maximum number of sectors that shall be transferred per
interrupt on READ/WRITE MULTIPLE commands
To match max_multisect and vendor3, the bytes must be swapped.
unsigned char max_multsect; /* 0=not_implemented */
unsigned char vendor3; /* vendor unique */
Ouch! Oh man. Depending on LE byte ordering in a u16, but only for certain
vars. Should this be ifdef'd in hdregs.h? And, and, oh jeez...
What is the solution here? Preserve the definitely non-arch neutral format
in hdregs.h? All the char values are troubling.
Or copy and rename the entire ide_fix_driveid() into isd200? This would be
Christoph's choice.
...
>The dependency is a bug, <linux/ide.h> is for IDE driver only.
The isd200 _is_ a bridge to ATA/ATAPI devices. Does this mean it cannot use
common code, just because it is not in drivers/ide?
>
>Doug, if you kill debugging code in isd200.c then only:
>
>id->command_set_1
>id->model
>id->fw_rev
>id->capability
>id->lba_capacity
>id->heads
>id->cyls
>id->sectors
>id->command_set_2
>
>need to be byte-swapped.
>
I don't plan on killing any debug code.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-25 22:55 ` Doug Maxey
@ 2004-10-25 23:12 ` Paul Mackerras
2004-10-25 23:55 ` Doug Maxey
2004-10-25 23:20 ` Bartlomiej Zolnierkiewicz
1 sibling, 1 reply; 10+ messages in thread
From: Paul Mackerras @ 2004-10-25 23:12 UTC (permalink / raw)
To: Doug Maxey
Cc: Bartlomiej Zolnierkiewicz, Christoph Hellwig, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
Doug Maxey writes:
> Ok, lets look at those vars. Both are defined in hdreg.h as bytes.
> No fields in the data from the device are bytes, but are 16 bit. On big
> endian, the relative positions for an LE u16 are swapped. If the swap is
> not done on those, then one replaces the other when read. Probably not
> what was intended. It appears that another bug is being fixed here.
No. The only sane way to do things is to transfer data from the
device to memory as a byte stream, in other words, preserving the
ordering of the individual bytes. That is what we do on PPC and PPC64
platforms. That ordering is preserved (and must be preserved)
irrespective of whether the transfer is actually done in 8, 16 or 32
bit chunks.
That means that 16-bit quantities might need to be byte-swapped to be
interpreted in host byte order, but single-byte fields should always
be in their correct sequence.
Paul.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-25 22:55 ` Doug Maxey
2004-10-25 23:12 ` Paul Mackerras
@ 2004-10-25 23:20 ` Bartlomiej Zolnierkiewicz
1 sibling, 0 replies; 10+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2004-10-25 23:20 UTC (permalink / raw)
To: Doug Maxey
Cc: Christoph Hellwig, Paul Mackerras, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Mon, 25 Oct 2004 17:55:50 -0500, Doug Maxey <dwm@austin.ibm.com> wrote:
> >The dependency is a bug, <linux/ide.h> is for IDE driver only.
>
> The isd200 _is_ a bridge to ATA/ATAPI devices. Does this mean it cannot use
> common code, just because it is not in drivers/ide?
no but the common ATA/ATAPI code resides in hdreg.h or/and ata.h,
ide.h is for IDE driver _only_
> >Doug, if you kill debugging code in isd200.c then only:
> >
> >id->command_set_1
> >id->model
> >id->fw_rev
> >id->capability
> >id->lba_capacity
> >id->heads
> >id->cyls
> >id->sectors
> >id->command_set_2
> >
> >need to be byte-swapped.
> >
>
> I don't plan on killing any debug code.
I do :)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-25 23:12 ` Paul Mackerras
@ 2004-10-25 23:55 ` Doug Maxey
2004-10-26 2:05 ` Paul Mackerras
0 siblings, 1 reply; 10+ messages in thread
From: Doug Maxey @ 2004-10-25 23:55 UTC (permalink / raw)
To: Paul Mackerras
Cc: Bartlomiej Zolnierkiewicz, Christoph Hellwig, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Tue, 26 Oct 2004 09:12:28 +1000, Paul Mackerras wrote:
>Doug Maxey writes:
>
>> Ok, lets look at those vars. Both are defined in hdreg.h as bytes.
>> No fields in the data from the device are bytes, but are 16 bit. On big
>> endian, the relative positions for an LE u16 are swapped. If the swap is
>> not done on those, then one replaces the other when read. Probably not
>> what was intended. It appears that another bug is being fixed here.
>
>No. The only sane way to do things is to transfer data from the
>device to memory as a byte stream, in other words, preserving the
>ordering of the individual bytes. That is what we do on PPC and PPC64
>platforms. That ordering is preserved (and must be preserved)
>irrespective of whether the transfer is actually done in 8, 16 or 32
>bit chunks.
Oh yes, I am aware. Just happen to be working on PPC64. Have been
writing drivers for this base for several years. It's the olde LE
device vs BE host. The transfers are done as a 16 bit quantity, PIO.
And yes, I understand, "we have always done it this way". Works well
when you only have to deal with single arch.
Possibly I am not making point very well, that one is preserving the
correct byte order and let the structures reflect to native location.
Strings get swapped, 16, 32, and 64 bit fields likewise. I just missed the
LE order that is is being preserved for *some* few fields only.
>
>That means that 16-bit quantities might need to be byte-swapped to be
>interpreted in host byte order, but single-byte fields should always
>be in their correct sequence.
There is not a single reference to byte field in the ATA spec for
IDENTIFY DEVICE. It just happens that some of the fields are 8 bits long. Or
32 or 64.
>
>Paul.
>
++doug
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-25 23:55 ` Doug Maxey
@ 2004-10-26 2:05 ` Paul Mackerras
2004-10-26 18:05 ` Doug Maxey
0 siblings, 1 reply; 10+ messages in thread
From: Paul Mackerras @ 2004-10-26 2:05 UTC (permalink / raw)
To: Doug Maxey
Cc: Bartlomiej Zolnierkiewicz, Christoph Hellwig, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
Doug Maxey writes:
> Oh yes, I am aware. Just happen to be working on PPC64. Have been
> writing drivers for this base for several years. It's the olde LE
For Linux or some other OS?
> device vs BE host. The transfers are done as a 16 bit quantity, PIO.
> And yes, I understand, "we have always done it this way". Works well
> when you only have to deal with single arch.
No, we haven't always done it this way on PPC. :) Various different
ways have been tried over the years and this is the only way that
doesn't suck.
> Possibly I am not making point very well, that one is preserving the
> correct byte order and let the structures reflect to native location.
I can't parse that sentence unambiguously...
> Strings get swapped, 16, 32, and 64 bit fields likewise. I just missed the
> LE order that is is being preserved for *some* few fields only.
Strings shouldn't get swapped, or at least, strings should only need
to be swapped on a BE platform if they also need to be swapped on an
LE platform.
> There is not a single reference to byte field in the ATA spec for
> IDENTIFY DEVICE. It just happens that some of the fields are 8 bits long. Or
> 32 or 64.
And your point is... ?
Paul.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] build modular usb isd200 with modular ide
2004-10-26 2:05 ` Paul Mackerras
@ 2004-10-26 18:05 ` Doug Maxey
0 siblings, 0 replies; 10+ messages in thread
From: Doug Maxey @ 2004-10-26 18:05 UTC (permalink / raw)
To: Paul Mackerras
Cc: Bartlomiej Zolnierkiewicz, Christoph Hellwig, Greg Kroah-Hartman,
Linux SCSI Mailing List, Linux IDE Mailing List, linuxppc64-dev
On Tue, 26 Oct 2004 12:05:29 +1000, Paul Mackerras wrote:
>For Linux or some other OS?
Linux for little over a year, 2.6 for about 3 months, some _other_ OS for
several years.
>
>> device vs BE host. The transfers are done as a 16 bit quantity, PIO.
>> And yes, I understand, "we have always done it this way". Works well
>> when you only have to deal with single arch.
>
>No, we haven't always done it this way on PPC. :) Various different
>ways have been tried over the years and this is the only way that
>doesn't suck.
>
>> Possibly I am not making point very well, that one is preserving the
>> correct byte order and let the structures reflect to native location.
>
>I can't parse that sentence unambiguously...
s/to native location/the normalized (for the host) layout/
>
>> Strings get swapped, 16, 32, and 64 bit fields likewise. I just missed the
>> LE order that is is being preserved for *some* few fields only.
>
>Strings shouldn't get swapped, or at least, strings should only need
>to be swapped on a BE platform if they also need to be swapped on an
>LE platform.
>
>> There is not a single reference to byte field in the ATA spec for
>> IDENTIFY DEVICE. It just happens that some of the fields are 8 bits long. Or
>> 32 or 64.
>
>And your point is... ?
To me, and I do seem to be in the minority, it seems that normalizing the
entire bytestream is the right thing (tm). But I can see the point that
leaving certain parts non-normalized is cheaper.
It was my mistake missing the use of the char fields. GIITD. In any
event, with 2.6.10-rc1 the problem seems to be solved in spite of my
meddling. :-)
++doug
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2004-10-26 18:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-10-24 0:55 [PATCH 1/1] build modular usb isd200 with modular ide Doug Maxey
2004-10-24 10:03 ` Christoph Hellwig
2004-10-24 12:45 ` Bartlomiej Zolnierkiewicz
2004-10-25 22:55 ` Doug Maxey
2004-10-25 23:12 ` Paul Mackerras
2004-10-25 23:55 ` Doug Maxey
2004-10-26 2:05 ` Paul Mackerras
2004-10-26 18:05 ` Doug Maxey
2004-10-25 23:20 ` Bartlomiej Zolnierkiewicz
2004-10-24 23:11 ` Doug Maxey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).