Re: [PATCH v3 2/2] scsi: libsas: Add linkrate and sas_addr change detection in rediscover

[yangxingui <yangxingui@huawei.com>]

On 2026/5/21 16:13, John Garry wrote:
> On 15/05/2026 09:45, Xingui Yang wrote:
>> In sas_rediscover_dev(), when detecting a "flutter" condition (same SAS
>> address and compatible device type), the code assumes the device remains
>> unchanged and only handles SATA pending state recovery. However, this
>> approach misses two important scenarios:
>>
>> First, the flutter detection only compares SAS address and device type,
>> ignoring potential linkrate changes that may have already occurred.
>>
>> Second, after sas_ex_phy_discover() re-queries the expander phy, both
>> linkrate and attached SAS address may be updated. The current code does
>> not validate these changes against the existing child device.
>>
>> Add validation checks after sas_ex_phy_discover() to detect linkrate and
>> sas_addr changes. When changes are detected, mark the device as gone and
>> schedule rediscovery via libsas's async discovery pattern:
>> - Set phy_change_count and ex_change_count to -1 to force revalidation
>> - Unregister the device and schedule DISCE_REVALIDATE_DOMAIN event
>> - The old device is destroyed by sas_destruct_devices()
>> - New event triggers discovery via sas_discover_new() since
>>    attached_sas_addr is cleared
>>
>> Suggested-by: John Garry <john.g.garry@oracle.com>
>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_expander.c | 32 ++++++++++++++++++++++++++----
>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index f55ae9a979cd..720db4128727 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -2017,15 +2017,39 @@ static int sas_rediscover_dev(struct 
>> domain_device *dev, int phy_id,
>>           goto out_free_resp;
>>       } else if (SAS_ADDR(sas_addr) == 
>> SAS_ADDR(phy->attached_sas_addr) &&
>>              dev_type_flutter(type, phy->attached_dev_type)) {
>> -        struct domain_device *ata_dev = sas_ex_to_ata(dev, phy_id);
>> +        struct domain_device *child_dev = sas_ex_to_dev(dev, phy_id);
>> +        bool need_rediscover = false;
>>           char *action = "";
>>           sas_ex_phy_discover(dev, phy_id);
>> -        if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING)
>> +        if (child_dev && dev_is_sata(child_dev) &&
>> +            phy->attached_dev_type == SAS_SATA_PENDING) {
>>               action = ", needs recovery";
>> -        pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
>> -             SAS_ADDR(dev->sas_addr), phy_id, action);
>> +        } else if (child_dev && phy->linkrate != child_dev->linkrate) {
>> +            pr_info("ex %016llx phy%02d linkrate changed from %d to 
>> %d\n",
>> +                SAS_ADDR(dev->sas_addr), phy_id,
>> +                child_dev->linkrate, phy->linkrate);
>> +            need_rediscover = true;
>> +        } else if (child_dev &&
>> +               SAS_ADDR(child_dev->sas_addr) != 
>> SAS_ADDR(phy->attached_sas_addr)) {
>> +            pr_info("ex %016llx phy%02d sas_addr changed from %016llx 
>> to %016llx\n",
>> +                SAS_ADDR(dev->sas_addr), phy_id,
>> +                SAS_ADDR(child_dev->sas_addr),
>> +                SAS_ADDR(phy->attached_sas_addr));
>> +            need_rediscover = true;
>> +        }
>> +
>> +        if (need_rediscover) {
>> +            set_bit(SAS_DEV_GONE, &child_dev->state);
>> +            phy->phy_change_count = -1;
>> +            ex->ex_change_count = -1;
> 
> the current code has following:
> 
> 
>      /* we always have to delete the old device when we went here */
>      pr_info("ex %016llx phy%02d replace %016llx\n",
>          SAS_ADDR(dev->sas_addr), phy_id,
>          SAS_ADDR(phy->attached_sas_addr));
>      sas_unregister_devs_sas_addr(dev, phy_id, last);
> 
>      res = sas_discover_new(dev, phy_id);
> 
> Can this be reused (to lose and find the device with updated info)? Or 
> why not good enough?
> 
> I don't know why you need full revalidation.

Hi, John
As the commit log. The existing pattern (unregister + sas_discover_new) 
handles the "replace" case where the SAS address changes completely, 
implying a different device.
For the flutter case where we detect linkrate/sas_addr changes, we 
cannot reuse this synchronous pattern because:
sas_unregister_devs_sas_addr() only marks the device as gone (sets 
SAS_DEV_GONE) and adds it to port->destroy_list. The actual sysfs 
cleanup (sas_rphy_delete) happens later in sas_destruct_devices() called 
at the end of sas_revalidate_domain() in sas_discover.c.
I did try the approach you suggested (unregister + sas_discover_new), 
but it produces sysfs duplicate directory errors:
Workqueue: 0000:74:02.0_disco_q sas_revalidate_domain
Call trace:
  dump_backtrace+0x0/0x18c
  show_stack+0x14/0x1c
  dump_stack+0x88/0xac
  sysfs_warn_dup+0x64/0x7c
  sysfs_create_dir_ns+0x90/0xa0
  kobject_add_internal+0xa0/0x284
  kobject_add+0xb8/0x11c
  device_add+0xe8/0x598
  sas_port_add+0x24/0x50
  sas_ex_discover_devices+0xb10/0xc30

The async pattern with DISCE_REVALIDATE_DOMAIN ensures proper ordering:
1. Old device added to destroy_list
2. Current revalidate work completes → sas_destruct_devices() truly 
deletes old device's sysfs
3. New DISCE_REVALIDATE_DOMAIN event triggers → discovery starts with 
clean sysfs state
This follows libsas's async design pattern similar to sas_resume_devices 
in sas_port.c.

Thanks,
Xingui