[PATCH] [SCSI] LIBSAS: fix libsas link error issue

[PATCH] [SCSI] LIBSAS: fix libsas link error issue

[yxlraid]

From: Xiangliang Yu <yuxiangl@marvell.com>

-- The value of child link rate should is minimum of link rate, or
   command will fail if child link rate is bigger than parent link rate.

Signed-off-by: Xiangliang Yu <yuxiangl@marvell.com>
---
 drivers/scsi/libsas/sas_expander.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 874e29d..6ccca09 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
 			sas_port_add_phy(port, phy->phy);
 		}
 	}
-	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
+	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);
 	child->pathways = min(child->pathways, parent->pathways);
 }
 
-- 
1.7.5.4

Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[James Bottomley]

On Tue, 2011-06-14 at 23:17 +0800, yxlraid@gmail.com wrote:
> From: Xiangliang Yu <yuxiangl@marvell.com>
> 
> -- The value of child link rate should is minimum of link rate, or
>    command will fail if child link rate is bigger than parent link rate.
> 
> Signed-off-by: Xiangliang Yu <yuxiangl@marvell.com>
> ---
>  drivers/scsi/libsas/sas_expander.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 874e29d..6ccca09 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
>  			sas_port_add_phy(port, phy->phy);
>  		}
>  	}
> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> +	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);

This patch doesn't look right.  It will clamp the phy to the minimum
possible link rate.  The child is supposed to support everywhere between
child->min_linkrate and child->max_linkrate.  The reason why we pick the
max is because we should use that if the parent supports it, and fall
back only if the parent isn't capable.

James

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

Subject: Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

On Tue, 2011-06-14 at 23:17 +0800, yxlraid@gmail.com wrote:
>> From: Xiangliang Yu <yuxiangl@marvell.com>
>> 
>> -- The value of child link rate should is minimum of link rate, or
>>    command will fail if child link rate is bigger than parent link rate.
>> 
>> Signed-off-by: Xiangliang Yu <yuxiangl@marvell.com>
>> ---
>>  drivers/scsi/libsas/sas_expander.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>> index 874e29d..6ccca09 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
>>  			sas_port_add_phy(port, phy->phy);
>>  		}
>>  	}
>> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>> +	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);

>This patch doesn't look right.  It will clamp the phy to the minim
>possible link rate.  
Link negotiation should get the minimum.

>The child is supposed to support everywhere between
>child->min_linkrate and child->max_linkrate.  The reason why we pick the
>max is because we should use that if the parent supports it, and fall
>back only if the parent isn't capable.
Now, the problem is command fail, and LIBSAS seem to do nothing.
Do you mean drive need to fix the link error?
Thanks!

PS: The Kernel will panic when link error happening.

Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Douglas Gilbert]

On 11-06-14 09:44 PM, Xiangliang Yu wrote:
> Subject: Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue
>
> On Tue, 2011-06-14 at 23:17 +0800, yxlraid@gmail.com wrote:
>>> From: Xiangliang Yu<yuxiangl@marvell.com>
>>>
>>> -- The value of child link rate should is minimum of link rate, or
>>>     command will fail if child link rate is bigger than parent link rate.
>>>
>>> Signed-off-by: Xiangliang Yu<yuxiangl@marvell.com>
>>> ---
>>>   drivers/scsi/libsas/sas_expander.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>>> index 874e29d..6ccca09 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
>>>   			sas_port_add_phy(port, phy->phy);
>>>   		}
>>>   	}
>>> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>>> +	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);
>
>> This patch doesn't look right.  It will clamp the phy to the minim
>> possible link rate.
> Link negotiation should get the minimum.

spl2r01.pdf [Most recent SAS-3 draft for the protocol layers]

5.7.4.2.1 SAS speed negotiation sequence overview

"The SAS speed negotiation sequence establishes communications between
the two phys on a physical link at the highest possible transmission
rate."


That seems to suggest:
    min(<one_end>.max_linkrate, <other_end>.max_linkrate)

Doug Gilbert

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1965 bytes --]


