PATCH: clean up the IDE iops, add ones for a dead iface

PATCH: clean up the IDE iops, add ones for a dead iface

[Alan Cox]

Also adds the new OUTBSYNC iop

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux-2.5.61/drivers/ide/ide-iops.c linux-2.5.61-ac2/drivers/ide/ide-iops.c
--- linux-2.5.61/drivers/ide/ide-iops.c	2003-02-10 18:38:18.000000000 +0000
+++ linux-2.5.61-ac2/drivers/ide/ide-iops.c	2003-02-18 18:06:17.000000000 +0000
@@ -31,60 +31,129 @@
 #include <asm/io.h>
 #include <asm/bitops.h>
 
+/*
+ *	IDE operator we assign to an unplugged device so that
+ *	we don't trash new hardware assigned the same resources
+ */
+ 
+static u8 ide_unplugged_inb (unsigned long port)
+{
+	return 0xFF;
+}
+
+static u16 ide_unplugged_inw (unsigned long port)
+{
+	return 0xFFFF;
+}
+
+static void ide_unplugged_insw (unsigned long port, void *addr, u32 count)
+{
+}
+
+static u32 ide_unplugged_inl (unsigned long port)
+{
+	return 0xFFFFFFFF;
+}
+
+static void ide_unplugged_insl (unsigned long port, void *addr, u32 count)
+{
+}
+
+static void ide_unplugged_outb (u8 addr, unsigned long port)
+{
+}
+
+static void ide_unplugged_outw (u16 addr, unsigned long port)
+{
+}
+
+static void ide_unplugged_outsw (unsigned long port, void *addr, u32 count)
+{
+}
+
+static void ide_unplugged_outl (u32 addr, unsigned long port)
+{
+}
+
+static void ide_unplugged_outsl (unsigned long port, void *addr, u32 count)
+{
+}
 
-static u8 ide_inb (ide_ioreg_t port)
+void unplugged_hwif_iops (ide_hwif_t *hwif)
+{
+	hwif->OUTB	= ide_unplugged_outb;
+	hwif->OUTBSYNC	= ide_unplugged_outb;
+	hwif->OUTW	= ide_unplugged_outw;
+	hwif->OUTL	= ide_unplugged_outl;
+	hwif->OUTSW	= ide_unplugged_outsw;
+	hwif->OUTSL	= ide_unplugged_outsl;
+	hwif->INB	= ide_unplugged_inb;
+	hwif->INW	= ide_unplugged_inw;
+	hwif->INL	= ide_unplugged_inl;
+	hwif->INSW	= ide_unplugged_insw;
+	hwif->INSL	= ide_unplugged_insl;
+}
+
+EXPORT_SYMBOL(unplugged_hwif_iops);
+
+/*
+ *	Conventional PIO operations for ATA devices
+ */
+
+static u8 ide_inb (unsigned long port)
 {
 	return (u8) inb(port);
 }
 
-static u16 ide_inw (ide_ioreg_t port)
+static u16 ide_inw (unsigned long port)
 {
 	return (u16) inw(port);
 }
 
-static void ide_insw (ide_ioreg_t port, void *addr, u32 count)
+static void ide_insw (unsigned long port, void *addr, u32 count)
 {
 	return insw(port, addr, count);
 }
 
-static u32 ide_inl (ide_ioreg_t port)
+static u32 ide_inl (unsigned long port)
 {
 	return (u32) inl(port);
 }
 
-static void ide_insl (ide_ioreg_t port, void *addr, u32 count)
+static void ide_insl (unsigned long port, void *addr, u32 count)
 {
 	insl(port, addr, count);
 }
 
-static void ide_outb (u8 value, ide_ioreg_t port)
+static void ide_outb (u8 addr, unsigned long port)
 {
-	outb(value, port);
+	outb(addr, port);
 }
 
-static void ide_outw (u16 value, ide_ioreg_t port)
+static void ide_outw (u16 addr, unsigned long port)
 {
-	outw(value, port);
+	outw(addr, port);
 }
 
-static void ide_outsw (ide_ioreg_t port, void *addr, u32 count)
+static void ide_outsw (unsigned long port, void *addr, u32 count)
 {
 	outsw(port, addr, count);
 }
 
-static void ide_outl (u32 value, ide_ioreg_t port)
+static void ide_outl (u32 addr, unsigned long port)
 {
-	outl(value, port);
+	outl(addr, port);
 }
 
