Re: [PATCH] IDE: Fix HDIO_DRIVE_RESET handling

[Elias Oltmanns <eo@nebensachen.de>]

Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> Hi,
>
> On Thursday 19 June 2008, Elias Oltmanns wrote:
>> Hi Bart,
>> 
>> currently, the code path executing an HDIO_DRIVE_RESET ioctl is broken
>> in various ways.  Firstly, ide_abort() is called with the ide_lock held
>> and may call ide_end_request() further down the line which will try to
>> grab the same lock again.  More importantly though, the whole concept of
>> aborting an inflight request is flawed -- at least as far as ide_abort()
>> is concerned.
>> 
>> The patch below tries to address these issues by handling the
>> HDIO_DRIVE_RESET ioctl in-band.
[...]
> One more thing, ide-2.6 is for syncing with Linus _only_ (it seems I'm to
> blame for the confusion), please rebase your work on top of pata-2.6 quilt
> tree (some comments which should ease the transition below):
>
> 	http://kernel.org/pub/linux/kernel/people/bart/pata-2.6/
>
> [ This tree is pulled into linux-next so you may prefer to just use the
>   latest -next snapshost instead. ]

Indeed, the following patch series is based on nex-20080620.  Just to be
absolutely clear though, this is actually a bug fix since a

# hdparm -w /dev/hda

currently freezes the system if there happens to be any I/O operation in
progress.  Not sure whether this is serious enough for -rc or, indeed,
-stable trees, but I thought I'd mention it.

[...]
>> +	rq->cmd[0] = REQ_DRIVE_RESET;
>> +	rq->cmd_type = REQ_TYPE_SPECIAL;
>> +	rq->cmd_flags |= REQ_SOFTBARRIER;
>> +	ide_do_drive_cmd(drive, rq, ide_end);
>
> ide_do_drive_cmd() now handles only ide_preempt + it seems that previously
> the ioctl would wait till the reset is complete so probably this needs to
> use blk_execute_rq() instead

There is some uncertainty here: Previously, the code didn't actually
wait for the reset to complete before returning but did so right after
the reset sequence had been initiated.  It might be desirable to wait
for completion before returning though and that's the way I've
implemented it in the new patch (first in the series).  This has the
advantage that user space could be informed when anything should have
gone wrong (like a timeout).  This would obviously mean a functional
change visible from user space but then fixing the bug in the first
place is a change visible in user space.  See the fourth patch in the
series for what I've had in mind.

>
> [...]
>
> Oh, and now that you've fixed HDIO_DRIVE_RESET you get the following bonus:
> ide_abort(), __ide_abort() and ide_driver_t.abort all can be removed in the
> incremental patch! ;)

Done.  It's the second in the series.

So here is the summary of the patch series to come:

1. Fixes the buggy implementation of HDIO_DRIVE_RESET handling.
2. Removes all code that has been made superfluous by the previous
   change.
3. Documents the way HDIO_DRIVE_RESET is handled accurately.
4. Adds some more error reporting facilities and documents them as well.

Please be particularly alert when reviewing the last patch.  I merely
did what seemed to be the right and obvious thing to do but I ironed out
some irregularities along the way which (for all improbability) may have
been there for some reason or other.  It beats me, for instance, why
->polling but not ->resetting should be reset to 0 when
sil_sata_reset_poll() returns non zero.  So, I now both are 0 once any
of the poll functions returns ide_stopped.

Regards,

Elias