Re: [PATCH 12/12] libata: EH / pio tasks synchronization

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch makes sure that pio tasks are flushed before proceeding
>> with EH.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
>> +static void ata_flush_pio_tasks(struct ata_port *ap)
>> +{
>> +    int tmp = 0;
>> +    unsigned long flags;
>> +
>> +    DPRINTK("ENTER\n");
>> +
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +    ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
>> +    spin_unlock_irqrestore(&ap->host_set->lock, flags);
>> +
>> +    DPRINTK("flush #1\n");
>> +    flush_workqueue(ata_wq);
>> +
>> +    /*
>> +     * At this point, if a task is running, it's guaranteed to see
>> +     * the FLUSH flag; thus, it will never queue pio tasks again.
>> +     * Cancel and flush.
>> +     */
>> +    tmp |= cancel_delayed_work(&ap->pio_task);
>> +    tmp |= cancel_delayed_work(&ap->packet_task);
>> +    if (!tmp) {
>> +        DPRINTK("flush #2\n");
>> +        flush_workqueue(ata_wq);
>> +    }
>> +
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +    ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
>> +    ap->hsm_task_state = HSM_ST_IDLE;
>> +    spin_unlock_irqrestore(&ap->host_set->lock, flags);
> 
> 
> Mostly ok, except
> 
> 1) forcing hsm_task_state to HSM_ST_IDLE seems unlikely to be correct. 
> The error handling code should already be doing that.

Currently no ->eng_timeout does that.  Unless hsm_task_state is useful 
for error analysis, EH handlers would be doing...

ata_flush_pio_tasks(ap);
ap->hsm_task_state = HSM_ST_IDLE;

Currently, it's only ata_qc_timeout and I'm very okay with both.  Do you 
think resetting hsm_task_state should be done in eng_timeout?

> 2) should break this patch up into two pieces:  add and use new helpers, 
> and add flush code.

Sure.

-- 
tejun