Reserving an ATA interface

Hi Bart, Alan !

I've been fighting with a problem with both 2.4 and 2.5 for which I would have a workaround, but I'd like to have your point of view on it first as this code is rather messy.

So the problem is all about deciding if an hwif slot is 'owned' by a driver, even after ide_unregister is called, after probe, etc... and problems with the way the current scheme works (and differences between the way PCI interfaces "pick" free slots vs. ide_register_hw does)

So, let's first explain the various cases I have to deal with in the context of ide/ppc/pmac.c.

1 - Normal case: interface is non hotswap, drive is present at startup, things just work normally

2 - Empty interface: at boot, en empty interface (no drive) is found. This happens a lot since Apple's ASIC often have at least one spare ATA/33 interface that isn't wired to anything. This hwif is filled up for ide/ppc/pmac (MMIO iops, chipset = ide_pmac, etc...). hwif->present is cleared by the common probe code

3 - Hotswap interface: This one may or may not have devices plugged at boot (laptop media bay). The hwif is filled with MMIO iops etc... but since it may not have the CD drive plugged at boot, hwif->present may be cleared by the common probe code.

So now, the problems:

- In both case 2 and 3, hwif->present may be cleared. That means
that something calling ide_register_hw() (like ide-cs for example) will eventually pick up that hwif slot and try to use it. However the hwif beeing filled for ide/ppc/pmac.c (MMIO iops among other) it will usually not work and just crash, since ide_register_hw() will _not_ reinit the fields. Also, in the case of the hotswap interface (3), It's simply not acceptable for the slot to be re-used (while it is for case 2) since you don't want the CD-ROM location to change, and because ide-pmac will have internally setup a data structure for this hwif that is tied to a given index that was obtained at boot.

So that leads to 2 actual needs: One, to deal with case 2 & re-use of the hwif by ide-cs (among others) is to have ide_register_hw actually clear the interface calling init_hwif_data() at some point when picking a "new" slot. The other one is to be able to "mark" an interface as "held" by the driver (hotswap bay that don't want to change numbering).

The simplest solution I have in mind is to add an hwif flag, called "hold" (or whatever better name you find). Drivers like ide/ppc/pmac.c would set this flag for the "hotswap" media bay interface, and not for others.

The only change to the core code would then be for ide_register_hw to 'skip' those when searching for an available slot, and to call init_hwif_data when (!hwif->present && !hwif->hold) to handle case 2 where the iops & other hwif fields (mmio among others) need to be reset to initial/legacy state.

That would allow "empty" fixed pmac interfaces to be released for use by ide-cs, while the hotswap media bay one stay reserved.

That would give us a patch like that:

===== include/linux/ide.h 1.47 vs edited =====

include/linux/ide.h +1 -0

@@ -1022,6 +1022,7 @@
 
 	unsigned	noprobe    : 1;	/* don't probe for this interface */
 	unsigned	present    : 1;	/* this interface exists */
+	unsigned	hold       : 1; /* this interface is always present */
 	unsigned	serialized : 1;	/* serialized all channel operation */
 	unsigned	sharing_irq: 1;	/* 1 = sharing irq with another hwif */
 	unsigned	reset      : 1;	/* reset after probe */

===== drivers/ide/ide.c 1.45 vs edited =====

drivers/ide/ide.c +4 -2

@@ -920,8 +920,8 @@
 		}
 		for (index = 0; index < MAX_HWIFS; ++index) {
 			hwif = &ide_hwifs[index];
-			if ((!hwif->present && !hwif->mate && !initializing) ||
-			    (!hwif->hw.io_ports[IDE_DATA_OFFSET] && initializing))
+			if (!hwif->hold && ((!hwif->present && !hwif->mate && !initializing) ||
+			    (!hwif->hw.io_ports[IDE_DATA_OFFSET] && initializing)))
 				goto found;
 		}
 		for (index = 0; index < MAX_HWIFS; index++)

@@ -931,6 +931,8 @@
 found:
 	if (hwif->present)
 		ide_unregister(index);