On 11-06-14 09:44 PM, Xiangliang Yu wrote:
>> Subject: Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue
>>
>> On Tue, 2011-06-14 at 23:17 +0800, yxlraid@gmail.com wrote:
>>>> From: Xiangliang Yu<yuxiangl@marvell.com>
>>>>
>>>> -- The value of child link rate should is minimum of link rate, or
>>>>     command will fail if child link rate is bigger than parent link rate.
>>>>
>>>> Signed-off-by: Xiangliang Yu<yuxiangl@marvell.com>
>>>> ---
>>>>   drivers/scsi/libsas/sas_expander.c |    2 +-
>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>>>> index 874e29d..6ccca09 100644
>>>> --- a/drivers/scsi/libsas/sas_expander.c
>>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>>> @@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
>>>>   			sas_port_add_phy(port, phy->phy);
>>>>   		}
>>>>   	}
>>>> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>>>> +	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);
>>
>>> This patch doesn't look right.  It will clamp the phy to the minim
>>> possible link rate.
>> Link negotiation should get the minimum.

>spl2r01.pdf [Most recent SAS-3 draft for the protocol layers]
>5.7.4.2.1 SAS speed negotiation sequence overview
>"The SAS speed negotiation sequence establishes communications between
>the two phys on a physical link at the highest possible transmission
>rate."
>That seems to suggest:
>    min(<one_end>.max_linkrate, <other_end>.max_linkrate)

Thanks£¡
The current statement like this: child->linkrate = min(child_phy_linkrate, max(parent->max_linkrate, child_phy_linkrate->max_linkrate);

How about this change:
child->linkrate = min(parent->max_linkrate, child->max_linkrate);

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[James Bottomley]

On Tue, 2011-06-14 at 18:44 -0700, Xiangliang Yu wrote:
> Subject: Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue
> 
> On Tue, 2011-06-14 at 23:17 +0800, yxlraid@gmail.com wrote:
> >> From: Xiangliang Yu <yuxiangl@marvell.com>
> >> 
> >> -- The value of child link rate should is minimum of link rate, or
> >>    command will fail if child link rate is bigger than parent link rate.
> >> 
> >> Signed-off-by: Xiangliang Yu <yuxiangl@marvell.com>
> >> ---
> >>  drivers/scsi/libsas/sas_expander.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> >> index 874e29d..6ccca09 100644
> >> --- a/drivers/scsi/libsas/sas_expander.c
> >> +++ b/drivers/scsi/libsas/sas_expander.c
> >> @@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
> >>  			sas_port_add_phy(port, phy->phy);
> >>  		}
> >>  	}
> >> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> >> +	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);
> 
> >This patch doesn't look right.  It will clamp the phy to the minim
> >possible link rate.  
> Link negotiation should get the minimum.

No ... according to the spec it should be the max of what the child
supports taking into account the max the expander phy supports.

> >The child is supposed to support everywhere between
> >child->min_linkrate and child->max_linkrate.  The reason why we pick the
> >max is because we should use that if the parent supports it, and fall
> >back only if the parent isn't capable.
> Now, the problem is command fail, and LIBSAS seem to do nothing.
> Do you mean drive need to fix the link error?
> Thanks!
> 
> PS: The Kernel will panic when link error happening.

Post the panic then ... it's probably a negotiation problem.  If the
child can't support the highest possible link rate because of problems
like transmission errors, it should fall back.

James

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

On Tue, 2011-06-14 at 18:44 -0700, Xiangliang Yu wrote:
>> Subject: Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue
>> 
>> On Tue, 2011-06-14 at 23:17 +0800, yxlraid@gmail.com wrote:
>> >> From: Xiangliang Yu <yuxiangl@marvell.com>
>> >> 
>> >> -- The value of child link rate should is minimum of link rate, or
>> >>    command will fail if child link rate is bigger than parent link rate.
>> >> 
>> >> Signed-off-by: Xiangliang Yu <yuxiangl@marvell.com>
>> >> ---
>> >>  drivers/scsi/libsas/sas_expander.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >> 
>> >> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>> >> index 874e29d..6ccca09 100644
>> >> --- a/drivers/scsi/libsas/sas_expander.c
>> >> +++ b/drivers/scsi/libsas/sas_expander.c
>> >> @@ -638,7 +638,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
>> >>  			sas_port_add_phy(port, phy->phy);
>> >>  		}
>> >>  	}
>> >> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>> >> +	child->linkrate = min(parent_phy->linkrate, child->min_linkrate);
>> 
>> >This patch doesn't look right.  It will clamp the phy to the minim
>> >possible link rate.  
>> Link negotiation should get the minimum.

