Re: Big Fix for 2.2.1

Gerard Roudier (groudier@club-internet.fr)
Tue, 26 Jan 1999 23:02:30 +0100 (MET)


I suggest to get rid of the bogus readX/writeX stuff in drivers that
know what they are doing, until 2.3, as follows:

#if defined(__i386__) /* i386 implements full FLAT memory/MMIO model */

#undef the stuff that does not work for __i386__ with PCI MMIO regions
mapped in high address space and ioremapped, then:

#define readb(a) (*(volatile unsigned char *) (a))
#define readw(a) (*(volatile unsigned short *) (a))
#define readl(a) (*(volatile unsigned int *) (a))
#define writeb(b,a) ((*(volatile unsigned char *) (a)) = (b))
#define writew(b,a) ((*(volatile unsigned short *) (a)) = (b))
#define writel(b,a) ((*(volatile unsigned int *) (a)) = (b))

#else etc ...

And obviously to _not_ use phys_to_virt() that seems to only make sense
on a single O/S.

BTW, already done in the development 896 driver.

Gerard.

On Tue, 26 Jan 1999, Linus Torvalds wrote:

>
>
> On Tue, 26 Jan 1999, Leonard N. Zubkoff wrote:
> >
> > (1) The identity phys_to_virt(virt_to_phys(x)) == x is violated.
>
> Not for the default setup. Guess why the default setup is the default
> setup?
>
> > (2) The readb/writeb/readw/writew/readl/writel macros are completely incorrect
> > since they are supposed to be applied to memory mapped I/O regions
> > allocated with ioremap, but ioremap returns a kernel virtual address
> > and hence no further mapping is appropriate.
>
> We also currently accept things like
>
> readb(0xA0000);
>
> ie the "legacy ISA region" is currently (for backwards compatibility
> reasons) not required to be io-remapped.
>
> Look at various network drivers for example - your patches probably break
> a noticeable number of them.
>
> > The BusLogic driver ran into (1) and the DAC960 driver ran into (2). With the
> > patch enclosed below, both drivers work fine when PAGE_OFFSET is changed.
>
> With the patch enclosed below, tons of random device drivers may break.
> Not something to be done lightly.
>
> > The
> > change to traps.c is also necessary as readl was incorrect usage, even though
> > the modified code is now uglier.
>
> The modified code in your patch is completely wrong, and is MUCH more
> incorrect than the old one.
>
> The old one made the assumption that the legacy ISA region doesn't need to
> be IO-remapped and can be directly accessed with read[bwl]/write[bwl] -
> which is a painful assumption, but it's a legacy one, and breaking that
> assumption means that _tons_ of drivers will have to be checked.
>
> The new code makes the assumption that the legacy ISA region is mapped in
> the kernel virtual address space, and that phys_to_virt() has any meaning
> for IO addresses. That happens to be the case right now, but it's a
> completely invalid assumption that does not work on many other
> architectures.
>
> So the old code was at least technically correct. The new one isn't.
>
> In short, I will accept this kind of patch for 2.3 (fixed up properly),
> but I hope you see why I would never consider it for 2.2.x.
>
> The _proper_ fixup is to make sure that every access to IO memory is
> preceded by a "ioremap()" to make sure the address is mapped. The
> "phys_to_virt()" function has absolutely nothing at all to do with IO
> memory, and anything that uses it is buggy.
>
> NOW do you understand why I use binary and/or and force the power-of-two
> rule?
>
> Linus
>
> -
> Linux SMP list: FIRST see FAQ at http://www.irisa.fr/prive/mentre/smp-faq/
> To Unsubscribe: send "unsubscribe linux-smp" to majordomo@vger.rutgers.edu
>
>

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