+	else if (!hwif->hold)
+		init_hwif_data(index);
 	if (hwif->present)
 		return -1;
 	memcpy(&hwif->hw, hw, sizeof(*hw));

Yesterday I was thinking about:
- default arch hwifs, added/probed in ide_init_defult_hwifs() if no IDE PCI/PCI (depends on arch, should be IDE PCI?) support is compiled in
- PCI hwifs
- legacy hwifs probed in ide_setup()
- legacy hwifs probed after PCI
and about ide_register_hw() + initializing flag.

What a mess... ordering issues can make you crazy.

Quoted 4 lines

The simplest solution I have in mind is to add an hwif flag, called "hold" (or whatever better name you find). Drivers like ide/ppc/pmac.c would set this flag for the "hotswap" media bay interface, and not for others.

This change is obviously correct and it doesn't have influence on any existing code.

Quoted 5 lines

The only change to the core code would then be for ide_register_hw to 'skip' those when searching for an available slot, and to call init_hwif_data when (!hwif->present && !hwif->hold) to handle case 2 where the iops & other hwif fields (mmio among others) need to be reset to initial/legacy state.

Less safe change but also okay, as callers .
btw, Can't "ghost ides" be dealed inside ppc specific code?
Do you know when interface is valid and when it is "ghost", and what other OS-es do in this case?

Such an interface can't really know what slot will be picked by ide_register_hw() and can't "prepare" the HWIF with special iops, so it won't be much harmed by the fact we are calling init_hwif_data, but still, we should ultimately think about splitting completely the fact of allocating an hwif slot, setting it up, and triggering a probe on it. Those are 3 different things that are currently mixed in bad ways. I don't beleive fixing that fits in the 2.6 timeframe though.

Quoted 8 lines

Such an interface can't really know what slot will be picked by ide_register_hw() and can't "prepare" the HWIF with special iops, so it won't be much harmed by the fact we are calling init_hwif_data, but still, we should ultimately think about splitting completely the fact of allocating an hwif slot, setting it up, and triggering a probe on it. Those are 3 different things that are currently mixed in bad ways. I don't beleive fixing that fits in the 2.6 timeframe though.

I just though about another possible crap, though I haven't looked enough to be sure, but PCI interfaces with no device will trigger a similar problem as "empty" ide-pmac interfaces in that sense that they will change dma ops, possibly mmio ops, etc.... If they hold no device, their hwif->present will not be set, and thus the hwif slot can possibly get re-used by thing like ide-cs (or anybody else that rely on ide_register_hw() to allocate a new slot) without those changes to hwif done by the PCI interface beeing cleared.

So my patch may actually fix some cases there too.

Ben.

Quoted 2 lines

So my patch may actually fix some cases there too.

No, look at ide_match_hwif() in setup-pci.c .
PCI grabs only ide_unknown interfaces.

btw, I think the only real long-term solution for all ordering issues
is customizable device mapper... 2.7?

Quoted 4 lines · Bartlomiej Zolnierkiewicz

Quoted 1 line

So my patch may actually fix some cases there too.

No, look at ide_match_hwif() in setup-pci.c .
PCI grabs only ide_unknown interfaces.

No, you missed my point.

1) setup-pci claims a free hwif slot
2) the driver sets some custom IOPs (MMIO PCI interface for example) and/or does other tweaks to hwif
3) no device is attached to this interface, the IDE probe code leaves hwif->present to 0, but the hwif fields (IOps etc... are still set by the PCI driver)
4) later on, ide-cs gets in, and picks that slot since hwif->present is 0 and ide-cs doesn't care about chipset. However, those IOps fields (and possibly other, DMA stuff etc...) are still those of the PCI interface. If the PCI interface set it to MMIO for example, boom !

Note that I haven't actually tested the above scenario as I don't have a box with such PCI IDE interfaces, but it seems the problem I have with ide-pmac in this case is identical.

With my patch, since the PCI interface will not set the "hold" flag, ide_register_hw() called by ide-cs will call init_hwif_data(), thus putting back the hwif to a sane state

