* Flaw in ide_unregister()
@ 2004-12-31 12:04 Richard Purdie
2005-01-02 14:36 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Richard Purdie @ 2004-12-31 12:04 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Flaw in ide_unregister()
2004-12-31 12:04 Flaw in ide_unregister() Richard Purdie
@ 2005-01-02 14:36 ` Alan Cox
2005-01-09 22:37 ` Richard Purdie
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2005-01-02 14:36 UTC (permalink / raw)
To: Richard Purdie; +Cc: Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Flaw in ide_unregister()
2005-01-02 14:36 ` Alan Cox
@ 2005-01-09 22:37 ` Richard Purdie
2005-01-10 11:09 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Richard Purdie @ 2005-01-09 22:37 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Flaw in ide_unregister()
2005-01-09 22:37 ` Richard Purdie
@ 2005-01-10 11:09 ` Alan Cox
2005-01-10 12:39 ` Richard Purdie
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2005-01-10 11:09 UTC (permalink / raw)
To: Richard Purdie; +Cc: Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Flaw in ide_unregister()
2005-01-10 11:09 ` Alan Cox
@ 2005-01-10 12:39 ` Richard Purdie
2005-01-10 19:23 ` Alan Cox
0 siblings, 1 reply; 7+ messages in thread
From: Richard Purdie @ 2005-01-10 12:39 UTC (permalink / raw)
To: Alan Cox; +Cc: Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Flaw in ide_unregister()
2005-01-10 12:39 ` Richard Purdie
@ 2005-01-10 19:23 ` Alan Cox
2005-01-10 21:17 ` Richard Purdie
0 siblings, 1 reply; 7+ messages in thread
From: Alan Cox @ 2005-01-10 19:23 UTC (permalink / raw)
To: Richard Purdie; +Cc: Linux Kernel Mailing List
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Flaw in ide_unregister()
2005-01-10 19:23 ` Alan Cox
@ 2005-01-10 21:17 ` Richard Purdie
0 siblings, 0 replies; 7+ messages in thread
From: Richard Purdie @ 2005-01-10 21:17 UTC (permalink / raw)
To: Alan Cox; +Cc: Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2005-01-11 2:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-12-31 12:04 Flaw in ide_unregister() Richard Purdie
2005-01-02 14:36 ` Alan Cox
2005-01-09 22:37 ` Richard Purdie
2005-01-10 11:09 ` Alan Cox
2005-01-10 12:39 ` Richard Purdie
2005-01-10 19:23 ` Alan Cox
2005-01-10 21:17 ` Richard Purdie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).