-static void ide_outsl (ide_ioreg_t port, void *addr, u32 count)
+static void ide_outsl (unsigned long port, void *addr, u32 count)
 {
-	outsl(port, addr, count);
+	return outsl(port, addr, count);
 }
 
 void default_hwif_iops (ide_hwif_t *hwif)
 {
 	hwif->OUTB	= ide_outb;
+	hwif->OUTBSYNC	= ide_outb;
 	hwif->OUTW	= ide_outw;
 	hwif->OUTL	= ide_outl;
 	hwif->OUTSW	= ide_outsw;
@@ -98,78 +167,66 @@
 
 EXPORT_SYMBOL(default_hwif_iops);
 
-static u8 ide_mm_inb (ide_ioreg_t port)
+/*
+ *	MMIO operations, typically used for SATA controllers
+ */
+
+static u8 ide_mm_inb (unsigned long port)
 {
 	return (u8) readb(port);
 }
 
-static u16 ide_mm_inw (ide_ioreg_t port)
+static u16 ide_mm_inw (unsigned long port)
 {
 	return (u16) readw(port);
 }
 
-static void ide_mm_insw (ide_ioreg_t port, void *addr, u32 count)
+static void ide_mm_insw (unsigned long port, void *addr, u32 count)
 {
-#ifdef CONFIG_PPC
-	/* Can we move the barrier out of the loop ? */
-	while (count--) { *(u16 *)addr = __raw_readw(port); iobarrier_r(); addr += 2; }
-#else /* everything else is sane benh */
-	while (count--) { *(u16 *)addr = readw(port); addr += 2; }
-#endif
+	__ide_mm_insw(port, addr, count);
 }
 
-static u32 ide_mm_inl (ide_ioreg_t port)
+static u32 ide_mm_inl (unsigned long port)
 {
 	return (u32) readl(port);
 }
 
-static void ide_mm_insl (ide_ioreg_t port, void *addr, u32 count)
+static void ide_mm_insl (unsigned long port, void *addr, u32 count)
 {
-#ifdef CONFIG_PPC
-	/* Can we move the barrier out of the loop ? */
-	while (count--) { *(u32 *)addr = __raw_readl(port); iobarrier_r(); addr += 4; }
-#else /* everything else is sane benh */
-	while (count--) { *(u32 *)addr = readl(port); addr += 4; }
-#endif
+	__ide_mm_insl(port, addr, count);
 }
 
-static void ide_mm_outb (u8 value, ide_ioreg_t port)
+static void ide_mm_outb (u8 value, unsigned long port)
 {
 	writeb(value, port);
 }
 
-static void ide_mm_outw (u16 value, ide_ioreg_t port)
+static void ide_mm_outw (u16 value, unsigned long port)
 {
 	writew(value, port);
 }
 
-static void ide_mm_outsw (ide_ioreg_t port, void *addr, u32 count)
+static void ide_mm_outsw (unsigned long port, void *addr, u32 count)
 {
-#ifdef CONFIG_PPC
-	/* Can we move the barrier out of the loop ? */
-	while (count--) { __raw_writew(*(u16 *)addr, port); iobarrier_w(); addr += 2; }
-#else /* everything else is sane benh */
-	while (count--) { writew(*(u16 *)addr, port); addr += 2; }
-#endif
+	__ide_mm_outsw(port, addr, count);
 }
 
-static void ide_mm_outl (u32 value, ide_ioreg_t port)
+static void ide_mm_outl (u32 value, unsigned long port)
 {
 	writel(value, port);
 }
 
-static void ide_mm_outsl (ide_ioreg_t port, void *addr, u32 count)
+static void ide_mm_outsl (unsigned long port, void *addr, u32 count)
 {
-#ifdef CONFIG_PPC
-	while (count--) { __raw_writel(*(u32 *)addr, port); iobarrier_w(); addr += 4; }
-#else /* everything else is sane benh */
-	while (count--) { writel(*(u32 *)addr, port); addr += 4; }
-#endif
+	__ide_mm_outsl(port, addr, count);
 }
 
 void default_hwif_mmiops (ide_hwif_t *hwif)
 {
 	hwif->OUTB	= ide_mm_outb;
+	/* Most systems will need to override OUTBSYNC, alas however
+	   this one is controller specific! */
+	hwif->OUTBSYNC	= ide_mm_outb;
 	hwif->OUTW	= ide_mm_outw;
 	hwif->OUTL	= ide_mm_outl;
 	hwif->OUTSW	= ide_mm_outsw;

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Russell King]

On Tue, Feb 18, 2003 at 06:03:21PM +0000, Alan Cox wrote:
> +static u8 ide_unplugged_inb (unsigned long port)
> +{
> +	return 0xFF;
> +}

Shouldn't this return 0x7f, ie bit 7 clear, as if we have an interface
without drive attached?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Alan Cox]