Quoted 2 lines

btw, I think the only real long-term solution for all ordering issues
is customizable device mapper... 2.7?

Or not relying on /dev/hdX entries for mounting ? (disk UUID etc..) ;)

Quoted 4 lines

With my patch, since the PCI interface will not set the "hold" flag, ide_register_hw() called by ide-cs will call init_hwif_data(), thus putting back the hwif to a sane state

So every time you remove your disks from one of your PCI IDE controllers, your ide-cs will get diffirent hwif and drives mappings and your RAID on ide-cs won't be recognized ;-)

Quoted 9 lines

Yesterday I was thinking about:
- default arch hwifs, added/probed in ide_init_defult_hwifs() if no IDE PCI/PCI (depends on arch, should be IDE PCI?) support is compiled in
- PCI hwifs
- legacy hwifs probed in ide_setup()
- legacy hwifs probed after PCI
and about ide_register_hw() + initializing flag.

Quoted 1 line

What a mess... ordering issues can make you crazy.

Yup, I'd suggest we think about re-writing all that stuff for 2.7 :)

Quoted 14 lines

Quoted 4 lines

The simplest solution I have in mind is to add an hwif flag, called "hold" (or whatever better name you find). Drivers like ide/ppc/pmac.c would set this flag for the "hotswap" media bay interface, and not for others.

This change is obviously correct and it doesn't have influence on any existing code.

Quoted 5 lines

The only change to the core code would then be for ide_register_hw to 'skip' those when searching for an available slot, and to call init_hwif_data when (!hwif->present && !hwif->hold) to handle case 2 where the iops & other hwif fields (mmio among others) need to be reset to initial/legacy state.

Quoted 4 lines

Less safe change but also okay, as callers .
btw, Can't "ghost ides" be dealed inside ppc specific code?
Do you know when interface is valid and when it is "ghost", and what other OS-es do in this case?

Not re-using the slot for an interface with the "hold" bit won't affect anybody but setters of that bit, so we are ok. The act of calling init_hwif_data when !present && !hold is the one bringing a possible change of behaviour to existing code.

However, who calls ide_register_hw() dynamically ? ide-cs and ? I don't think there would much harm in re-calling init_hwif_data at this point since hwif->present is not set, we _are_ re-using the hwif, wether it was previously initialized or not by somebody else. In this case, we really want to "clean" it, don't we ?

Right now, the only problem with re-initializing this way that I've found is with hotswap interfaces like ide-pmac, because they will have preset special MMIO ops etc... and that call would revert that pre-setting. That's exactly why such interfaces should set the "hold" bit to "reserve" the hwif slot. You see the point ? I don't think there are much drivers aroung playing with such tricks though.

All I can do within ide pmac itself is set or not that "hold" bit for those interfaces. I just need to set it on the hotswap ones (wthr they have devices connected or not) that way they stay around "reserved" for when a device gets plugged. Other "fixed" interfaces will have hwif->present cleared automatically by the probe code, and thus will be "freed" for other uses, if they don't have any device attached.
(Which is why that init_hwif_data is needed to reset their hwif to something good default, and not whatever ide pmac have set).

In 2.5, I can be slightly smarted since I'm calling the probe myself and no longer rely on the automatic initial probe done by the IDE layer, like for PCI devices, so I can actually "clear" those "empty" interfaces myself after they are probed. But still, it makes sense to have this "hold" flag to let a hotswap interface reserve a slot, and it makes sense when the interface isn't held by anybody to "clean it up" before giving it to somebody else.

The only problem I see right now is for a dynamic interface (like ide-cs) where the _controller_ itself is hotswap, so the hwif slot cannot be reserved in advance _and_ that interface needs special IOps (which is fortunately not the case of ide-cs)

Such an interface can't really know what slot will be picked by ide_register_hw() and can't "prepare" the HWIF with special iops, so it won't be much harmed by the fact we are calling init_hwif_data, but still, we should ultimately think about splitting completely the fact of allocating an hwif slot, setting it up, and triggering a probe on it. Those are 3 different things that are currently mixed in bad ways. I don't beleive fixing that fits in the 2.6 timeframe though.

