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

[Mark Lord <liml@rtr.ca>]

Tejun Heo wrote:
> 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().  
..

Yes, the code already has a loop "outside of sata_link_hardreset()"
for the speed errata handling.  So nothing new there.

And the rest of that routine is a line-by-line clone of ata_std_hardreset().
This is smaller than what other drivers have cloned for these routines,
and a lot better than the old code from sata_mv that it replaces.

The comment in my patch above is just a reminder that someday we could
go back in and address those things.  In *all* LLDs, not just sata_mv.

I guess I'd better stop adding such comments in the future..  :)

Still think it needs any changes ?