On Tue, 2003-02-18 at 23:09, Russell King wrote:
> On Tue, Feb 18, 2003 at 06:03:21PM +0000, Alan Cox wrote:
> > +static u8 ide_unplugged_inb (unsigned long port)
> > +{
> > +	return 0xFF;
> > +}
> 
> Shouldn't this return 0x7f, ie bit 7 clear, as if we have an interface
> without drive attached?

0xFF is what most hardware returns for an unclaimed pci address. I'd rather
make sure it also works when we don't get the event, or the device 
summarily explodes than fake up something to make it look good. If
0xff doesnt work we have a bug anyway, lets find them.

Alan

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Benjamin Herrenschmidt]

On Wed, 2003-02-19 at 02:53, Alan Cox wrote:
> On Tue, 2003-02-18 at 23:09, Russell King wrote:
> > On Tue, Feb 18, 2003 at 06:03:21PM +0000, Alan Cox wrote:
> > > +static u8 ide_unplugged_inb (unsigned long port)
> > > +{
> > > +	return 0xFF;
> > > +}
> > 
> > Shouldn't this return 0x7f, ie bit 7 clear, as if we have an interface
> > without drive attached?
> 
> 0xFF is what most hardware returns for an unclaimed pci address. I'd rather
> make sure it also works when we don't get the event, or the device 
> summarily explodes than fake up something to make it look good. If
> 0xff doesnt work we have a bug anyway, lets find them.

Hrm... I tend to agree with Russell here... 0x7f is the "safe" value
for IDE. IDE controllers with nothing wired shall have a pull down
on D7. The reason is simple: busy loops in the IDE code waiting for
BSY to go down.

Now, if your point is to keep BSY up and have wait loops timeout,
then 0xff may actually make some sense ;)

Ben.

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Alan Cox]

On Wed, 2003-02-19 at 09:39, Benjamin Herrenschmidt wrote:
> Hrm... I tend to agree with Russell here... 0x7f is the "safe" value
> for IDE. IDE controllers with nothing wired shall have a pull down
> on D7. The reason is simple: busy loops in the IDE code waiting for
> BSY to go down.
> 
> Now, if your point is to keep BSY up and have wait loops timeout,
> then 0xff may actually make some sense ;)

0xFF is what happens with a real hot unplug so we must handle it
irrespective of convenience. Your probe loop might want to check
some kind of 'dead' variable when this is all done I guess. That
way we can have

	unplugged_event()
	{
		change iops
		drive->dead = 1;
	}

and the probe code can spot ->dead in the poll loop

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Linus Torvalds]

On 19 Feb 2003, Benjamin Herrenschmidt wrote:
> 
> Hrm... I tend to agree with Russell here... 0x7f is the "safe" value
> for IDE. IDE controllers with nothing wired shall have a pull down
> on D7. The reason is simple: busy loops in the IDE code waiting for
> BSY to go down.

But that's a BUG.

We've seen that before: try unplugging a PCMCIA IDE card unexpectedly. 

Guess what? It will start returning 0xff. And the machine dies, because 
the PCMCIA interrupt happened due to the removal event will also be shared 
by the IDE driver, so the IDE driver will react badly even before anybody 
has had a chance to tell it that the hardware no longer exists.

So if you have code that doesn't work with 0xff, then that code is already 
a-priori buggy. And getting it fixed would be a damn good idea.

		Linus

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Benjamin Herrenschmidt]

On Wed, 2003-02-19 at 17:56, Linus Torvalds wrote:

> We've seen that before: try unplugging a PCMCIA IDE card unexpectedly. 
> 
> Guess what? It will start returning 0xff. And the machine dies, because 
> the PCMCIA interrupt happened due to the removal event will also be shared 
> by the IDE driver, so the IDE driver will react badly even before anybody 
> has had a chance to tell it that the hardware no longer exists.
> 
> So if you have code that doesn't work with 0xff, then that code is already 
> a-priori buggy. And getting it fixed would be a damn good idea.

Yup, you are right. Removing a disk from a controller shall return
anything with bit 7 at 0 per spec, but removing the controller
itself will return 0xff. Actually, in my "wait for BSY low" loop
I added to the probe code for pmac (should be made generic sooner
or later), I did special case 0xff.

So we should indeed fix the various bits in IDE. 0xff out of
status, I beleive, never means anything and can always be considered
as "this interface is gone".
 
