Flaw in ide_unregister()

Flaw in ide_unregister()

[Richard Purdie]

I've been having some problems with calls to ide_unregister() (in ide.c).

This function is declared void which should mean it always succeeds and yet 
it can fail *silently* under the following condition:

if (drive->usage || DRIVER(drive)->busy) goto abort;

(it also fails if (!hwif->present) although that is not a problem)

The driver I've been having problems with is ide-cs.c. Specifically if a CF 
card is removed without ejecting and unmounting the card first. In this 
case, the hardware is gone so we want the ide_unregister call to succeed and 
yet the above code aborts the unregister (silently). This makes it an ide 
problem rather than an ide-cs/pcmcia problem.

There are several solutions:

1. Fix ide_unregister so it always succeeds. (Preferred Solution)
2. Add parameter to ide_unregister to state whether it should abort on 
busy/usage or not. (ugly)
3. Add a return value. What does ide-cs.c do with it though? The hardware is 
gone. (doesn't help)

Only a limited number of drivers use ide_unregister():

drivers/ide/ide-pnp.c
drivers/ide/arm/rapide.c
drivers/ide/legacy/ide-cs.c
drivers/macintosh/mediabay.c

Of these, I can't see anything that would break if we make ide_unregister 
always succeed.

I've tried removing the if statement above and just letting ide_unregister 
succeed but the call then just deadlocks. I had to make some small changes 
to ide.c and ide-disk.c to stop it using interfaces we've marked as dead and 
allow deregistration of dead devices.

The bare minimum of code I needed to make ide_unregister succeed when called 
from ide-cs.c is in the following patch: 
http://www.rpsys.net/openzaurus/ide.patch

There are probably other places checks are needed. I would appreciate it if 
someone could comment on whether these changes can be made to the ide 
drivers, what else may need to be done or if there is an alternative 
solution I've overlooked.

Thanks,

Richard




-- 
No virus found in this outgoing message.
Checked by AVG Anti-Virus.
Version: 7.0.298 / Virus Database: 265.6.7 - Release Date: 30/12/2004

Re: Flaw in ide_unregister()

[Alan Cox]

On Gwe, 2004-12-31 at 12:04, Richard Purdie wrote:
> I've been having some problems with calls to ide_unregister() (in ide.c).
> 
> This function is declared void which should mean it always succeeds and yet 
> it can fail *silently* under the following condition:

Actually in 2.4.x and 2.6.x (except 2.6.9-ac and 2.6.10-ac) its
essentially unusable and full of races and should always be avoided.

> 3. Add a return value. What does ide-cs.c do with it though? The hardware is 
> gone. (doesn't help)

In 2.6.9-ac and 2.6.10-ac the ide_unregister_hwif calls return an error
if the drive is in use. At this point the ide-cs code still throws it
away. The -ac code IDE also adds "removed_hwif_iops" so the bits are
there for the correct result which is something like

	if(ide_unregister_hwif(hwif) < 0 {
		printk("Whine whine...");
		removed_hwif_ops(hwif);
		while(ide_unregister_hwif(hwif) < 0)
			msleep(1000);
	}

I've just not had time yet to propogate this into the drivers and into
the new PCI helper for hotplugging IDE controllers.

Alan

Re: Flaw in ide_unregister()

[Richard Purdie]

Alan Cox:
[Re: ide_unregister and problems with ide-cs.c]
> In 2.6.9-ac and 2.6.10-ac the ide_unregister_hwif calls return an error
> if the drive is in use. At this point the ide-cs code still throws it
> away. The -ac code IDE also adds "removed_hwif_iops" so the bits are
> there for the correct result which is something like
>
> if(ide_unregister_hwif(hwif) < 0 {
> printk("Whine whine...");
> removed_hwif_ops(hwif);
> while(ide_unregister_hwif(hwif) < 0)
> msleep(1000);
> }

I've just tried 2.6.10-ac8 on my target (an arm PDA) and it doesn't help 
ide-cs.c. When I pull out a mounted CF out the socket, the kernel prints 
"Unregister 0 fail 1 0" repeatedly on the console showing the usage count 
permanently stays at 1. This is the same problem as under 2.6.10.

I haven't investigated it yet but I suspect the usage count is held by 
ide-disk as the CF card has a mounted filesystem. As previously mentioned 
and for reference, this patch has the changes I had to make to get standard 
2.6.10 to work:

http://www.rpsys.net/openzaurus/2.6.10/ide_fixes-r1.patch

Richard 



-- 
No virus found in this outgoing message.
Checked by AVG Anti-Virus.
Version: 7.0.300 / Virus Database: 265.6.9 - Release Date: 06/01/2005

Re: Flaw in ide_unregister()

[Alan Cox]

On Sul, 2005-01-09 at 22:37, Richard Purdie wrote:
> I haven't investigated it yet but I suspect the usage count is held by 
> ide-disk as the CF card has a mounted filesystem. As previously mentioned 
> and for reference, this patch has the changes I had to make to get standard 
> 2.6.10 to work:

Correct. This is intentional - what the -ac code allows you to do
(although you probably need to move the final free up to a workqueue) is
to free the hardware resources. The ide resources will then free later
on the umount

Re: Flaw in ide_unregister()

[Richard Purdie]

Alan Cox:
> On Sul, 2005-01-09 at 22:37, Richard Purdie wrote:
>> I haven't investigated it yet but I suspect the usage count is held by
>> ide-disk as the CF card has a mounted filesystem. As previously mentioned
>> and for reference, this patch has the changes I had to make to get 
>> standard
>> 2.6.10 to work:
>
> Correct. This is intentional - what the -ac code allows you to do
> (although you probably need to move the final free up to a workqueue) is
> to free the hardware resources. The ide resources will then free later
> on the umount

Ok. I can see what you're saying and I can visualise a patch for ide-cs.c 
which will probably work using a workqueue as you suggest.

I'd like to question whether the driver or the ide code should be taking 
care of this freeing of resources though?

It all depends what a call to ide_unregister()  is supposed to mean. I'd 
have thought it should mean *remove this inferface by doing whatever is 
neccessary to do so*. If its busy and/or in use, wait until it isn't and 
then remove it by queueing some work to do so at a later date.

Offloading this responsibility onto each and every driver seems rather 
rather unwise and will result in a lot of code duplication. Are there any 
circumstances where we need ide_unregister to abort on busy? Even if there 
are would a flag to indicate what it should do with a busy drive be better?

Richard 

Re: Flaw in ide_unregister()

[Alan Cox]

On Llu, 2005-01-10 at 12:39, Richard Purdie wrote:
> Offloading this responsibility onto each and every driver seems rather 
> rather unwise and will result in a lot of code duplication. Are there any 
> circumstances where we need ide_unregister to abort on busy? Even if there 
> are would a flag to indicate what it should do with a busy drive be better?

Currently there are four things in the -ac tree that use this feature

1.	ISA PnP, where we don't support hotplug (and anyway the only maker of
consumer hotpluggable ISA docking stations I know of - IBM - wont
provide docs on them)
2.	ide-cs
3.	delkin (cardbus IDE)
4.	PCI layer

Of those the PCI one is a common shared function so I put the supporting
logic in the IDE PCI helper function, The others need different handling
at the PCMCIA or Cardbus level in order to free up resources and clean
up.

Re: Flaw in ide_unregister()

[Richard Purdie]

Alan Cox:
> Currently there are four things in the -ac tree that use this feature
>
> 1. ISA PnP, where we don't support hotplug (and anyway the only maker of
> consumer hotpluggable ISA docking stations I know of - IBM - wont
> provide docs on them)
> 2. ide-cs
> 3. delkin (cardbus IDE)
> 4. PCI layer
>
> Of those the PCI one is a common shared function so I put the supporting
> logic in the IDE PCI helper function, The others need different handling
> at the PCMCIA or Cardbus level in order to free up resources and clean
> up.

PCI seems to use __ide_unregister_hwif() directly. Case 2 and 3 seem to need 
the "retry until the device is unregistered" behaviour and case 1 seems to 
be compatible with that.

That suggests ide_unregister_hwif() could call itself back via a work queue 
until the device unregisters successfully. Anything that doesn't want this 
behaviour can use __ide_unregister_hwif() directly as PCI does?

Richard