Re: [PATCH] IDE: Fix HDIO_DRIVE_RESET handling

[Elias Oltmanns <eo@nebensachen.de>]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> in various ways.  Most importantly, it is treated as an out of band
>> request in an illegal way which may very likely lead to system lock ups.
>> Use the drive's request queue to avoid this problem (and fix a locking
>> issue for free along the way).
>
> It was always designed to be, and used out of band. One of the important
> uses of the ioctl is to abort a running command when an interface has
> jammed up. If you end up queueing it behind that command you've lost most
> of the reason for the ioctl anyway (and you might as well just remove it
> really given SG_IO exists).

Well, I can see your point. In fact, there really doesn't seem to be an
alternative to the out of band approach for the purposes you described.
Now, I even think that I could perhaps fix the request aborting properly
and restore the original behaviour. Moreover, I may very likely live to
regret having removed ide_abort() and friends when implementing disk
shock protection in the IDE layer. Maybe I should try to send an
alternative patch for discussion. On the other hand I don't see the
equivalent for HDIO_DRIVE_RESET in libata which makes me wonder whether
this ioctl has actually been used in real life for the purposes you
described.

>
> Other than the command aborting bit, it looks a good idea - that code
>has
> always been racy and raced against timer handlers, irq handlers and if
> neither of them got it then a speed changedown raced the lot 8(

My idea to solve this would be roughly this: Change ide_set_handler to
leave the ->handler and ->expiry members alone if they have been set on
entry. If a request is being processed by the time a HDIO_DRIVE_RESET
ioctl is received, these callbacks will be changed so the reset sequence
will be started on the next interrupt, timeout, or when the ->busy flag
is cleared. I'm not quite sure yet whether things will work out the way
I want them to and I don't know whether HDIO_DRIVE_RESET actually
justifies the effort since I don't knowof an equivalent in libata
anyway. But as I said, it might come in handy for other purposes.

Comments?

Elias