diff mbox series

scsi: libsas: Fix disk not being scanned in after being removed

Message ID 20240221073159.29408-1-yangxingui@huawei.com
State New
Headers
Series scsi: libsas: Fix disk not being scanned in after being removed |

Commit Message

yangxingui Feb. 21, 2024, 7:31 a.m. UTC 
  As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
update PHY info"), do discovery will send a new SMP_DISCOVER and update
phy->phy_change_count. We found that if the disk is reconnected and phy
change_count changes at this time, the disk scanning process will not be
triggered.

So update the PHY info with the last query results.

Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

John Garry Feb. 22, 2024, 12:41 p.m. UTC | #1 
On 21/02/2024 07:31, Xingui Yang wrote:
> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
> update PHY info"), do discovery will send a new SMP_DISCOVER and update
> phy->phy_change_count. We found that if the disk is reconnected and phy
> change_count changes at this time, the disk scanning process will not be
> triggered.
> 
> So update the PHY info with the last query results.
> 
> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index a2204674b680..9563f5589948 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
>   		if (*type == 0)
>   			memset(sas_addr, 0, SAS_ADDR_SIZE);
>   	}
> +
> +	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
> +		sas_set_ex_phy(dev, phy_id, disc_resp);
> +
>   	kfree(disc_resp);
>   	return res;
>   }
> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>   		phy->phy_state = PHY_EMPTY;
>   		sas_unregister_devs_sas_addr(dev, phy_id, last);
> -		/*
> -		 * Even though the PHY is empty, for convenience we discover
> -		 * the PHY to update the PHY info, like negotiated linkrate.
> -		 */
> -		sas_ex_phy_discover(dev, phy_id);

It would be nice to be able to call sas_set_ex_phy() here (instead of 
sas_get_phy_attached_dev()), but I assume that you can't do that as the 
disc_resp memory is not available.

If we were to manually set the PHY info here instead, how would that look?

Thanks,
John


