Re: [PATCH] M68k IDE updates

Geert Uytterhoeven (geert@linux-m68k.org)
Mon, 14 Apr 2003 21:54:58 +0200 (MEST)


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 -- geert@linux-m68k.org

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

- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/