>it's probably a negotiation problem.  If the
>child can't support the highest possible link rate because of problems
>like transmission errors, it should fall back.

I get the issue like this: 
1. HBA support 3Gbps, level 1 expander support 6Gbps, and level 2 expander support 6Gbps; 
2. level 1 is fine, and find that level 2 link rate is 6Gbps in DISCOVER command response.
3. LIBSAS think level 2 is 6Gbps by sas_ex_get_linkrate function, and send command to level 2 expander
4. LIBSAS link error.

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

> 
> I get the issue like this:
> 1. HBA support 3Gbps, level 1 expander support 6Gbps, and level 2 expander
> support 6Gbps;
> 2. level 1 is fine, and find that level 2 link rate is 6Gbps in DISCOVER
command
> response.
> 3. LIBSAS think level 2 is 6Gbps by sas_ex_get_linkrate function, and send
> command to level 2 expander
> 4. LIBSAS link error.
> 
[Jack Wang] 
What do you mean by "LIBSAS link error", OPEN_REJECT(CONNECTION RATE NOT
SUPPORTED)? LIBSAS only get the error event through LLDD. 
According to SAS2r15 7.8.3

A SAS initiator port shall set the initial CONNECTION RATE field to:
a) the highest supported connection rate supported by a potential pathway as
determined during the 
discover process (e.g., based on the logical link rates of each logical link
reported in the SMP 
DISCOVER responses); or
b) the logical link rate of the logical phy used to transmit the OPEN
address frame.
If a SAS initiator port selected a connection rate based on discover process
information but the connection 
request results in OPEN_REJECT (CONNECTION RATE NOT SUPPORTED), then the
discover process 
information is no longer current and the discover process should be run
again.

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

>> I get the issue like this:
>> 1. HBA support 3Gbps, level 1 expander support 6Gbps, and level 2 expander
>> support 6Gbps;
>> 2. level 1 is fine, and find that level 2 link rate is 6Gbps in DISCOVER
command
>> response.
>> 3. LIBSAS think level 2 is 6Gbps by sas_ex_get_linkrate function, and send
>> command to level 2 expander
>> 4. LIBSAS link error.
>> 
>[Jack Wang] 
>What do you mean by "LIBSAS link error", OPEN_REJECT(CONNECTION RATE NOT
>SUPPORTED)? LIBSAS only get the error event through LLDD. 
>According to SAS2r15 7.8.3
I mean that HBA get link error. Sorry.

>A SAS initiator port shall set the initial CONNECTION RATE field to:
>a) the highest supported connection rate supported by a potential pathway as
determined during the 
>discover process (e.g., based on the logical link rates of each logical link
reported in the SMP 
>DISCOVER responses); or
Sas_ex_get_linkrate function

>b) the logical link rate of the logical phy used to transmit the OPEN
address frame.
MVSAS get value of linkrate from sas_ex_get_linkrate function(child->linkrate)

>If a SAS initiator port selected a connection rate based on discover process
information but the connection 
>request results in OPEN_REJECT (CONNECTION RATE NOT SUPPORTED), then the
discover process 
>information is no longer current and the discover process should be run
again.

Fail again.

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

> >> I get the issue like this:
> >> 1. HBA support 3Gbps, level 1 expander support 6Gbps, and level 2
expander
> >> support 6Gbps;
> >> 2. level 1 is fine, and find that level 2 link rate is 6Gbps in
DISCOVER
> command
> >> response.
> >> 3. LIBSAS think level 2 is 6Gbps by sas_ex_get_linkrate function, and
send
> >> command to level 2 expander
> >> 4. LIBSAS link error.
> >>
> >[Jack Wang]
> >What do you mean by "LIBSAS link error", OPEN_REJECT(CONNECTION RATE NOT
> >SUPPORTED)? LIBSAS only get the error event through LLDD.
> >According to SAS2r15 7.8.3
> I mean that HBA get link error. Sorry.
> 
> >A SAS initiator port shall set the initial CONNECTION RATE field to:
> >a) the highest supported connection rate supported by a potential pathway
as
> determined during the
> >discover process (e.g., based on the logical link rates of each logical
link
> reported in the SMP
> >DISCOVER responses); or
> Sas_ex_get_linkrate function
> 
[Jack Wang] 
It's the same, sas_ex_get_linkrate use DISCOVER response as linkrate.