>   		return res;
>   	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
>   		   dev_type_flutter(type, phy->attached_dev_type)) {
yangxingui Feb. 23, 2024, 4:04 a.m. UTC | #2 
Hi, John

On 2024/2/22 20:41, John Garry wrote:
> On 21/02/2024 07:31, Xingui Yang wrote:
>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>> phy->phy_change_count. We found that if the disk is reconnected and phy
>> change_count changes at this time, the disk scanning process will not be
>> triggered.
>>
>> So update the PHY info with the last query results.
>>
>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>> update PHY info")
>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index a2204674b680..9563f5589948 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>> domain_device *dev, int phy_id,
>>           if (*type == 0)
>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>       }
>> +
>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>> +        sas_set_ex_phy(dev, phy_id, disc_resp);
>> +
>>       kfree(disc_resp);
>>       return res;
>>   }
>> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct 
>> domain_device *dev, int phy_id,
>>       if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>>           phy->phy_state = PHY_EMPTY;
>>           sas_unregister_devs_sas_addr(dev, phy_id, last);
>> -        /*
>> -         * Even though the PHY is empty, for convenience we discover
>> -         * the PHY to update the PHY info, like negotiated linkrate.
>> -         */
>> -        sas_ex_phy_discover(dev, phy_id);
> 
> It would be nice to be able to call sas_set_ex_phy() here (instead of 
> sas_get_phy_attached_dev()), but I assume that you can't do that as the 
> disc_resp memory is not available.
> 
> If we were to manually set the PHY info here instead, how would that look?
Yes, I think it is indeed better to use sas_set_ex_phy, as you said, 
disc_resp memory is not available. Maybe we can use sas_get_phy_discover 
instead of sas_get_phy_attached_dev so we can use disc_resp?
as follow:
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1940,6 +1940,7 @@ static int sas_rediscover_dev(struct domain_device 
*dev, int phy_id,
         struct expander_device *ex = &dev->ex_dev;
         struct ex_phy *phy = &ex->ex_phy[phy_id];
         enum sas_device_type type = SAS_PHY_UNUSED;
+       struct smp_disc_resp *disc_resp;
         u8 sas_addr[SAS_ADDR_SIZE];
         char msg[80] = "";
         int res;
@@ -1951,33 +1952,41 @@ static int sas_rediscover_dev(struct 
domain_device *dev, int phy_id,
                  SAS_ADDR(dev->sas_addr), phy_id, msg);

         memset(sas_addr, 0, SAS_ADDR_SIZE);
-       res = sas_get_phy_attached_dev(dev, phy_id, sas_addr, &type);
+       disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
+       if (!disc_resp)
+               return -ENOMEM;
+       res = sas_get_phy_discover(dev, phy_id, disc_resp);
..
-               /*
-                * Even though the PHY is empty, for convenience we discover
-                * the PHY to update the PHY info, like negotiated linkrate.
-                */
-               sas_ex_phy_discover(dev, phy_id);
-               return res;
+		if (res == 0)
+               	sas_set_ex_phy(dev, phy_id, disc_resp);
+               goto out;

..

+out:
+	kfree(disc_resp);
+	return res;

Thanks.
Xingui
Jason Yan Feb. 23, 2024, 7:04 a.m. UTC | #3 
On 2024/2/23 12:04, yangxingui wrote:
> Hi, John
> 
> On 2024/2/22 20:41, John Garry wrote:
>> On 21/02/2024 07:31, Xingui Yang wrote:
>>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>>> phy->phy_change_count. We found that if the disk is reconnected and phy
>>> change_count changes at this time, the disk scanning process will not be
>>> triggered.
>>>
>>> So update the PHY info with the last query results.
>>>
>>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>>> update PHY info")
>>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>>> ---
>>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index a2204674b680..9563f5589948 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>>> domain_device *dev, int phy_id,
>>>           if (*type == 0)
>>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>>       }
>>> +
>>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>>> +        sas_set_ex_phy(dev, phy_id, disc_resp);
>>> +
>>>       kfree(disc_resp);
>>>       return res;
>>>   }
>>> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct 
>>> domain_device *dev, int phy_id,
>>>       if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>>>           phy->phy_state = PHY_EMPTY;
>>>           sas_unregister_devs_sas_addr(dev, phy_id, last);
>>> -        /*
>>> -         * Even though the PHY is empty, for convenience we discover
>>> -         * the PHY to update the PHY info, like negotiated linkrate.
>>> -         */
>>> -        sas_ex_phy_discover(dev, phy_id);
>>
>> It would be nice to be able to call sas_set_ex_phy() here (instead of 
>> sas_get_phy_attached_dev()), but I assume that you can't do that as 
>> the disc_resp memory is not available.
>>
>> If we were to manually set the PHY info here instead, how would that 
>> look?
> Yes, I think it is indeed better to use sas_set_ex_phy, as you said, 
> disc_resp memory is not available. Maybe we can use sas_get_phy_discover 
> instead of sas_get_phy_attached_dev so we can use disc_resp?

Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? 
For an empty PHY the other variables means nothing, so why bother get 
and update them?

Thanks,
Jason
yangxingui Feb. 27, 2024, 3:06 a.m. UTC | #4 
Hi Jason,