Ben.

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Alan Cox]

On Wed, 2003-02-19 at 17:06, Benjamin Herrenschmidt wrote:
> Yup, you are right. Removing a disk from a controller shall return
> anything with bit 7 at 0 per spec, but removing the controller
> itself will return 0xff. Actually, in my "wait for BSY low" loop
> I added to the probe code for pmac (should be made generic sooner
> or later), I did special case 0xff.
> 
> So we should indeed fix the various bits in IDE. 0xff out of
> status, I beleive, never means anything and can always be considered
> as "this interface is gone".

I think thats the wrong approach too. We need to be defensive on things
like IDE probes. We just have to be sure that we -do- eventually say
'its bust', and when we know from hotplug a channel has vanished also
be sure to check the 'its dead jim' flag once I add it

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Benjamin Herrenschmidt]

On Wed, 2003-02-19 at 19:22, Alan Cox wrote:
> On Wed, 2003-02-19 at 17:06, Benjamin Herrenschmidt wrote:
> > Yup, you are right. Removing a disk from a controller shall return
> > anything with bit 7 at 0 per spec, but removing the controller
> > itself will return 0xff. Actually, in my "wait for BSY low" loop
> > I added to the probe code for pmac (should be made generic sooner
> > or later), I did special case 0xff.
> > 
> > So we should indeed fix the various bits in IDE. 0xff out of
> > status, I beleive, never means anything and can always be considered
> > as "this interface is gone".
> 
> I think thats the wrong approach too. We need to be defensive on things
> like IDE probes. We just have to be sure that we -do- eventually say
> 'its bust', and when we know from hotplug a channel has vanished also
> be sure to check the 'its dead jim' flag once I add it

Ok, then let's make sure we have no endless loops caused by BSY
returning 1 and _not_ checking the "dead" flag. I'm sure I can find
some of these in a couple of places (like when setting the PIO/DMA
mode).

Ben.

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[David Woodhouse]

On Wed, 2003-02-19 at 16:56, Linus Torvalds wrote:
> On 19 Feb 2003, Benjamin Herrenschmidt wrote:
> > Hrm... I tend to agree with Russell here... 0x7f is the "safe" value
> > for IDE. IDE controllers with nothing wired shall have a pull down
> > on D7. The reason is simple: busy loops in the IDE code waiting for
> > BSY to go down.
> 
> But that's a BUG.
> 
> We've seen that before: try unplugging a PCMCIA IDE card unexpectedly. 
> 
> Guess what? It will start returning 0xff. And the machine dies, because 
> the PCMCIA interrupt happened due to the removal event will also be shared 
> by the IDE driver, so the IDE driver will react badly even before anybody 
> has had a chance to tell it that the hardware no longer exists.

Er, actually they make the card detect pins a different length for a
reason -- not just so they look pretty :) 

You are guaranteed a certain amount of time between the card detect
firing to signal removal, and the actual disconnection of the device.

And you can't rely on getting 0xFF; sometimes your box just locks up
hard if you try to access the missing hardware any time after the
'warning period' has expired and the device has actually gone.

Admittedly that's a fairly screwed box -- but I have them and was
expected to make them userproof.

-- 
dwmw2

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Geert Uytterhoeven]

On Tue, 18 Feb 2003, Alan Cox wrote:
> -static void ide_outb (u8 value, ide_ioreg_t port)
> +static void ide_outb (u8 addr, unsigned long port)

> -static void ide_outw (u16 value, ide_ioreg_t port)
> +static void ide_outw (u16 addr, unsigned long port)

> -static void ide_outl (u32 value, ide_ioreg_t port)
> +static void ide_outl (u32 addr, unsigned long port)

The first parameter should be called `value', not `addr'.

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

Re: PATCH: clean up the IDE iops, add ones for a dead iface

[Alan Cox]

On Thu, 2003-02-20 at 13:16, Geert Uytterhoeven wrote:
> On Tue, 18 Feb 2003, Alan Cox wrote:
> > -static void ide_outb (u8 value, ide_ioreg_t port)
> > +static void ide_outb (u8 addr, unsigned long port)
> 
> > -static void ide_outw (u16 value, ide_ioreg_t port)
> > +static void ide_outw (u16 addr, unsigned long port)
> 
> > -static void ide_outl (u32 value, ide_ioreg_t port)
> > +static void ide_outl (u32 addr, unsigned long port)
> 
> The first parameter should be called `value', not `addr'.


Yep. That change missed the base 2.4 tree so it rippled back
into 2.5. I'll fix it up at some point