Ben.

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 9 lines

Yesterday I was thinking about:
- default arch hwifs, added/probed in ide_init_defult_hwifs() if no IDE PCI/PCI (depends on arch, should be IDE PCI?) support is compiled in
- PCI hwifs
- legacy hwifs probed in ide_setup()
- legacy hwifs probed after PCI
and about ide_register_hw() + initializing flag.

Quoted 3 lines

Quoted 1 line

What a mess... ordering issues can make you crazy.

Yup, I'd suggest we think about re-writing all that stuff for 2.7 :)

Yes, only remaining question is how... :-)

Quoted 24 lines

Quoted 14 lines

Quoted 4 lines

The simplest solution I have in mind is to add an hwif flag, called "hold" (or whatever better name you find). Drivers like ide/ppc/pmac.c would set this flag for the "hotswap" media bay interface, and not for others.

This change is obviously correct and it doesn't have influence on any existing code.

Quoted 5 lines

The only change to the core code would then be for ide_register_hw to 'skip' those when searching for an available slot, and to call init_hwif_data when (!hwif->present && !hwif->hold) to handle case 2 where the iops & other hwif fields (mmio among others) need to be reset to initial/legacy state.

Quoted 4 lines

Less safe change but also okay, as callers .
btw, Can't "ghost ides" be dealed inside ppc specific code?
Do you know when interface is valid and when it is "ghost", and what other OS-es do in this case?

Not re-using the slot for an interface with the "hold" bit won't affect anybody but setters of that bit, so we are ok. The act of calling init_hwif_data when !present && !hold is the one bringing a possible change of behaviour to existing code.

Quoted 5 lines

However, who calls ide_register_hw() dynamically ? ide-cs and ? I don't think there would much harm in re-calling init_hwif_data at this point since hwif->present is not set, we _are_ re-using the hwif, wether it was previously initialized or not by somebody else. In this case, we really want to "clean" it, don't we ?

Yes.

Quoted 18 lines

Right now, the only problem with re-initializing this way that I've found is with hotswap interfaces like ide-pmac, because they will have preset special MMIO ops etc... and that call would revert that pre-setting. That's exactly why such interfaces should set the "hold" bit to "reserve" the hwif slot. You see the point ? I don't think there are much drivers aroung playing with such tricks though.

All I can do within ide pmac itself is set or not that "hold" bit for those interfaces. I just need to set it on the hotswap ones (wthr they have devices connected or not) that way they stay around "reserved" for when a device gets plugged. Other "fixed" interfaces will have hwif->present cleared automatically by the probe code, and thus will be "freed" for other uses, if they don't have any device attached.
(Which is why that init_hwif_data is needed to reset their hwif to something good default, and not whatever ide pmac have set).

Quoted 8 lines

In 2.5, I can be slightly smarted since I'm calling the probe myself and no longer rely on the automatic initial probe done by the IDE layer, like for PCI devices, so I can actually "clear" those "empty" interfaces myself after they are probed. But still, it makes sense to have this "hold" flag to let a hotswap interface reserve a slot, and it makes sense when the interface isn't held by anybody to "clean it up" before giving it to somebody else.

Fully agreed.

Quoted 5 lines

The only problem I see right now is for a dynamic interface (like ide-cs) where the _controller_ itself is hotswap, so the hwif slot cannot be reserved in advance _and_ that interface needs special IOps (which is fortunately not the case of ide-cs)

Quoted 8 lines

Such an interface can't really know what slot will be picked by ide_register_hw() and can't "prepare" the HWIF with special iops, so it won't be much harmed by the fact we are calling init_hwif_data, but still, we should ultimately think about splitting completely the fact of allocating an hwif slot, setting it up, and triggering a probe on it. Those are 3 different things that are currently mixed in bad ways. I don't beleive fixing that fits in the 2.6 timeframe though.

Yeah, to be done, probably 2.7 :\.
--
Bartlomiej

Quoted 1 line

Ben.

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