On 2024/2/23 15:04, Jason Yan wrote:
> On 2024/2/23 12:04, yangxingui wrote:
>> Hi, John
>>
>> On 2024/2/22 20:41, John Garry wrote:
>>> On 21/02/2024 07:31, Xingui Yang wrote:
>>>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>>>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>>>> phy->phy_change_count. We found that if the disk is reconnected and phy
>>>> change_count changes at this time, the disk scanning process will 
>>>> not be
>>>> triggered.
>>>>
>>>> So update the PHY info with the last query results.
>>>>
>>>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>>>> update PHY info")
>>>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>>>> ---
>>>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>>> b/drivers/scsi/libsas/sas_expander.c
>>>> index a2204674b680..9563f5589948 100644
>>>> --- a/drivers/scsi/libsas/sas_expander.c
>>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>>>> domain_device *dev, int phy_id,
>>>>           if (*type == 0)
>>>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>>>       }
>>>> +
>>>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>>>> +        sas_set_ex_phy(dev, phy_id, disc_resp);
>>>> +
>>>>       kfree(disc_resp);
>>>>       return res;
>>>>   }
>>>> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct 
>>>> domain_device *dev, int phy_id,
>>>>       if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>>>>           phy->phy_state = PHY_EMPTY;
>>>>           sas_unregister_devs_sas_addr(dev, phy_id, last);
>>>> -        /*
>>>> -         * Even though the PHY is empty, for convenience we discover
>>>> -         * the PHY to update the PHY info, like negotiated linkrate.
>>>> -         */
>>>> -        sas_ex_phy_discover(dev, phy_id);
>>>
>>> It would be nice to be able to call sas_set_ex_phy() here (instead of 
>>> sas_get_phy_attached_dev()), but I assume that you can't do that as 
>>> the disc_resp memory is not available.
>>>
>>> If we were to manually set the PHY info here instead, how would that 
>>> look?
>> Yes, I think it is indeed better to use sas_set_ex_phy, as you said, 
>> disc_resp memory is not available. Maybe we can use 
>> sas_get_phy_discover instead of sas_get_phy_attached_dev so we can use 
>> disc_resp?
> 
> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? 
> For an empty PHY the other variables means nothing, so why bother get 
> and update them?
The value of the negotiated link rate has two possible values ​​in the 
current processing branch: SAS_LINK_RATE_UNKNOWN and SAS_PHY_DISABLED, 
and both come from disc_resp. If we do not use disc_resp, but set a 
fixed value SAS_PHY_DISABLED for it, it may not be appropriate.

Thanks,
Xingui
Jason Yan Feb. 27, 2024, 7:16 a.m. UTC | #5 
On 2024/2/27 11:06, yangxingui wrote:
> Hi Jason,
> 
> On 2024/2/23 15:04, Jason Yan wrote:
>> On 2024/2/23 12:04, yangxingui wrote:
>>> Hi, John
>>>
>>> On 2024/2/22 20:41, John Garry wrote:
>>>> On 21/02/2024 07:31, Xingui Yang wrote:
>>>>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>>>>> update PHY info"), do discovery will send a new SMP_DISCOVER and 
>>>>> update
>>>>> phy->phy_change_count. We found that if the disk is reconnected and 
>>>>> phy
>>>>> change_count changes at this time, the disk scanning process will 
>>>>> not be
>>>>> triggered.
>>>>>
>>>>> So update the PHY info with the last query results.
>>>>>
>>>>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>>>>> update PHY info")
>>>>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>>>>> ---
>>>>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>>>> b/drivers/scsi/libsas/sas_expander.c
>>>>> index a2204674b680..9563f5589948 100644
>>>>> --- a/drivers/scsi/libsas/sas_expander.c
>>>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>>>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>>>>> domain_device *dev, int phy_id,
>>>>>           if (*type == 0)
>>>>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>>>>       }
>>>>> +
>>>>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>>>>> +        sas_set_ex_phy(dev, phy_id, disc_resp);
>>>>> +
>>>>>       kfree(disc_resp);
>>>>>       return res;
>>>>>   }
>>>>> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct 
>>>>> domain_device *dev, int phy_id,
>>>>>       if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>>>>>           phy->phy_state = PHY_EMPTY;
>>>>>           sas_unregister_devs_sas_addr(dev, phy_id, last);
>>>>> -        /*
>>>>> -         * Even though the PHY is empty, for convenience we discover
>>>>> -         * the PHY to update the PHY info, like negotiated linkrate.
>>>>> -         */
>>>>> -        sas_ex_phy_discover(dev, phy_id);
>>>>
>>>> It would be nice to be able to call sas_set_ex_phy() here (instead 
>>>> of sas_get_phy_attached_dev()), but I assume that you can't do that 
>>>> as the disc_resp memory is not available.
>>>>
>>>> If we were to manually set the PHY info here instead, how would that 
>>>> look?
>>> Yes, I think it is indeed better to use sas_set_ex_phy, as you said, 
>>> disc_resp memory is not available. Maybe we can use 
>>> sas_get_phy_discover instead of sas_get_phy_attached_dev so we can 
>>> use disc_resp?
>>
>> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? 
>> For an empty PHY the other variables means nothing, so why bother get 
>> and update them?
> The value of the negotiated link rate has two possible values ​​in the 
> current processing branch: SAS_LINK_RATE_UNKNOWN and SAS_PHY_DISABLED, 
> and both come from disc_resp. If we do not use disc_resp, but set a 
> fixed value SAS_PHY_DISABLED for it, it may not be appropriate.

