linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).