Re: [PATCH] 2.5.15 IDE 62

[Martin Dalecki <dalecki@evision-ventures.com>]

Uz.ytkownik Jens Axboe napisa?:
> On Mon, May 13 2002, Martin Dalecki wrote:
> 
>>Uz.ytkownik Jens Axboe napisa?:
>>
>>>On Mon, May 13 2002, Martin Dalecki wrote:
>>>
>>>
>>>>Mon May 13 12:38:11 CEST 2002 ide-clean-62
>>>>
>>>>- Add missing locking around ide_do_request in do_ide_request().
>>>
>>>
>>>This is broken, do_ide_request() is already called with the request lock
>>>held. tq_disk run -> generic_unplug_device (grab lock) ->
>>>__generic_unplug_device -> do_ide_request(). You just introduced a
>>>deadlock.
>>>
>>>This code would have caused hangs or massive corruption immediately if
>>>ide_lock wasn't ready held there. Not to mention instant spin_unlock
>>>BUG() triggers in queue_command()
>>>
>>
>>Oops. Indeed I see now that the ide_lock is exported to
>>the upper layers above it in ide-probe.c
>>
>>blk_init_queue(q, do_ide_request, &ide_lock);
>>
>>But this is problematic in itself, since it means that
>>we are basically serialiazing between *all* requests
>>on all channels.
> 
> 
> Correct.
> 
> 
>>So I think we should have per channel locks on this level
>>right? This is anyway our unit for serialization.
>>(I'm just surprised that blk_init_queue() doesn't
>>provide queue specific locking and relies on exported
>>locks from the drivers...)
> 
> 
> Sure go ahead and fine grain it, I had no time to go that much into
> detail when ripping out io_request_lock. A drive->lock passed to
> blk_init_queue would do nicely.
> 
> But beware that ide locking is a lot nastier than you think. I saw other
> irq changes earlier, I just want to make sure that you are _absolutely_
> certain that these changes are safe??

Well on the channel level they are safe modulo cmd640 and rz1000.
We can handle them by serializing them on the global lock
in do_ide_request. Like:

if (ch->drive[0].serialized|| ch->drive[1].serialized)
    then
    spin_lock(serialize_lock);

The other case are shared PCI irq's between two channel,
but this case I can easly test on my HPT772 controller card.

You could have observed the hwgroup_t melting down... step by step.