OK, makes sense.

> 
> Thanks,
> Xingui
> 
> .
John Garry Feb. 27, 2024, 9:06 a.m. UTC | #6 
On 27/02/2024 07:16, Jason Yan wrote:
>>>
>>> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED here? 
>>> For an empty PHY the other variables means nothing, so why bother get 
>>> and update them?
>> The value of the negotiated link rate has two possible values ​​in the 
>> current processing branch: SAS_LINK_RATE_UNKNOWN and SAS_PHY_DISABLED, 
>> and both come from disc_resp. If we do not use disc_resp, but set a 
>> fixed value SAS_PHY_DISABLED for it, it may not be appropriate.

But we know that the phy is disabled, right? It's our phy, isn't it?

Thanks,
John
yangxingui Feb. 27, 2024, 9:42 a.m. UTC | #7 
Hi John,

On 2024/2/27 17:06, John Garry wrote:
> On 27/02/2024 07:16, Jason Yan wrote:
>>>>
>>>> Can we directly set phy->negotiated_linkrate = SAS_PHY_DISABLED 
>>>> here? For an empty PHY the other variables means nothing, so why 
>>>> bother get and update them?
>>> The value of the negotiated link rate has two possible values ​​in 
>>> the current processing branch: SAS_LINK_RATE_UNKNOWN and 
>>> SAS_PHY_DISABLED, and both come from disc_resp. If we do not use 
>>> disc_resp, but set a fixed value SAS_PHY_DISABLED for it, it may not 
>>> be appropriate.
> 
> But we know that the phy is disabled, right? It's our phy, isn't it?
Yes, just like the previous submission, if we disable phy ourselves 
through the sysfs node, we can configure the negotiation rate to 
SAS_PHY_DISABLED by setting phy->phy->enable to 0. It might be better to 
use sas_set_ex_phy() as you described before, it will refresh other phy 
information synchronously, such as sas_address, device_type, 
target_protocols, etc. If we only update the negotiation rate and 
maintain the old information, is it because it is special? Is it better 
to update phy information uniformly?

Thanks,
Xingui
yangxingui Feb. 28, 2024, 1:13 p.m. UTC | #8 
Hi John,

