Re: [PATCH] M68k IDE updates

[Geert Uytterhoeven]

On Mon, 14 Apr 2003, Linus Torvalds wrote:
> On Mon, 14 Apr 2003, Paul Mackerras wrote:
> > Since __ide_mm_insw doesn't get told whether it is transferring normal
> > sector data or drive ID data, it can't necessarily do the right thing
> > in both situations.
> 
> Can we please then just separate the two functions out into "fetch sector
> data" and "fetch drive ID"? And NOT playing with another frigging broken
> passed-down flag that people get wrong and isn't obvious what it does
> anyway? It's a lot easier to do
> 
> 	/* On sane architectures, data and ID are accessed the same */
> 	#define ide_fetch_sector_data(...) __ide_fetch_data(..)
> 	#define ide_fetch_id_data(...) __ide_fetch_data(..)
> 
> than it is to carry a flag around and having to remember to get it right 
> in every place this is used.
> 
> It's more efficient too, but the _clarity_ and lack of dynamic flags is a 
> hell of a lot more important.
> 
> And stupid architectures that may have to re-implement (and possible
> duplicate) the ID fetch code only have themselves to blame. Although it 
> might easily be as simple as
> 
> 	/*
> 	 * The PCI bus is wired up the wrong way, we need to byteswap
> 	 * the ID results after they come back
> 	 */
> 	static inline xxx ide_fetch_id_data(...)
> 	{
> 		__ide_fetch_data(..)
> 		bswap_id_data(..)
> 	}
> 
> and please keep this in some m68k-specific file instead of forcing 
> _everybody_ to know about the braindamage.

I think the least-intrusive solution is something like this:

--- linux-2.5/drivers/ide/ide-iops.c.orig	Mon Apr 14 21:43:30 2003
+++ linux-2.5/drivers/ide/ide-iops.c	Mon Apr 14 21:44:53 2003
@@ -423,8 +423,7 @@
  */
 void ide_fix_driveid (struct hd_driveid *id)
 {
-#ifndef __LITTLE_ENDIAN
-# ifdef __BIG_ENDIAN
+    if (ide_driveid_needs_swapping(id)) {
 	int i;
 	u16 *stringcast;
 
@@ -512,10 +511,7 @@
 	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);

Where ide_driveid_needs_swapping() is #define'd to return 0 (never swap), 1
(always swap), or whatever architecture-specific logic you need.

We can even have defaults in <linux/ide.h>

    #ifndef ide_driveid_needs_swapping
    # ifdef __LITTLE_ENDIAN
    #  define ide_driveid_needs_swapping(id)	0
    # else
    #  ifdef __BIG_ENDIAN
    #   define ide_driveid_needs_swapping(id)	1
    #  else
    #   error "Please fix <asm/byteorder.h>"
    #  endif
    # endif
    #endif

Sounds sufficiently sane?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds