Re: [PATCH 4/5] sata_mv new mv_sata_hardreset handler

[Tejun Heo <htejun@gmail.com>]

Mark Lord wrote:
> Tejun Heo wrote:
>> Mark Lord wrote:
> ..
>>> +    /* FIXME:
>>> +     * Except for the outer do-while construct below, this function
>>> +     * is an exact clone of sata_std_hardreset() from libata-core.c.
>>> +     *
>>> +     * Once this driver is stable, we should re-org libata so we can 
>>> share
>>> +     * more of that code, rather than duplicating so much of it here
>>> +     * and in other drivers.
>>> +     */
>>
>> After modularize patchsets, sata_link_hardreset() does all the chores 
>> needed around hardreset and sata_mv should be able to just build a 
>> loop around it.
> ..
> 
> Mmm... I don't see how this helps.
> 
> The bulk of mv_sata_hardreset() is from sata_std_hardreset().
> 
> The only part those two do *not* have in common, is that
> sata_mv needs to do it's own equivalent of sata_link_hardreset(),
> so sata_link_hardreset() cannot be reused here.  Wrapper or not.
> 
> Now, if we had a per-LLD .link_hardreset op, defaulting to 
> sata_link_hardreset,
> then this would be trivial.

The MV specific part is retry-if-offline w/ lower link speed, right? 
You can do that just as well by looping outside of 
sata_link_hardreset().  It will fail fast if link is offline (not very 
different from your inner loop) and do everything you need to w/ proper 
check_ready() callback on success path.  Am I missing something?

Thanks.

-- 
tejun