On 2024/2/22 20:41, John Garry wrote:
> On 21/02/2024 07:31, Xingui Yang wrote:
>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>> phy->phy_change_count. We found that if the disk is reconnected and phy
>> change_count changes at this time, the disk scanning process will not be
>> triggered.
>>
>> So update the PHY info with the last query results.
>>
>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>> update PHY info")
>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index a2204674b680..9563f5589948 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>> domain_device *dev, int phy_id,
>>           if (*type == 0)
>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>       }
>> +
>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>> +        sas_set_ex_phy(dev, phy_id, disc_resp);
>> +
>>       kfree(disc_resp);
>>       return res;
>>   }
>> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct 
>> domain_device *dev, int phy_id,
>>       if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>>           phy->phy_state = PHY_EMPTY;
>>           sas_unregister_devs_sas_addr(dev, phy_id, last);
>> -        /*
>> -         * Even though the PHY is empty, for convenience we discover
>> -         * the PHY to update the PHY info, like negotiated linkrate.
>> -         */
>> -        sas_ex_phy_discover(dev, phy_id);
> 
> It would be nice to be able to call sas_set_ex_phy() here (instead of 
> sas_get_phy_attached_dev()), but I assume that you can't do that as the 
> disc_resp memory is not available.
> 
By the way, I have updated a version and call sas_set_ex_phy() here, 
please check it again.

Thanks,
Xingui
John Garry Feb. 28, 2024, 4:26 p.m. UTC | #9 
On 28/02/2024 13:13, yangxingui wrote:
> Hi John,
> 
> On 2024/2/22 20:41, John Garry wrote:
>> On 21/02/2024 07:31, Xingui Yang wrote:
>>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>>> phy->phy_change_count. We found that if the disk is reconnected and phy
>>> change_count changes at this time, the disk scanning process will not be
>>> triggered.
>>>
>>> So update the PHY info with the last query results.
>>>
>>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>>> update PHY info")
>>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>>> ---
>>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index a2204674b680..9563f5589948 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>>> domain_device *dev, int phy_id,
>>>           if (*type == 0)
>>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>>       }
>>> +
>>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>>> +        sas_set_ex_phy(dev, phy_id, disc_resp);
>>> +
>>>       kfree(disc_resp);
>>>       return res;
>>>   }
>>> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct 
>>> domain_device *dev, int phy_id,
>>>       if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>>>           phy->phy_state = PHY_EMPTY;
>>>           sas_unregister_devs_sas_addr(dev, phy_id, last);
>>> -        /*
>>> -         * Even though the PHY is empty, for convenience we discover
>>> -         * the PHY to update the PHY info, like negotiated linkrate.
>>> -         */
>>> -        sas_ex_phy_discover(dev, phy_id);
>>
>> It would be nice to be able to call sas_set_ex_phy() here (instead of 
>> sas_get_phy_attached_dev()), but I assume that you can't do that as 
>> the disc_resp memory is not available.
>>
> By the way, I have updated a version and call sas_set_ex_phy() here, 
> please check it again.
> 

Then maybe the first patch is a better approach. Let me check it again. 
Sorry for the delay.

John
John Garry Feb. 28, 2024, 6:13 p.m. UTC | #10 
On 21/02/2024 07:31, Xingui Yang wrote:
> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
> update PHY info"), do discovery will send a new SMP_DISCOVER and update
> phy->phy_change_count. We found that if the disk is reconnected and phy
> change_count changes at this time, the disk scanning process will not be
> triggered.
> 
> So update the PHY info with the last query results.
> 
> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to update PHY info")
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index a2204674b680..9563f5589948 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
>   		if (*type == 0)
>   			memset(sas_addr, 0, SAS_ADDR_SIZE);
>   	}
> +
> +	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))

It's odd to call sas_set_ex_phy() if we got res == -ECOMM. I mean, in 
this this case disc_resp is not filled in as the command did not 
execute, right? I know that is what the current code does, but it is 
strange.

> +		sas_set_ex_phy(dev, phy_id, disc_resp);

So can we just call this here when we know that the SMP command was 
executed properly?

Thanks,
John

