[PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Mark Langsdorf]

The Calxeda SATA phy intermittently fails to bring up a link with Gen3
Retrying the phy hard reset can work around the issue, but the drive
may fail again. In less than 150 out of 15000 test runs, it took more
than 10 tries for the link to be established (but never more than 35).
Triple the maximum observed retry count to provide plenty of margin for
rare events and to guarantee that the link is established.

Also, the default 2 second time-out on a failed drive is too long in
this situation. The uboot implementation of the same driver function
uses a much shorter time-out period and never experiences a time out
issue. Shorten the Linux time-out value for this driver to 500 ms and
keep the other timing constants the same as the stock AHCI driver. This
change was also tested 15000 times on 24 drives and none of them
experienced a time out.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
Changes from v1
	Add const to the timing variable definition
	Added more detail in why the various numbers were chosen

 drivers/ata/sata_highbank.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index b20aa96..005cb13 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -199,7 +199,7 @@ static int highbank_initialize_phys(struct device *dev, void __iomem *addr)
 static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 				unsigned long deadline)
 {
-	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
+	const unsigned long timing[] = { 5, 100, 500};
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
 	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
@@ -207,7 +207,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 	bool online;
 	u32 sstatus;
 	int rc;
-	int retry = 10;
+	int retry = 100;
 
 	ahci_stop_engine(ap);
 
-- 
1.8.1.2

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Timur Tabi]

On 05/30/2013 09:18 AM, Mark Langsdorf wrote:
> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
> +	const unsigned long timing[] = { 5, 100, 500};

You'll save space and time if you also make this array "static",
otherwise the compiler will build the array every time this function is
called.

-- 
Timur Tabi
Software Developer at Calxeda

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Sergei Shtylyov]

Hello.

On 30-05-2013 18:26, Timur Tabi wrote:

>> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>> +	const unsigned long timing[] = { 5, 100, 500};

> You'll save space and time if you also make this array "static",
> otherwise the compiler will build the array every time this function is
> called.

    No, *const* specifier is enough to not build this array every time. 
It will be put into the .const section.

WBR, Sergei

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Timur Tabi]

On 05/30/2013 09:38 AM, Sergei Shtylyov wrote:
>>> >> -	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>>> >> +	const unsigned long timing[] = { 5, 100, 500};
>> > You'll save space and time if you also make this array "static",
>> > otherwise the compiler will build the array every time this function is
>> > called.
>     No, *const* specifier is enough to not build this array every time. 
> It will be put into the .const section.

Ok.  Now that I think about it, that makes sense.

-- 
Timur Tabi
Software Developer at Calxeda

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Tejun Heo]

On Thu, May 30, 2013 at 09:18:29AM -0500, Mark Langsdorf wrote:
> The Calxeda SATA phy intermittently fails to bring up a link with Gen3
> Retrying the phy hard reset can work around the issue, but the drive
> may fail again. In less than 150 out of 15000 test runs, it took more
> than 10 tries for the link to be established (but never more than 35).
> Triple the maximum observed retry count to provide plenty of margin for
> rare events and to guarantee that the link is established.
> 
> Also, the default 2 second time-out on a failed drive is too long in
> this situation. The uboot implementation of the same driver function
> uses a much shorter time-out period and never experiences a time out
> issue. Shorten the Linux time-out value for this driver to 500 ms and
> keep the other timing constants the same as the stock AHCI driver. This
> change was also tested 15000 times on 24 drives and none of them
> experienced a time out.
> 
> Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
> ---
> Changes from v1
> 	Add const to the timing variable definition
> 	Added more detail in why the various numbers were chosen

Can you make the detailed explanation comment in the code so that
people don't have to try to hunt down this commit later on?  The
commit message can explain it briefly and refer to the comment in the
body.

Thanks.

-- 
tejun

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Clemens Ladisch]

Sergei Shtylyov wrote:
> On 30-05-2013 18:26, Timur Tabi wrote:
>>> +    const unsigned long timing[] = { 5, 100, 500};
>
>> You'll save space and time if you also make this array "static",
>> otherwise the compiler will build the array every time this function is
>> called.
>
> No, *const* specifier is enough to not build this array every time. It will be put into the .const section.

gcc disagrees:

$ cat const_static.c
int f(int x) {
	const unsigned long timing[] = { 5, 100, 500};
	static const unsigned long timing2[] = { 5, 100, 500};
	return timing[x] + timing2[x];
}
$ gcc -Os -S const_static.c
$ cat const_static.s
	...
timing2.0:
        .long   5
        .long   100
        .long   500
	...
        movl    8(%ebp), %edx
        movl    $5, -16(%ebp)
        movl    $100, -12(%ebp)
        movl    $500, -8(%ebp)
        movl    timing2.0(,%edx,4), %eax
        addl    -16(%ebp,%edx,4), %eax


Regards,
Clemens

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Tejun Heo]

On Fri, May 31, 2013 at 08:53:37AM +0200, Clemens Ladisch wrote:
> Sergei Shtylyov wrote:
> > On 30-05-2013 18:26, Timur Tabi wrote:
> >>> +    const unsigned long timing[] = { 5, 100, 500};
> >
> >> You'll save space and time if you also make this array "static",
> >> otherwise the compiler will build the array every time this function is
> >> called.
> >
> > No, *const* specifier is enough to not build this array every time. It will be put into the .const section.
> 
> gcc disagrees:

Doesn't really matter this or that way but let's do static const as,
more than anything else, that's what other places are doing.

Thanks.

-- 
tejun

Re: [PATCH v2] ata: increase retry count but shorten duration for Calxeda controller

[Sergei Shtylyov]

Hello.

On 31-05-2013 10:53, Clemens Ladisch wrote:

>>>> +    const unsigned long timing[] = { 5, 100, 500};

>>> You'll save space and time if you also make this array "static",
>>> otherwise the compiler will build the array every time this function is
>>> called.

>> No, *const* specifier is enough to not build this array every time. It will be put into the .const section.

> gcc disagrees:

> $ cat const_static.c
> int f(int x) {
> 	const unsigned long timing[] = { 5, 100, 500};
> 	static const unsigned long timing2[] = { 5, 100, 500};
> 	return timing[x] + timing2[x];
> }
> $ gcc -Os -S const_static.c
> $ cat const_static.s
	...
> timing2.0:
>          .long   5
>          .long   100
>          .long   500
> 	...
>          movl    8(%ebp), %edx
>          movl    $5, -16(%ebp)
>          movl    $100, -12(%ebp)
>          movl    $500, -8(%ebp)
>          movl    timing2.0(,%edx,4), %eax
>          addl    -16(%ebp,%edx,4), %eax

    Hm, I remember I was convinced by somebody just *const* was enough 
for the data to be put to .const section. Don't remember if he gave an 
object code example... Well, it means that person was wrong. Mark, we 
then need *static* as well...

> Regards,
> Clemens

WBR, Sergei