> >b) the logical link rate of the logical phy used to transmit the OPEN
> address frame.
> MVSAS get value of linkrate from sas_ex_get_linkrate
> function(child->linkrate)
> 
> >If a SAS initiator port selected a connection rate based on discover
process
> information but the connection
> >request results in OPEN_REJECT (CONNECTION RATE NOT SUPPORTED), then the
> discover process
> >information is no longer current and the discover process should be run
> again.
> 
> Fail again.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

> [Jack Wang] 
>It's the same, sas_ex_get_linkrate use DISCOVER response as linkrate.
But the function have problem, actually, the error statement is:
child->linkrate = min(parent_phy->linkrate, child->max_linkrate); 
its mean like this:
child->linkrate = min(child_phy->linkrate, max(parent->max_linkrate,child_phy->linkrate));
and if parent->max_linkrate(3Gbps) is less than child_phy->linkrate(6Gbps),
the statement will be change this:
child->linkrate = child_phy->linkrate, forget the parent linkrate.

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

> > [Jack Wang]
> >It's the same, sas_ex_get_linkrate use DISCOVER response as linkrate.
> But the function have problem, actually, the error statement is:
> child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> its mean like this:
> child->linkrate = min(child_phy->linkrate,
> max(parent->max_linkrate,child_phy->linkrate));
> and if parent->max_linkrate(3Gbps) is less than
child_phy->linkrate(6Gbps),
> the statement will be change this:
> child->linkrate = child_phy->linkrate, forget the parent linkrate.
[Jack Wang] 
I don't think the statement below is error:
child->linkrate = min(parent_phy->linkrate, child->max_linkrate);

parent_phy->linkrate is came from sas_set_ex_phy which will set the linkrate
to negotiated logical linkrate. For your eg: you topo like this:
hba(3G)---expander1(6G)---expander2(6G):

Then expander1's linkrate will set to 3G, and expander2's linkrate will set
to 6G, that is correct. But the connection rate will be 3G from hba to
expander2.

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

>> > [Jack Wang]
>> >It's the same, sas_ex_get_linkrate use DISCOVER response as linkrate.
>> But the function have problem, actually, the error statement is:
>> child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>> its mean like this:
>> child->linkrate = min(child_phy->linkrate,
>> max(parent->max_linkrate,child_phy->linkrate));
>> and if parent->max_linkrate(3Gbps) is less than
child_phy->linkrate(6Gbps),
>> the statement will be change this:
>> child->linkrate = child_phy->linkrate, forget the parent linkrate.
>[Jack Wang] 
>I don't think the statement below is error:
>child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>parent_phy->linkrate is came from sas_set_ex_phy which will set the linkrate
>to negotiated logical linkrate. For your eg: you topo like this:
>hba(3G)---expander1(6G)---expander2(6G):

Yes, you can test like this.
>Then expander1's linkrate will set to 3G, and expander2's linkrate will set
>to 6G, that is correct.
yes
> But the connection rate will be 3G from hba to
>expander2.
How to configure the connection rate?
Now, MVSAS driver get 6G from sas_ex_get_linkrate function, and set linkrate of OPEN address frame to the value. right?

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