> +
>   	kfree(disc_resp);
>   	return res;
>   }
> @@ -1972,11 +1976,6 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
>   		phy->phy_state = PHY_EMPTY;
>   		sas_unregister_devs_sas_addr(dev, phy_id, last);
> -		/*
> -		 * Even though the PHY is empty, for convenience we discover
> -		 * the PHY to update the PHY info, like negotiated linkrate.
> -		 */
> -		sas_ex_phy_discover(dev, phy_id);
>   		return res;
>   	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
>   		   dev_type_flutter(type, phy->attached_dev_type)) {
Jason Yan March 1, 2024, 1:55 a.m. UTC | #11 
On 2024/2/29 2:13, John Garry wrote:
> On 21/02/2024 07:31, Xingui Yang wrote:
>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>> phy->phy_change_count. We found that if the disk is reconnected and phy
>> change_count changes at this time, the disk scanning process will not be
>> triggered.
>>
>> So update the PHY info with the last query results.
>>
>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>> update PHY info")
>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index a2204674b680..9563f5589948 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>> domain_device *dev, int phy_id,
>>           if (*type == 0)
>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>       }
>> +
>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
> 
> It's odd to call sas_set_ex_phy() if we got res == -ECOMM. I mean, in 
> this this case disc_resp is not filled in as the command did not 
> execute, right? I know that is what the current code does, but it is 
> strange.

The current code actually re-send the SMP command and update the PHY 
status only when the the SMP command is responded correctly.

Xinggui, can you please fix this and send v3?

Thanks,
Jason
yangxingui March 4, 2024, 12:50 p.m. UTC | #12 
Hi Jason,

On 2024/3/1 9:55, Jason Yan wrote:
> On 2024/2/29 2:13, John Garry wrote:
>> On 21/02/2024 07:31, Xingui Yang wrote:
>>> As of commit d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to
>>> update PHY info"), do discovery will send a new SMP_DISCOVER and update
>>> phy->phy_change_count. We found that if the disk is reconnected and phy
>>> change_count changes at this time, the disk scanning process will not be
>>> triggered.
>>>
>>> So update the PHY info with the last query results.
>>>
>>> Fixes: d8649fc1c5e4 ("scsi: libsas: Do discovery on empty PHY to 
>>> update PHY info")
>>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>>> ---
>>>   drivers/scsi/libsas/sas_expander.c | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index a2204674b680..9563f5589948 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -1681,6 +1681,10 @@ int sas_get_phy_attached_dev(struct 
>>> domain_device *dev, int phy_id,
>>>           if (*type == 0)
>>>               memset(sas_addr, 0, SAS_ADDR_SIZE);
>>>       }
>>> +
>>> +    if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
>>
>> It's odd to call sas_set_ex_phy() if we got res == -ECOMM. I mean, in 
>> this this case disc_resp is not filled in as the command did not 
>> execute, right? I know that is what the current code does, but it is 
>> strange.
> 
> The current code actually re-send the SMP command and update the PHY 
> status only when the the SMP command is responded correctly.
> 
> Xinggui, can you please fix this and send v3?
The current location cannot directly update the phy information. The 
previous phy information will be used later, and the previous sas 
address will be compared with the currently queried sas address. At 
present, v2 is more suitable after many days of testing.

Thanks,
Xingui
diff mbox series

Patch

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index a2204674b680..9563f5589948 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1681,6 +1681,10 @@  int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
 		if (*type == 0)
 			memset(sas_addr, 0, SAS_ADDR_SIZE);
 	}
+
+	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM))
+		sas_set_ex_phy(dev, phy_id, disc_resp);
+
 	kfree(disc_resp);
 	return res;
 }
@@ -1972,11 +1976,6 @@  static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 	if ((SAS_ADDR(sas_addr) == 0) || (res == -ECOMM)) {
 		phy->phy_state = PHY_EMPTY;
 		sas_unregister_devs_sas_addr(dev, phy_id, last);
-		/*
-		 * Even though the PHY is empty, for convenience we discover
-		 * the PHY to update the PHY info, like negotiated linkrate.
-		 */
-		sas_ex_phy_discover(dev, phy_id);
 		return res;
 	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
 		   dev_type_flutter(type, phy->attached_dev_type)) {