> >> > [Jack Wang]
> >> >It's the same, sas_ex_get_linkrate use DISCOVER response as linkrate.
> >> But the function have problem, actually, the error statement is:
> >> child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> >> its mean like this:
> >> child->linkrate = min(child_phy->linkrate,
> >> max(parent->max_linkrate,child_phy->linkrate));
> >> and if parent->max_linkrate(3Gbps) is less than
> child_phy->linkrate(6Gbps),
> >> the statement will be change this:
> >> child->linkrate = child_phy->linkrate, forget the parent linkrate.
> >[Jack Wang]
> >I don't think the statement below is error:
> >child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> >parent_phy->linkrate is came from sas_set_ex_phy which will set the
linkrate
> >to negotiated logical linkrate. For your eg: you topo like this:
> >hba(3G)---expander1(6G)---expander2(6G):
> 
> Yes, you can test like this.
> >Then expander1's linkrate will set to 3G, and expander2's linkrate will
set
> >to 6G, that is correct.
> yes
> > But the connection rate will be 3G from hba to
> >expander2.
> How to configure the connection rate?
> Now, MVSAS driver get 6G from sas_ex_get_linkrate function, and set
linkrate
> of OPEN address frame to the value. right?
> 
> --
[Jack Wang] 
Connection rate is auto, link layer will insert deletable primitives to do
rate matching (sas2r15 7.14).

Yes, you should set the linkrate to that value .
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

> >[Jack Wang]
> >I don't think the statement below is error:
> >child->linkrate = min(parent_phy->linkrate, child->max_linkrate);

The parent_phy is same to child_phy, I don't think it's right.

>> Yes, you can test like this.
>> >Then expander1's linkrate will set to 3G, and expander2's linkrate will
set
>> >to 6G, that is correct.
>> yes
>> > But the connection rate will be 3G from hba to
>> >expander2.
>> How to configure the connection rate?
>> Now, MVSAS driver get 6G from sas_ex_get_linkrate function, and set
linkrate
>> of OPEN address frame to the value. right?
>> 
>> --
>[Jack Wang] 
>Connection rate is auto, link layer will insert deletable primitives to do
>rate matching (sas2r15 7.14).

>Yes, you should set the linkrate to that value .

OK.MVSAS driver seem right. But, the problem is command fail because of link error, that is why I commit this patch. And my patch work fine.
You can reproduce the issue and fix it. Thanks!

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

> > >[Jack Wang]
> > >I don't think the statement below is error:
> > >child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> 
> The parent_phy is same to child_phy, I don't think it's right.
> 
[Jack Wang] The parent_phy->linkrate is report from DISCOVER response as
negotiated link rate(parent phy with child phy), so same is right.

> >> Yes, you can test like this.
> >> >Then expander1's linkrate will set to 3G, and expander2's linkrate
will
> set
> >> >to 6G, that is correct.
> >> yes
> >> > But the connection rate will be 3G from hba to
> >> >expander2.
> >> How to configure the connection rate?
> >> Now, MVSAS driver get 6G from sas_ex_get_linkrate function, and set
> linkrate
> >> of OPEN address frame to the value. right?
> >>
> >> --
> >[Jack Wang]
> >Connection rate is auto, link layer will insert deletable primitives to
do
> >rate matching (sas2r15 7.14).
> 
> >Yes, you should set the linkrate to that value .
> 
> OK.MVSAS driver seem right. But, the problem is command fail because of
link
> error, that is why I commit this patch. And my patch work fine.
> You can reproduce the issue and fix it. Thanks!
[Jack Wang]  I re-consider it , you maybe should compare the HBA linkrate
with the child phy linkrate and chose a minor.

I

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

>> > >[Jack Wang]
>> > >I don't think the statement below is error:
>> > >child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>> 
>> The parent_phy is same to child_phy, I don't think it's right.
>> 
>[Jack Wang] The parent_phy->linkrate is report from DISCOVER response as
>negotiated link rate(parent phy with child phy), so same is right.

Parent_phy = &parent->ex_dev.ex_phy[phy_id]; (sas_ex_discover_expander)
struct expander_device *parent_ex = &parent->ex_dev;(sas_ex_get_linkrate)
struct ex_phy *phy = &parent_ex->ex_phy[i];
so, parent_phy and phy are same one variable.

>> >> Yes, you can test like this.
>> >> >Then expander1's linkrate will set to 3G, and expander2's linkrate
will
>> set
>> >> >to 6G, that is correct.
>> >> yes
>> >> > But the connection rate will be 3G from hba to
>> >> >expander2.
>> >> How to configure the connection rate?
>> >> Now, MVSAS driver get 6G from sas_ex_get_linkrate function, and set
>> linkrate
>> >> of OPEN address frame to the value. right?
>> >>
>> >> --
>> >[Jack Wang]
>> >Connection rate is auto, link layer will insert deletable primitives to
do
>> >rate matching (sas2r15 7.14).
>> 
>> >Yes, you should set the linkrate to that value .
>> 
>> OK.MVSAS driver seem right. But, the problem is command fail because of
link
>> error, that is why I commit this patch. And my patch work fine.
>> You can reproduce the issue and fix it. Thanks!
>[Jack Wang]  I re-consider it , you maybe should compare the HBA linkrate
>with the child phy linkrate and chose a minor.

Yes, how about this:
-	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
 +	child->linkrate = min(parent->max_linkrate, child->max_linkrate);
The max link rate of parent have HBA link rate info:
dev->max_linkrate = port->linkrate; (in sas_get_port_device function).

Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

> >> > >[Jack Wang]
> >> > >I don't think the statement below is error:
> >> > >child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
> >>
> >> The parent_phy is same to child_phy, I don't think it's right.
> >>
> >[Jack Wang] The parent_phy->linkrate is report from DISCOVER response as
> >negotiated link rate(parent phy with child phy), so same is right.
> 
> Parent_phy = &parent->ex_dev.ex_phy[phy_id]; (sas_ex_discover_expander)
> struct expander_device *parent_ex = &parent->ex_dev;(sas_ex_get_linkrate)
> struct ex_phy *phy = &parent_ex->ex_phy[i];
> so, parent_phy and phy are same one variable.
> 
> >> >> Yes, you can test like this.
> >> >> >Then expander1's linkrate will set to 3G, and expander2's linkrate
> will
> >> set
> >> >> >to 6G, that is correct.
> >> >> yes
> >> >> > But the connection rate will be 3G from hba to
> >> >> >expander2.
> >> >> How to configure the connection rate?
> >> >> Now, MVSAS driver get 6G from sas_ex_get_linkrate function, and set
> >> linkrate
> >> >> of OPEN address frame to the value. right?
> >> >>
> >> >> --
> >> >[Jack Wang]
> >> >Connection rate is auto, link layer will insert deletable primitives
to
> do
> >> >rate matching (sas2r15 7.14).
> >>
> >> >Yes, you should set the linkrate to that value .
> >>
> >> OK.MVSAS driver seem right. But, the problem is command fail because of
> link
> >> error, that is why I commit this patch. And my patch work fine.
> >> You can reproduce the issue and fix it. Thanks!
> >[Jack Wang]  I re-consider it , you maybe should compare the HBA linkrate
> >with the child phy linkrate and chose a minor.
> 
> Yes, how about this:
> -	child->linkrate = min(parent_phy->linkrate, child->max_linkrate);
>  +	child->linkrate = min(parent->max_linkrate, child->max_linkrate);
> The max link rate of parent have HBA link rate info:
> dev->max_linkrate = port->linkrate; (in sas_get_port_device function).

[Jack Wang] 
No, I mean you when you set connection rate in OPEN Address frame, you
compare your HBA linkrate with the linkrate of the phy you want to open, and
chose the minor.

RE: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Xiangliang Yu]

>[Jack Wang] 
>No, I mean you when you set connection rate in OPEN Address frame, you
>compare your HBA linkrate with the linkrate of the phy you want to open, and
>chose the minor.

I don't understand. What is the purpose of sas_ex_get_linkrate function?

Re: [PATCH] [SCSI] LIBSAS: fix libsas link error issue

[Jack Wang]

 
> >[Jack Wang]
> >No, I mean you when you set connection rate in OPEN Address frame, you
> >compare your HBA linkrate with the linkrate of the phy you want to open,
and
> >chose the minor.
> 
> I don't understand. What is the purpose of sas_ex_get_linkrate function?
[Jack Wang] 
As I understand, this function set the linkrate(pathways max/min linkrate)
of the child device(expander or end device) attached to parent expander
parent, through parent_phy, if there's wide port also handle that.